On Sat, Oct 23, 2010 at 4:05 PM, Stefano Lattarini <stefano.lattar...@gmail.com> 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 > `...@path_separator@'). 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 valentin.da...@gmail.com