Re: User extensions

[Ralf Wildenhues]

Hi Valentin,

let's move to the -patches list.  And sorry for the long delay.

* Valentin David wrote on Fri, Sep 03, 2010 at 06:56:53PM CEST:
> I propose a patch as attached.
> 
> * The lang_*_rewrite are added to the Language structure. The default
> is lang_sub_obj. They do not return anymore the object extension
> because the field 'output_extensions' already computes it.

As Stefano already suggested, this change is fine for master in a patch
by itself, and when it passes 'make check', regardless of the rest.

Please format patches in the way that is done throughout Automake
history (see HACKING), with ChangeLog entries, test suite additions,
and commit including modifications induced by ./bootstrap (given the
most recent stable Autoconf version in $PATH).  Thanks.  Generated files
need not be listed in the log.

> * Compilers that generate source files might still generate dependency files.
> 
> * --libdir= can be called several times, the arguments can also have a
> list of paths separated by a colon. Empty paths correspond to the
> original libdir.

Due to the colon/semicolon issues, how about we just ignore the issue
for now and require one --libdir argument per directory?

Also, the version issues can end up fairly intricate: we cannot really
expect automake 1.13 to successfully load the same Perl modules that
were meant for 1.12; there are *lots* of things that can go wrong, and
we just opened a can of API worms.  Still, I would really like to see
progress here, so let's for the moment not yet settle on a system-wide
(set of) directory(ies) where such additional third-party files should
end up at.  Another issue is overriding Automake's own .am files; we've
always treated them as requiring to be in sync with the automake.in
version and it could end up being pretty tricky to specify their API as
public in any way.

Also, I like the approach of calling the whole feature experimental and
at the same time asking users to tell us which features from automake.in
they need so we can document them and add testsuite coverage for them so
we can be sure to not break them in the future.

