Re: User extensions

[Stefano Lattarini]

On Wednesday 27 October 2010, Valentin David wrote:
> 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.
Of course; I was just pointing out that the "preliminary" refactoring your
patch requires is IMHO useful also by itself (since it makes the code
simpler and somewhat "better incapsulated"), and not just in sight of the
implementation of your "User extensions" feature.  That's why I'd like to
see it done in a separate patch.  Not a big deal, though.
> For example language
> dependent code should probably all move in a different file and be
> imported via the extension thing.
I heartily agree.

> >> * 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?
It's not dependent on the shell, but on the system.  This is an unfortunate
necessity due (I think) to the fact that something like `C:\foo\bar' is a
perfectly valid *and natural* absolute path on windows, so that `:' cannot
be portably used to separate different entries in a list of file or
direrctory paths.

> >> 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 say there are other significant software packages doing something
similar to your proposed implementation, then I'd like to stick to it, as
I prefer to be consistent with pre-existing policies whenever possible.
Also, in our case, being consistent with the `PATH' environment variable
("an empty entry means the current directory") is not a priority IMHO (and
I personally find that `PATH' policy suboptimal to say the least).

> If you have any suggestion instead of empty, then shoot.
In fact I don't.  So let's just keep it as it is (my "objection" was
bike-shedding anyway, and not worth losing other time upon).
 
> >> 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'm glad you agree on this.

> >> 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.
For the moment we could just declare your feauture as "experimental and
subject to changes", until things settle down.  Anyway, using an
experimental but *documented and supported* feature is IMO way better
then having to maintain a patch against automake.
 
> >> +         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...
I don't use emacs ;-)
 
> >> +sub libfile ($)
> >> +{
> >> +  my ($f) = @_;
> >> +  foreach (@userlibdir) {
> >> +    return "$_/$f"
> >> +      if -r "$_/$f";
> > Wouldn't a `-f "$_/$f"' be more correct here?
> 
I mean, `-r FILE' succeeds even if FILE is indeed a (readable) directory,
but we are looking for regular files.
> Is there a case we want to know if a file exists but fail reading it?
> Should we continue to search for another file?
Not sure about these ...
> For me both are OK. But I can change to "-f"
Or add *also* "-f FILE" to the test, e.g.:

 + return "$_/$f"
 +   if -f "$_/$f" && -r "$_/$f";

WDYT?

> >> +  }
> >> +  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?
Yes and no, because (due to a weakness in the automake testsuite) at
least all the tests requiring libtool and libtoolize get skipped by
"make distcheck".  Anyway, an error in your implementation would almost
surely cause failures in many other tests, so there's something else
going on here.
> Because in my case it was working.
Weird; as I said, I couldn't even bootstrap ...

> >> +     '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.
See above.

> 
> >> +user_def_language.test \
> > Typo; should be `user_def_lang.test'.
> 
> I am wondering if I sent you the right patch...
Well, I'd be glad to get an updated patch whenever you like ...
especially if based off master and created by "git format-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.
You might find the `tests/README' file useful then.

> >> +grep 'bar.c' Makefile.in
> > A tiny wee bit weak test for such an important new feature, isn't it? ;-)
> 
> I agree.
Once the overall design and basic documentation of the new feature are
settled, I'll be happy to help writing some test cases.

Thanks,
   Stefano