Re: [PATCH 04/10] Refactoring: new $automake_remake_options global variable.

[Stefano Lattarini]

On Sunday 02 January 2011, Ralf Wildenhues wrote:
> Hi Stefano,
> 
> * address@hidden wrote on Thu, Dec 23, 2010 at 12:27:40PM CET:
> > This change is useful only in view of soon-to-follow refactorings
> > and simplifications, related to the fixing of Automake bug#7669
> > a.k.a. PR/547.
> > 
> > * automake.in (%am_remake_options): New global hash variable.
> > (parse_arguments): Initialize it.
> > (scan_autoconf_traces) [$macro eq 'AM_INIT_AUTOMAKE']: Update it.
> > (handle_configure): Use it.
> > * tests/remake-am-opts.test: New test.
> > * tests/Makefile.am (TESTS): Update.
> 
> This is the first patch I'm having high-level concerns with.  Basically,
> I don't understand why patches 04, 05, 06, and 09 are needed at all for
> fixing the particular issue this patch series aims to fix.
>
Well, while they might not be *strictly* needed, they removed some
unnecessary complexity and *greatly* helped me realize what steps I
had to take in order to fix the bug in question [1].  These steps
might become more complicated or numerous without the changes in
the patches you're referring to.

[1] When a refactoring helps in making a bugfix (almost) obvious,
    I think it has very good chances of being a good refactoring.

> I also don't think that moving from module-local variables and
> concepts (options vs. global_options)
>
That was maybe a good distinction when it was first introduced; but
that was before automake started to trace autoconf macros and to allow
options to be specified in AM_INIT_AUTOMAKE.  Now the distinction has
bitrotten somewhat -- if we really still want a meaningful distinction,
we should aim at a threefold one:
  (cmdline options) vs. (global options) vs. (local options)

But such a more granular distinction is not needed ATM, so I've preferred
to go by the shortest route.  Once the bugfix and the tests are in place,
we could go for a second round of refactorings.