> * For all libdir, all libdir/lang/*.pm are loaded. This happens right
> after parsing the options.
> 
> The test tests/user_def_lang.test should show how the feature works.
> 
> I can understand that loading perl files is not really nice because
> there easily can be problems of backward compatibility.

Exactly.


Ideally, I would like to see testsuite coverage for each code path
("branch coverage") for new code.  I understand that only Stefano is
able to produce this in reasonable amount of time, so whatever you guys
can manage is better than nothing.  The new code should have a NEWS
entry and at least some documentation in the manual.  You can target
a new branch that is meant for eventual inclusion into master, let's
name it user-ext, and whether you base patches off of maint or of master
I don't care.  The latter might be easier for you.

Some nits below, in addition (or in contrast ;-) to Stefano's.

> --- a/automake.in
> +++ b/automake.in

> @@ -1858,17 +1871,15 @@ sub handle_single_transform ($$$$$%)
>                 }
>             }
>  
> -         # Note: computed subr call.  The language rewrite function
> -         # should return one of the LANG_* constants.  It could
> -         # also return a list whose first value is such a constant
> -         # and whose second value is a new source extension which
> -         # should be applied.  This means this particular language
> -         # generates another source file which we must then process
> -         # further.
> -         my $subr = \&{'lang_' . $lang->name . '_rewrite'};
> -         my ($r, $source_extension)
> -             = &$subr ($directory, $base, $extension,
> +         # The language rewrite function should return one of the
> +            # LANG_* constants.
> +         my $r
> +             = &{$lang->rewrite} ($directory, $base, $extension,
>                         $nonansi_obj, $have_per_exec_flags, $var);
> +
> +         my $output_extension
> +             = (&{$lang->output_extensions} ($extension))[0];
> +
>           # Skip this entry if we were asked not to process it.
>           next if $r == LANG_IGNORE;
>  
> @@ -1876,9 +1887,9 @@ sub handle_single_transform ($$$$$%)
>           $linker = $lang->linker;
>  
>           my $this_obj_ext;
> -         if (defined $source_extension)
> +         if ($output_extension ne '.$(OBJEXT)')

Why this?  This looks wrong, but I cannot judge because I don't
understand why your patch would need this.

>           {
> -             $this_obj_ext = $source_extension;
> +             $this_obj_ext = $output_extension;
>               $derived_source = 1;
>           }
>           elsif ($lang->ansi)
> @@ -2056,10 +2067,19 @@ sub handle_single_transform ($$$$$%)
>           $lang->target_hook ($aggregate, $object, $full, %transform);
>       }
>  
> +     # Transform .o or $o file into .P file (for automatic
> +     # dependency code).
> +     if ($lang && $lang->autodep ne 'no')
> +     {
> +         my $depfile = $object;
> +         $depfile =~ s/\.([^.]*)$/.P$1/;
> +         $depfile =~ s/\$\(OBJEXT\)$/o/;
> +         $dep_files{dirname ($depfile) . '/$(DEPDIR)/'
> +                      . basename ($depfile)} = 1;
> +     }
> +
>       if ($derived_source)
>         {
> -         prog_error ($lang->name . " has automatic dependency tracking")
> -           if $lang->autodep ne 'no';

This removal (without replacement) strikes me as weird.  What did you
need it for?  We should have a testsuite addition that exposes the
error, and see that a fix of yours keeps the old failure working.

>           # Make sure this new source file is handled next.  That will
>           # make it appear to be at the right place in the list.
>           unshift (@files, $object);
> @@ -2120,16 +2140,6 @@ sub handle_single_transform ($$$$$%)
>               if scalar @dep_list > 0;
>       }
>  
> -     # Transform .o or $o file into .P file (for automatic
> -     # dependency code).
> -     if ($lang && $lang->autodep ne 'no')
> -     {
> -         my $depfile = $object;
> -         $depfile =~ s/\.([^.]*)$/.P$1/;
> -         $depfile =~ s/\$\(OBJEXT\)$/o/;
> -         $dep_files{dirname ($depfile) . '/$(DEPDIR)/'
> -                      . basename ($depfile)} = 1;
> -     }
>      }
>  
>      return @result;
> @@ -2555,7 +2565,7 @@ sub handle_compile ()
>      }
>  
>      my ($coms, $vars, $rules) =
> -      &file_contents_internal (1, "$libdir/am/compile.am",
> +      &file_contents_internal (1, libfile ("am/compile.am"),
>                              new Automake::Location,
>                              ('DEFAULT_INCLUDES' => $default_includes,
>                               'MOSTLYRMS' => join ("\n", @mostly_rms),
> @@ -5726,16 +5736,9 @@ sub check_gnits_standards
>  #
>  # Functions to handle files of each language.
>  
> -# Each `lang_X_rewrite($DIRECTORY, $BASE, $EXT)' function follows a
> -# simple formula: Return value is LANG_SUBDIR if the resulting object
> -# file should be in a subdir if the source file is, LANG_PROCESS if
> -# file is to be dealt with, LANG_IGNORE otherwise.
> -
>  # Much of the actual processing is handled in
>  # handle_single_transform.  These functions exist so that
>  # auxiliary information can be recorded for a later cleanup pass.
> -# Note that the calls to these functions are computed, so don't bother
> -# searching for their precise names in the source.
>  
>  # This is just a convenience function that can be used to determine
>  # when a subdir object should be used.


> @@ -7187,7 +7070,20 @@ sub make_paragraphs ($%)
>    return @res;
>  }
>  
> -
> +# libfile ($FILE)
> +# ---------------
> +# Look for $FILE in the library directories. Returns the first path
> +# found. If not found, returns a path from the library directory with

Two spaces after period, please.

> +# highest priority.
> +sub libfile ($)
> +{
> +  my ($f) = @_;
> +  foreach (@userlibdir) {
> +    return "$_/$f"
> +      if -r "$_/$f";

-f is ok here.  Let access fail with a read error if unreadable, the
user is entitled to learn about broken setups.

> +  }
> +  return $userlibdir[0] . "/" . $f;
> +}
>  
>  # ($COMMENT, $VARIABLES, $RULES)
>  # &file_contents_internal ($IS_AM, $FILE, $WHERE, [%TRANSFORM])
> @@ -7246,7 +7142,7 @@ sub file_contents_internal ($$$%)
>       {
>           if ($cond != FALSE)
>             {
> -             my $file = ($is_am ? "$libdir/am/" : '') . $1;
> +             my $file = ($is_am ? libfile ("am/".$1) : $1);
>               $where->push_context ("`$file' included from here");
>               # N-ary `.=' fails.
>               my ($com, $vars, $rules)
> @@ -7391,7 +7287,7 @@ sub file_contents ($$%)
>  {
>      my ($basename, $where, %transform) = @_;
>      my ($comments, $variables, $rules) =
> -      file_contents_internal (1, "$libdir/am/$basename.am", $where,
> +      file_contents_internal (1, libfile ("am/$basename.am"), $where,
>                             %transform);
>      return "$comments$variables$rules";
>  }
> @@ -7864,7 +7760,7 @@ sub require_file_internal ($$$@)
>             my $message = "required file `$fullfile' not found";
>             if ($add_missing)
>               {
> -               if (-f "$libdir/$file")
> +               if (-f libfile ("$file") )

You may want to cache the file name you found here, for cases where you
look up the same again below.  Lookups are expensive, I'm guessing that
this change alone with several --libdir args can slow down automake
noticeably.

>                   {
>                     $suppress = 1;
>  
> @@ -7888,14 +7784,14 @@ sub require_file_internal ($$$@)
>                     unlink ($fullfile) if -f $fullfile;
>                     if ($symlink_exists && ! $copy_missing)
>                       {
> -                       if (! symlink ("$libdir/$file", $fullfile)
> +                       if (! symlink (libfile ("$file"), $fullfile)
>                             || ! -e $fullfile)
>                           {
>                             $suppress = 0;
>                             $trailer = "; error while making link: $!";
>                           }
>                       }
> -                   elsif (system ('cp', "$libdir/$file", $fullfile))
> +                   elsif (system ('cp', libfile ("$file"), $fullfile))
>                       {
>                         $suppress = 0;
>                         $trailer = "\n    error while copying";
> @@ -7923,7 +7819,7 @@ sub require_file_internal ($$$@)
>             else
>               {
>                 $trailer = "\n  `automake --add-missing' can install `$file'"
> -                 if -f "$libdir/$file";
> +                 if -f libfile ("$file");
>               }
>  
>             # If --force-missing was specified, and we have
> @@ -8458,7 +8354,9 @@ sub parse_arguments ()
>    my $cli_where = new Automake::Location;
>    my %cli_options =
>      (
> -     'libdir=s'      => \$libdir,
> +     'libdir=s'      => sub { push @userlibdir,
> +                            (map { ($_ eq '') ? "$libdir" : $_ }
> +                                    split (':', $_[1])); },

See above, let's omit the colon feature for now.

> @@ -8724,6 +8622,25 @@ sub handle_makefiles_threaded ($)
>      }
>  }
>  
> +# load_languages ()
> +# -----------------
> +# Load Perl files for each .pm files in all libdir/lang.
> +sub load_languages
> +{
> +  foreach (@userlibdir) {
> +    my $dir = $_;
> +    if (-d "$dir/lang") {
> +      opendir(LANG_DIR, "$dir/lang") || next;

The user is entitled to learn about read failures, including error
message.  We have a bit wrapping code in lib/Automake/FileUtils.pm
not sure if anything is useful here.

> +      my @files=readdir(LANG_DIR);
> +      foreach (@files) {
> +     do "$dir/lang/$_"

Verbose output here that we're about to source some file?

> +       if (-r "$dir/lang/$_" && $_ =~ /\.pm$/);

The parens are unnecessary here, they kind of disturb reading of postfix
if.

> +      }
> +      closedir(LANG_DIR);
> +    }
> +  }
> +}
> +
>  ################################################################
>  
>  # Parse the WARNINGS environment variable.
> @@ -8732,6 +8649,8 @@ parse_WARNINGS;
>  # Parse command line.
>  parse_arguments;
>  
> +load_languages;
> +
>  $configure_ac = require_configure_ac;
>  
>  # Do configure.ac scan only once.

> --- /dev/null
> +++ b/tests/user_def_lang.test

> +cat > lib/am/foo.am <<'END'
> +?GENERIC?%EXT%%DERIVED-EXT%:
> +?!GENERIC?%OBJ%: %SOURCE%
> +?GENERIC?    %VERBOSE%%COMPILE% <%SOURCE% >%OBJ%
> +?GENERIC?    %SILENT%echo %OBJ%: somefile >%DEPBASE%.Po
> +?!GENERIC?   %VERBOSE%%COMPILE% <`test -f '%SOURCE%' || echo 
> '$(srcdir)/'`%SOURCE% >%OBJ%
> +?!GENERIC??SUBDIROBJ?        %SILENT%depbase=`echo %OBJ% | sed 
> 's|[^/]*$$|$(DEPDIR)/&|;s|\.o$$||'`;\
> +?!GENERIC??SUBDIROBJ?        echo %OBJ%: somefile >%DEPBASE%.Po
> +?!GENERIC??!SUBDIROBJ?       %SILENT%echo %OBJ%: somefile >%DEPBASE%.Po

Just these %transform elements mean quite a bit of API exposure already,
API that I'd ideally like to be able to change at will in the future, if
it means that we can generate better makefile code ...

> +END
> +
> +mkdir -p lib/lang
> +cat > lib/lang/foo.pm <<'END'
> +register_language (# Short name of the language (c, f77...).
> +                'name' => "foo",
> +                # Nice name of the language (C, Fortran 77...).
> +                'Name' => "Foo",
> +                # List of configure variables which must be defined.
> +                'config_vars' => ['FOO'],
> +                'autodep' => 'FOO',
> +
> +                # Name of the compiling variable (COMPILE).
> +                'compiler'  => "FOOCOMPILE",
> +                # Content of the compiling variable.
> +                'compile'  => '$(FOO) $(FOOFLAGS)',
> +                # Flag to require compilation without linking (-c).
> +                'extensions' => ['.foo'],
> +                # A subroutine to compute a list of possible extensions of
> +                # the product given the input extensions.
> +                # (defaults to a subroutine which returns ('.$(OBJEXT)', 
> '.lo'))
> +                'output_extensions' => sub { return ('.c',); },
> +                # A list of flag variables used in 'compile'.
> +                # (defaults to [])
> +                'flags' => ["FOOFLAGS"],
> +
> +                # The file to use when generating rules for this language.
> +                # The default is 'depend2'.
> +                'rule_file' => "foo",
> +
> +                # Name of the compiler variable (CC).
> +                'ccer' => "FOO",
> +
> +                '_finish' => sub {},
> +
> +                # This is a subroutine which is called whenever we finally
> +                # determine the context in which a source file will be
> +                # compiled.
> +
> +                '_target_hook' => sub {
> +                  my ($self, $aggregate, $output, $input, %transform) = @_;
> +                  $clean_files{$output} = MAINTAINER_CLEAN;
> +                },
> +
> +                # If TRUE, nodist_ sources will be compiled using specific 
> rules
> +                # (i.e. not inference rules).  The default is FALSE.
> +                'nodist_specific' => 1);

All of these struct elements would need to be documented eventually, I
guess.  Pretty ugly, with things like _target_hook and the like...

Cheers, and thanks,
Ralf