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

[Ralf Wildenhues]

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.  I also don't
think that moving from module-local variables and concepts (options vs.
global_options) toward global variables like am_remake_options would be
an improvement.  On the contrary, so far I've thought that the
distinction and current API would be a good one, no?  What makes it
unsuitable?

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.

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: 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.

Thanks,
Ralf

>  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.

> +     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}.  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.

>    $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.

> +# 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 ...

> +# 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.

It seems like this test would be testing functionality that is dubious
at best and wrong at worst.  Why are we doing this?

> +
> +. ./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?
What is this supposed to do, and why?  Why is the same not
achieved with a couple of greps in the usage part of the
test below?

> +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
[...]