Additionaly, you can easily see that the pre-existing distinction between
global and local automake options was *really* needed *only* in the
`handle_configure' subroutine, so the complexity it entailed was IMHO
not really justified (at least w.r.t. the current form of the codebase).

> toward global variables like am_remake_options would be
> an improvement.
>
In the long run, that variable should disappear too; i.e., either
we are going not to meddle with command-line options outside of
the parse_arguments subroutine [1], or we are going to introduce
the threefold distinction I cited above.

[1] This might be the case if we'll decide that automake-generated
    rebuild rules should not propagate command-line options derived
    either from the AM_INIT_AUTOMAKE call or from the first automake
    invocation.  But that's material for another thread (I have an
    unfinished patch about this in my local repository, but it depends
    on the current patch series being applied first).

> On the contrary, so far I've thought that the
> distinction and current API would be a good one, no?  What makes it
> unsuitable?
>
I hope you'll find the above argumentations and explanations convincing
enough as an answer to this question.

> Why are you even changing code that deals with the rerun arguments if
> you are not fixing the issues with it?  This is not a rhetorical
> question, I might again just be overlooking all the keys.
>
Because I need some refactorings to make my fix look "simple & natural",
but those refactorings might have an impact on the external behaviour of
the automake-generated rebuild rules if those other modifications you're
referring to are not applied first.

To put it in another way,  we are trading some old complexity with some
new complications in order to keep the automake behaviour as unchanged
as possible in this patch series; in the long run, we should remove this
additional complication by either changing the semantics of the automake
rebuild rules (if that's deemed correct), or by introducing a more
proper complexity (with less "historical burdens" attached to it).

> I have lower-level nits as well, in this and the other patches (see
> below for some), but I think it is better if I wait for your reply to
> above before continuing with review.  I'm just leaving them in since
> I've written them already.
>
> And just so you don't misunderstand me:
>
Oh, I hope you don't deem me susceptible enough to be offended by
constructive criticism and technical objections ;-)

> I think it's great that you have
> attacked this bug and written this patch series!  I just want to make
> sure we don't regress in code quality while fixing user-visible bugs.
>
I honestly do believe that this patch series might be a useful first
step in a journey to tidy up the automake codebase.  I hope not to
be fooling o deluding myself on this!

> Thanks,
> Ralf
> 
Thanks for the review.  My replies to your remarks and suggestions
are in-line below.

> >  2010-12-20  Stefano Lattarini  <address@hidden>
> >  
> > +   Refactoring: new $automake_remake_options global variable.
> 
> The variable name referred to here does not appear in the patch
> contents.
>
Oops, leftover of a previous version of the patch.  Fixed now.

> > +   This change is useful only in view of soon-to-follow refactorings
> > +   and simplifications, related to the fixing of Automake bug#7669
> > +   a.k.a. PR/547.
> > +   * automake.in (%am_remake_options): New global hash variable.
> > +   (parse_arguments): Initialize it.
> > +   (scan_autoconf_traces) [$macro eq 'AM_INIT_AUTOMAKE']: Update it.
> > +   (handle_configure): Use it.
> > +   * tests/remake-am-opts.test: New test.
> > +   * tests/Makefile.am (TESTS): Update.
> 
> > diff --git a/automake.in b/automake.in
> > index fd00369..9bef982 100644
> > --- a/automake.in
> > +++ b/automake.in
> > @@ -298,6 +298,9 @@ use constant QUEUE_STRING    => "string";
> >  ## Variables related to the options.  ##
> >  ## ---------------------------------- ##
> >  
> > +# Options to be passed to automake in the generated remake rules.
> > +my %am_remake_options;
> > +
> >  # TRUE if we should always generate Makefile.in.
> >  my $force_generation = 1;
> >  
> > @@ -4263,9 +4266,9 @@ sub handle_configure ($$$@)
> >    push @configuredeps, '$(ACLOCAL_M4)' if -f 'aclocal.m4';
> >    define_pretty_variable ('am__configure_deps', TRUE, INTERNAL,
> >                       @configuredeps);
> > -
> > -  my $automake_options = '--' . (global_option 'cygnus' ? 'cygnus' : 
> > $strictness_name)
> > -                    . (global_option 'no-dependencies' ? ' --ignore-deps' 
> > : '');
> > +  my $automake_options = '--' . $am_remake_options{strictness}
> > +                         . ($am_remake_options{ignore_deps} ?
> > +                            ' --ignore-deps' : '');
> 
> Wow.  I didn't know it was perl-strict-clean to put barewords inside
> hash derefs a la $hash{word}.
>
Yes, for good or for bad, it is. Unfortunately, I cannot remember if
there's an easy way to forbid the use of barewords as hash keys
(google is not much helpulf on the issue).

> That seems like an undesirable feature
> to me, because it is easy to forget a $ as in $hash{$word}.
> Can we please not make use of this, it looks like a typo above already
> (with $strictness being a valid variable name)?  Thanks.
>
Fine with me; I amended the patch to do so.

> >    $output_rules .= file_contents
> >      ('configure',
> > @@ -5460,9 +5463,16 @@ sub scan_autoconf_traces ($)
> >         }
> >       elsif (defined $args[1])
> >         {
> > +         my @opts = split (' ', $args[1]);
> > +         foreach my $opt (@opts)
> > +           {
> > +             $am_remake_options{strictness} = $opt
> > +               if $opt =~ /^(cygnus|foreign|gnits|gnu)$/;
> > +             $am_remake_options{ignore_deps} = 1
> > +               if $opt eq 'no-dependencies';
> > +           }
> >           exit $exit_code
> > -           if (process_global_option_list ($where,
> > -                                           split (' ', $args[1])));
> > +           if process_global_option_list ($where, @opts);
> >         }
> >     }
> >        elsif ($macro eq 'AM_MAINTAINER_MODE')
> > @@ -8501,6 +8511,9 @@ sub parse_arguments ()
> >      &parse_warnings('-W', $warning);
> >    }
> >  
> > +  $am_remake_options{strictness} = ($cygnus ? 'cygnus' : $strictness);
> > +  $am_remake_options{ignore_deps} = $ignore_deps;
> > +
> >    return unless @ARGV;
> >  
> >    if ($ARGV[0] =~ /^-./)
> 
> > --- /dev/null
> > +++ b/tests/remake-am-opts.test
> > @@ -0,0 +1,195 @@
> 
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +
> > +#
> 
> Extra line.
>
Oops. Removed.

> > +# Test that remake rules calls automake with proper command-line
> > +# options derived from options in AM_INIT_AUTOMAKE, or passed on
> > +# the command line of the original invocation.
> > +#
> > +# NOTE: That just described above might not be a very smart semantic,
> 
> These semantics might not be smart after all ...
>
Yes, better.

> > +# after all.  See this message related to automake bug #7673:
> > +#  <http://debbugs.gnu.org/cgi/bugreport.cgi?bug=7673#11>
> > +# Update this testcase if the semantic is changed.
> > +#
> 
> Ditto.
>
Fixed.

> It seems like this test would be testing functionality that is dubious
> at best and wrong at worst.  Why are we doing this?
>
To ensure that the later patches doesn't change this functionality
behind our backs. If we break backward compatibility, even for dubious
features, we should do so consciously and deliberately, and not through
unexpected side effects of fundamentally unrelated changes.

> > +
> > +. ./defs || Exit 1
> > +
> > +set -e
> > +
> > +write_automake_wrapper ()
> > +{
> > +   d='#'
> > +   strictness=
> > +   while test $# -gt 0; do
> > +     echo "info: process option $1" >&2
> > +     case $1 in
> > +       -S) strictness=$2; shift 2;;
> > +       -I) d=' '; shift;;
> > +       *) Exit 99;;
> > +     esac
> > +   done
> > +   test -n "$strictness" || Exit 99
> > +   cat <<END
> > +#!$SHELL
> > +seen_strictness=false
> > +${d}seen_ignore_deps=false
> > +while test \$# -gt 0; do
> > +  case \$1 in
> > +    --$strictness) seen_strictness=: ;;
> > +$d  --ignore-deps) seen_ignore_deps=: ;;
> > +    Makefile|./Makefile) ;;
> > +    *) echo "\$0: \$1: invalid argument" >&2; exit 1;;
> > +  esac
> > +  shift
> > +done
> > +\$seen_strictnrss || {
> > +  echo "\$0: option --$strictness not seen" >&2
> > +  exit 1
> > +}
> > +$d\$seen_ignore_deps || {
> > +$d  echo "\$0: option --ignore_deps not seen" >&2
> > +$d  exit 1
> > +$d}
> > +exit 0
> > +END
> > +}
> 
> Don't you think this is a bit more complex than necessary?
>
Maybe, but I preferred to err on the side of caution.

Also, for what concerns maintainability, my "secret" hope is that, in
a not-so-distant future, we could change the semantics of the rebuild
rules, and then get rid of this testcase (or rewrite it to check the
opposite behaviour, which would make it definitely simpler).

> What is this supposed to do, and why?
>
Check that the automake instances invoked by the automake-generated
rebuild rules are passed the expected options (and only those options).

> Why is the same not achieved with a couple of greps in the usage
> part of the test below?
>
Because the rebuild rules are run "silenced" (their recipe is
prepended with a `@'), and use calls to "echo" to show what commands
they're about to perform [1].  These displayed commands might thus
not correspond to the real ones, and we want to check both then.

