Re: User extensions

[Valentin David]

On Sat, Oct 23, 2010 at 4:05 PM, Stefano Lattarini
<address@hidden> wrote:
> Hi Valentin.  Thank you very much for your patch implementing this
> much-desired automake feature.  Here is a preliminary review...



>
> On Friday 03 September 2010, Valentin David wrote:
>> * 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.
> Hmm... looking at the git diffs, this seems like a useful refactoring
> by itself.  Could this be done in separate, "preparatory" patch preceding
> the implementation of the "user extensions support" proper?

Sure, I can make a series of patches like that. But there might be
also a lot of refactoring afterwards as well. For example language
dependent code should probably all move in a different file and be
imported via the extension thing.

>
>> * 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.
> We should really use the system PATH separator here, for better portability
> (such PATH separator is already computed and AC_SUBST'd by configure as
> address@hidden@').

If you say so. Is it normal to have different syntax depending from
which shell you call automake?

>> Empty paths correspond to the original libdir.
> I guess this is done so that the user can override the previously-passed
> custom libdirs with the default one, without having to know its complete
> path -- right?  I think this is useful and smart, but couldn't we find a
> better speacial value than "empty" to indicate the default libdir?

I thought I have seen it somewhere else, but I do not remember which
software. If you have any suggestion instead of empty, then shoot.

>> The test tests/user_def_lang.test should show how the feature works.
> We'll need much deeper test than the one provided by your patch (I can help
> writing them if you want).

I do agree. I did more modifications since that patch, and I
understood it was not good.

>> I can understand that loading perl files is not really nice because
>> there easily can be problems of backward compatibility.
> But it's surely better than the present situation IMHO.

It also mean that an API documentation will be required in the future.

>> +         my $r
>> +             = &{$lang->rewrite} ($directory, $base, $extension,
>>                         $nonansi_obj, $have_per_exec_flags, $var);
> What about a saner indentation here? As in:
>
>    my $r = &{$lang->rewrite} ($directory, $base, $extension,
>                               $nonansi_obj, $have_per_exec_flags,
>                               $var);
>
> [...]
>
> The function implementation below doesn't respect the GCS (GNU coding
> standards) w.r.t. use of white spaces, indentation and brackets placement.
> And while I dislike what the GCS mandate on these matters, we should
> strive to be consistent anyway.

I trusted emacs...

>> +sub libfile ($)
>> +{
>> +  my ($f) = @_;
>> +  foreach (@userlibdir) {
>> +    return "$_/$f"
>> +      if -r "$_/$f";
> Wouldn't a `-f "$_/$f"' be more correct here?

Is there a case we want to know if a file exists but fail reading it?
Should we continue to search for another file? For me both are OK. But
I can change to "-f"

>> +  }
>> +  return $userlibdir[0] . "/" . $f;
> Shouldn't this be `$libdir' instead of `$userlibdir[0]'?  Otherwise,

No. $userlibdir[0] should contain the first directory in $libdir. If
it was not like that, then there is a problem.

> Automake even fails to bootstrap itself, giving the error message:
>
>  Use of uninitialized value $userlibdir[0] in concatenation (.) or string at 
> ./automake.tmp line 7088.
>  automake.tmp: error: cannot open < /am/header-vars.am: No such file or 
> directory

I am not sure how you got this one.

> With the change I'm proposing, Automake bootstrap itself *and* passes
> the whole testsuite.

Is not it "make distcheck" enough? Because in my case it was working.

>> +     'libdir=s'      => sub { push @userlibdir,
>> +                            (map { ($_ eq '') ? "$libdir" : $_ }
>> +                                    split (':', $_[1])); },
> We should use $PATH_SEPARATOR here, not ':'.

OK

>> +  foreach (@userlibdir) {
>> +    my $dir = $_;
>> +    if (-d "$dir/lang") {
>> +      opendir(LANG_DIR, "$dir/lang") || next;
>> +      my @files=readdir(LANG_DIR);
>> +      foreach (@files) {
>> +     do "$dir/lang/$_"
>> +       if (-r "$dir/lang/$_" && $_ =~ /\.pm$/);
> Wouldn't a `-f "$dir/lang/$_"' be more correct here?

To me, that is the same.

>> +user_def_language.test \
> Typo; should be `user_def_lang.test'.

I am wondering if I sent you the right patch...

>> diff --git a/tests/user_def_lang.test b/tests/user_def_lang.test
>> new file mode 100755
>> index 0000000..9b015da
>> --- /dev/null
>> +++ b/tests/user_def_lang.test
> This test is missing the "#! /bin/sh" shebang line, the copyright header, and
> a brief comment explaining what is being tested.

>> +cat > configure.in << 'END'
>> +AC_INIT
>> +AM_INIT_AUTOMAKE(nonesuch, nonesuch)
>> +AC_PROG_CC
>> +AC_SUBST([FOO], "foo")
>> +AC_OUTPUT(Makefile)
>> +END

> Why not use the configure.in setup already provided by ./defs?

Ah ok. I did not know that.

>> +grep 'bar.c' Makefile.in
> A tiny wee bit weak test for such an important new feature, isn't it? ;-)

I agree.

-- 
Valentin David
address@hidden