[1] Excerpts from lib/am/configure.am:

    %MAKEFILE-IN%: %MAINTAINER-MODE% %MAKEFILE-AM% %MAKEFILE-IN-DEPS% 
$(am__configure_deps)
    ## If configure.ac or one of configure's dependencies has changed, all
    ## Makefile.in are to be updated; it is then more efficient to run
    ## automake on all the Makefiles at once.  It also allow Automake to be
    ## run for newly added directories.
        @for dep in $?; do \
          case '$(am__configure_deps)' in \
            *$$dep*) \
    ?TOPDIR_P?     echo ' cd $(srcdir) && $(AUTOMAKE) %AUTOMAKE-OPTIONS%'; \
    ?TOPDIR_P?     $(am__cd) $(srcdir) && $(AUTOMAKE) %AUTOMAKE-OPTIONS% \
    ?TOPDIR_P?       && exit 0; \
        ... \
    ## Otherwise, rebuild only this file.
        echo ' cd $(top_srcdir) && $(AUTOMAKE) %AUTOMAKE-OPTIONS% 
%MAKEFILE-AM-SOURCES%'; \
        $(am__cd) $(top_srcdir) && \
          $(AUTOMAKE) %AUTOMAKE-OPTIONS% %MAKEFILE-AM-SOURCES%

> > +write_automake_wrapper -S foreign -I > wrap-automake-I-foreign
> > +write_automake_wrapper -S foreign    > wrap-automake-foreign
> > +write_automake_wrapper -S cygnus     > wrap-automake-cygnus
> > +write_automake_wrapper -S cygnus -I  > wrap-automake-I-cygnus
> > +write_automake_wrapper -S gnu        > wrap-automake-gnu
> > +chmod a+x wrap-automake-*
> > +
> > +# We need (almost) complete control over automake options.
> > +AUTOMAKE=`(set $AUTOMAKE && echo $1)`' -Werror' || Exit 99
> > +
> > +: > Makefile.am
> > +
> > +cwd=`pwd` || Exit 1
> > +
> > +cat > configure.in <<END
> > +AC_INIT([$me], [1.0])
> > +AM_INIT_AUTOMAKE([foreign no-dependencies])
> > +AC_CONFIG_FILES([Makefile])
> > +AC_OUTPUT
> > +END
> > +
> > +$ACLOCAL
> > +$AUTOCONF
> > +$AUTOMAKE
> > +./configure
> > +
> > +for am_opt in -Wportability --gnu --gnits --include-deps; do
> > +  $AUTOMAKE $am_opt Makefile
> > +  ./config.status
> > +  $sleep
> > +  touch Makefile.am
> > +  AUTOMAKE="$cwd/wrap-automake-I-foreign" $MAKE -e Makefile >stdout \
> > +    || { cat stdout; Exit 1; }
> > +  cat stdout
> > +  grep 'wrap-automake-I-foreign .*--foreign' stdout
> > +  grep 'wrap-automake-I-foreign .*--ignore-deps' stdout
> > +  grep ' -W' stdout && Exit 1
> > +  grep '.*--gnu' stdout && Exit 1
> > +  grep '.*--gnits' stdout && Exit 1
> > +done
> [...]
> 

Regards,
   Stefano