Hello Mathias, all, let me please say that I'm a bit ashamed that I put this off for so long. Please accept my apologies. I promise to push this through quickly now.
But first of all, thanks for your contributions to Automake! So here we go with a review of <http://thread.gmane.org/gmane.comp.sysutils.automake.patches/2860/focus=513>. FWIW, I volunteer to fix the issues, if only to make up for my guilty conscience, but see below for help I need and questions I have. Overall, the patches are pretty good. * Mathias Hasselmann wrote on Sat, Sep 29, 2007 at 06:18:56PM CEST: > After talking with Jürg today I've updated my patches for Vala support > in automake: > > - forgot to tell automake to distribute some of the files added > - non-recursive builds should work now > - automake automatically injects the --library switch for Vala > libraries now. By default the library's base name is used. Its > possible to override it with _PKGNAME variables I'm not overly fond of adding the new PKGNAME prefix, but I guess this is mostly due to the fact that I don't quite understand the value of it yet. The user can just add --library= to _LDFLAGS no? Does vala require to use libtool? If no, then a test without libtool would be good. If yes, maybe AM_PROG_VALA should AC_REQUIRE([AC_PROG_LIBTOOL]). FWIW, it would be logical to merge (squash) the first two patches, and move the change s/AC_PROG_VALA/AM_PROG_VALA/ from the third to the first one, too. More comments to the patches inline. The patches seem to be missing the lib/am/vala.am file. Can you please post it? (My review of the code you posted is a bit incomplete due to the missing file; I will finish that then.) Cheers, and thanks again, Ralf * Mathias Hasselmann wrote on Sat, Sep 29, 2007 at 06:18:56PM CEST: > > Index: automake.in > =================================================================== > RCS file: /cvs/automake/automake/automake.in,v > retrieving revision 1.1649 > diff -u -r1.1649 automake.in > --- automake.in 30 Sep 2007 04:18:20 -0000 1.1649 > +++ automake.in 30 Sep 2007 05:01:27 -0000 > @@ -113,7 +113,7 @@ > my ($self) = @_; > if (defined $self->_finish) > { > - &{$self->_finish} (); > + &{$self->_finish} (@_); > } > } > > @@ -545,6 +545,7 @@ > # Keep track of all programs declared in this Makefile, without > # $(EXEEXT). @substitution@ are not listed. > my %known_programs; > +my %known_libraries; > > # Keys in this hash are the basenames of files which must depend on > # ansi2knr. Values are either the empty string, or the directory in > @@ -666,6 +667,7 @@ > @dist_targets = (); > > %known_programs = (); > + %known_libraries= (); > > %de_ansi_files = (); > > @@ -776,6 +778,22 @@ > # Nothing to do. > '_finish' => sub { }); > > +# Vala > +register_language ('name' => 'vala', > + 'Name' => 'Vala', > + 'config_vars' => ['VALAC'], > + 'flags' => ['VALAFLAGS'], > + 'compile' => '$(VALAC) $(VALAFLAGS) $(AM_VALAFLAGS)', AM_VALAFLAGS should not come after VALAFLAGS: the user should be able to override the package author. Unless it's the first and not the last flags that win an override. > + 'compiler' => 'VALACOMPILE', > + 'extensions' => ['.vala'], > + 'output_extensions' => sub { (my $ext1 = $_[0]) =~ > s/vala$/c/; > + (my $ext2 = $_[0]) =~ > s/vala$/h/; > + return ($ext1, $ext2) }, > + 'rule_file' => 'vala', > + '_finish' => \&lang_vala_finish, > + '_target_hook' => \&lang_vala_target_hook, > + 'nodist_specific' => 1); > + > # Yacc (C & C++). > register_language ('name' => 'yacc', > 'Name' => 'Yacc', > @@ -2548,6 +2566,8 @@ > . "did you mean `$suggestion'?") > } > > + ($known_libraries{$onelib} = $bn) =~ s/\.a$//; > + > $where->push_context ("while processing library `$onelib'"); > $where->set (INTERNAL->get); > > @@ -2739,6 +2759,8 @@ > . "did you mean `$suggestion'?") > } > > + ($known_libraries{$onelib} = $bn) =~ s/\.la$//; > + > $where->push_context ("while processing Libtool library `$onelib'"); > $where->set (INTERNAL->get); > > @@ -5309,6 +5331,15 @@ > return LANG_IGNORE; > } > > +# Rewrite a single Vala source file. > +sub lang_vala_rewrite > +{ > + my ($directory, $base, $ext) = @_; > + > + (my $newext = $ext) =~ s/vala$/c/; > + return (LANG_SUBDIR, $newext); > +} > + > # Rewrite a single yacc file. > sub lang_yacc_rewrite > { > @@ -5467,6 +5498,84 @@ > } > } > > +sub lang_vala_finish_target ($$$) > +{ > + my ($self, $name, $pkgname) = @_; > + > + my $derived = canonicalize ($name); > + my $varname = $derived . '_SOURCES'; > + my $var = var ($varname); > + > + if ($var) > + { > + foreach my $file ($var->value_as_list_recursive) > + { > + $output_rules .= "$file: ${derived}_vala.stamp\n" > + if ($file =~ s/(.*)\.vala$/$1.c $1.h/); > + } > + } > + > + my $compile = $self->compile; > + > + if (defined ($pkgname)) > + { > + $varname = $derived . '_PKGNAME'; > + $var = var ($varname); > + > + $pkgname = $var->variable_value if $var; > + $compile =~s/\$\(AM_VALAFLAGS\)/--library=$pkgname $&/; > + } > + > + # Rewrite each occurrence of `AM_$flag' in the compile > + # rule into `${derived}_$flag' if it exists. > + for my $flag (@{$self->flags}) > + { > + my $val = "${derived}_$flag"; > + $compile =~ s/\(AM_$flag\)/\($val\)/ > + if set_seen ($val); > + } > + > + my $dirname = dirname ($name); > + > + $compile .= " -d $dirname" if $dirname ne '.'; > + > + $output_rules .= > + "${derived}_vala.stamp: \$(${derived}_SOURCES)\n". > + "\t${compile} \$^ && touch [EMAIL PROTECTED]"; > +} > + > +# This is a vala helper which is called after all source file > +# processing is done. > +sub lang_vala_finish > +{ > + my ($self) = @_; > + > + foreach my $prog (keys %known_programs) > + { > + lang_vala_finish_target ($self, $prog, 0); > + } > + > + while (my ($name, $pkgname) = each %known_libraries) > + { > + lang_vala_finish_target ($self, $name, $pkgname); > + } > +} > + > +# This is a vala helper which is called whenever we have decided to > +# compile a vala file. > +sub lang_vala_target_hook > +{ > + my ($self, $aggregate, $output, $input, %transform) = @_; > + > + (my $output_base = $output) =~ s/$KNOWN_EXTENSIONS_PATTERN$//; > + my $header = $output_base . '.h'; > + > + &push_dist_common ($header); > + > + $clean_files{$header} = MAINTAINER_CLEAN; > + $clean_files{$output} = MAINTAINER_CLEAN; > +} > + > # This is a yacc helper which is called whenever we have decided to > # compile a yacc file. > sub lang_yacc_target_hook > Index: doc/automake.texi > =================================================================== > RCS file: /cvs/automake/automake/doc/automake.texi,v > retrieving revision 1.171 > diff -u -r1.171 automake.texi > --- doc/automake.texi 19 Aug 2007 07:46:52 -0000 1.171 > +++ doc/automake.texi 30 Sep 2007 05:01:33 -0000 > @@ -6397,6 +6399,60 @@ > the @code{_LDFLAGS} variable for the program. > > > [EMAIL PROTECTED] Vala Support > [EMAIL PROTECTED] node-name, next, previous, up > [EMAIL PROTECTED] Vala Support > + > [EMAIL PROTECTED] Vala Support > [EMAIL PROTECTED] Support for Vala > + > +Automake provides support for Vala compilation. Maybe this could use an URL? I've never heard of Vala before. > + > [EMAIL PROTECTED] > +foo_SOURCES = foo.vala bar.vala zardoc.c > [EMAIL PROTECTED] example > + > +Any .vala file listed in a @code{_SOURCE} variable will be compiled @file{.vala} s/ $// > +into C code by the Vala compiler. > + > +Automake ships with an Autoconf macro called @code{AM_PROG_VALAC} > +that will locate the Vala compiler and optionally check its version > +number. > + > [EMAIL PROTECTED] AM_PROG_VALAC ([EMAIL PROTECTED]) This can use @ovar. > + > +Check whether the Vala compiler exists in `PATH'. If it is found the s/`PATH'/@env{PATH}/ > +variable VALAC is set. Optionally a minimum release number of the compiler > +can be requested. > + > [EMAIL PROTECTED] > +AM_PROG_VALAC([0.1.3]) > [EMAIL PROTECTED] example > + > [EMAIL PROTECTED] defmac > + > +There are a few variables that are used when compiling Vala sources: > + > [EMAIL PROTECTED] @code > + > [EMAIL PROTECTED] VALAC > +Path to the the Vala compiler. > + > [EMAIL PROTECTED] VALAFLAGS > +Additional arguments for the Vala compiler. > + > [EMAIL PROTECTED] PKGNAME > +The pkg-config and VAPI name to use when building Vala based library. pkg-config is not defined (and not GNU). We prefer to not have to rely on non-GNU tools, but if we really have to, we should explain what it is and link to it. > + > [EMAIL PROTECTED] > +lib_LTLIBRARIES = libfoo.la > +libfoo_la_PKGNAME = foo-2.0 > +libfoo_la_SOURCES = foo.vala > [EMAIL PROTECTED] example > + > [EMAIL PROTECTED] vtable > + > + > @node Support for Other Languages > @comment node-name, next, previous, up > @section Support for Other Languages > Index: lib/am/Makefile.am > =================================================================== > RCS file: /cvs/automake/automake/lib/am/Makefile.am,v > retrieving revision 1.175 > diff -u -r1.175 Makefile.am > --- lib/am/Makefile.am 7 Jul 2007 11:23:28 -0000 1.175 > +++ lib/am/Makefile.am 30 Sep 2007 05:01:33 -0000 > @@ -61,4 +61,5 @@ > texi-vers.am \ > texibuild.am \ > texinfos.am \ > +vala.am \ > yacc.am Why do you need this empty file? > Index: tests/Makefile.am > =================================================================== > RCS file: /cvs/automake/automake/tests/Makefile.am,v > retrieving revision 1.623 > diff -u -r1.623 Makefile.am > --- tests/Makefile.am 16 Aug 2007 23:47:09 -0000 1.623 > +++ tests/Makefile.am 30 Sep 2007 05:01:33 -0000 > @@ -589,6 +589,10 @@ > upc.test \ > upc2.test \ > upc3.test \ > +vala.test \ > +vala1.test \ A minor nit: it's practice in Automake to name tests foo, foo2, ... > +vala2.test \ > +vala3.test \ > vars.test \ > vars3.test \ > vartar.test \ > --- /dev/null 2007-09-29 14:18:42.220064750 +0200 > +++ m4/vala.m4 2007-09-30 06:48:21.000000000 +0200 > @@ -0,0 +1,36 @@ > +# Autoconf support for the Vala compiler > + > +# Copyright (C) 2007 Free Software Foundation, Inc. > +# > +# This file is free software; the Free Software Foundation > +# gives unlimited permission to copy and/or distribute it, > +# with or without modifications, as long as this notice is preserved. > + > +# serial 2 > + > +# Check whether the Vala compiler exists in `PATH'. If it is found the > +# variable VALAC is set. Optionally a minimum release number of the compiler > +# can be requested. > +# > +# Author: Mathias Hasselmann <[EMAIL PROTECTED]> It's not usual in autotools to add author information to macro files, esp. not outside `##' comments (and these are only done in some cases of rather old macros); at least the latter won't be copied to user's package's aclocal.m4 files. Rather, attribution is done by way of AUTHORS, THANKS, and the ChangeLog entry. I hope you don't have a problem with this convention. > +# > +# AM_PROG_VALAC([MINIMUM-VERSION]) > +# -------------------------------------------------------------------------- > +AC_DEFUN([AM_PROG_VALAC],[ > + AC_PATH_PROG([VALAC], [valac], []) > + AC_SUBST(VALAC) AC_PATH_PROG already calls AC_SUBST, so you don't need to do it. > + > + if test -z "${VALAC}"; then > + AC_MSG_WARN([No Vala compiler found. You will not be able to recompile > .vala source files.]) > + elif test -n "$1"; then > + AC_REQUIRE([AC_PROG_AWK]) AC_REQUIRE's are expanded before the macro text, so for clarity it would be best to move this to the beginning of the script, too. But see below for we may be able to drop line this completely. > + AC_MSG_CHECKING([valac is at least version $1]) > + > + if "${VALAC}" --version | "${AWK}" -v r='$1' 'function vn(s) { if (3 == > split(s,v,".")) return (v[1]*1000+v[2])*1000+v[3]; else exit 2; } /^Vala / { > exit vn(r) > vn($[2]) }'; then This won't work with Solaris awk, as it's ancient and does not have function support. Also, it looks to me that the array subscripts [..] are m4-underquoted here. Can this use AS_VERSION_COMPARE instead? It's a currently undefined Autoconf macro, but present in the Autoconf versions that this Automake requires, and we can push Autoconf to make it defined. > + AC_MSG_RESULT([yes]) > + else > + AC_MSG_RESULT([no]) > + AC_MSG_ERROR([Vala $1 not found.]) Would it be useful to have an optional IF-FAILS argument that allows the configure.ac author to not let the script error if some version is not available? > + fi > + fi > +]) > --- /dev/null 2007-09-29 14:18:42.220064750 +0200 > +++ tests/vala.test 2007-09-30 06:48:21.000000000 +0200 > @@ -0,0 +1,61 @@ > +#! /bin/sh > +# Copyright (C) 1996, 2001, 2002, 2006 Free Software Foundation, Inc. If this file is completely new, it should list 2007 only. If you derived it from some other test, then please add 2007. Likewise for the other tests. > +# Test to make sure intermediate .c files are built from vala source. > + > +required="libtool" > +. ./defs || exit 1 The tests need updating to use Exit instead of exit for git Automake. Likewise for the other tests. > +set -e > + > +cat >> 'configure.in' << 'END' > +AC_PROG_CC > +AC_PROG_LIBTOOL > +AM_PROG_VALAC > +AC_OUTPUT > +END > + > +cat > 'Makefile.am' <<'END' > +bin_PROGRAMS = zardoz > +zardoz_SOURCES = zardoz.vala > +zardoz_VALAFLAGS = --debug > + > +lib_LTLIBRARIES = libzardoz.la > +libzardoz_la_SOURCES = zardoz-foo.vala zardoz-bar.vala > +END > + > +: > ltmain.sh > +: > config.sub > +: > config.guess > + > +$ACLOCAL > +$AUTOMAKE -a > + > +grep -w -- 'VALAC' 'Makefile.in' `grep -w' is not portable. The `--' is not needed here and in most other cases, and I'm not sure it's portable; at least it's avoided in all of the testsuite by prepending either a space or `.*' to the pattern. Likewise for the other tests. Not that it hurts, but most of the single quoting is not needed either. > +grep -w -- 'am_zardoz_OBJECTS' 'Makefile.in' > +grep -w -- 'am_libzardoz_la_OBJECTS' 'Makefile.in' > +grep -w -- 'zardoz_vala.stamp' 'Makefile.in' > +grep -w -- 'libzardoz_la_vala.stamp' 'Makefile.in' > +grep -w -- '--library=libzardoz' 'Makefile.in' > +grep -w -- 'zardoz\.c' 'Makefile.in' > +grep -w -- 'zardoz\.h' 'Makefile.in' > +grep -w -- 'zardoz-foo\.c' 'Makefile.in' > +grep -w -- 'zardoz-foo\.h' 'Makefile.in' I'm not quite sure what all these greps are to accomplish, but for example am_zardoz_OBJECTS, am_libzardoz_la_OBJECTS, zardoz_vala.stamp and libzardoz_la_vala.stamp are (should be!) internal details, no? Can we test for something equivalent without using them? (If we can't, then I guess it's better to leave these in than to not have them, but in general execution tests are better.) > --- /dev/null 2007-09-29 14:18:42.220064750 +0200 > +++ tests/vala1.test 2007-09-30 06:48:21.000000000 +0200 > +# Test to make sure intermediate .c files are built from vala sources > +# in non-recursive automake mode. > + > +required="libtool" > +. ./defs || exit 1 > + > +set -e > + > +cat >> 'configure.in' << 'END' > +AC_PROG_CC > +AC_PROG_LIBTOOL > +AM_PROG_VALAC > +AC_OUTPUT > +END > + > +cat > 'Makefile.am' <<'END' > +bin_PROGRAMS = src/zardoz > +src_zardoz_SOURCES = src/zardoz.vala > + > +lib_LTLIBRARIES = src/libzardoz.la > +src_libzardoz_la_SOURCES = src/zardoz-foo.vala src/zardoz-bar.vala > +END > --- /dev/null 2007-09-29 14:18:42.220064750 +0200 > +++ tests/vala2.test 2007-09-30 06:48:21.000000000 +0200 > +# Test to make foo_PKGNAME variables are considered. > + > +required="libtool" > +. ./defs || exit 1 > + > +set -e > + > +cat >> 'configure.in' << 'END' > +AC_PROG_CC > +AC_PROG_LIBTOOL > +AM_PROG_VALAC > +AC_OUTPUT > +END > + > +cat > 'Makefile.am' <<'END' > +lib_LTLIBRARIES = src/libzardoz.la > +src_libzardoz_la_SOURCES = src/zardoz-foo.vala src/zardoz-bar.vala > +src_libzardoz_la_PKGNAME = zardoz+-3.0 > +END > + > +: > ltmain.sh > +: > config.sub > +: > config.guess > + > +$ACLOCAL > +$AUTOMAKE -a > + > +grep -w -- 'VALAC' 'Makefile.in' > +grep -w -- 'src_libzardoz_la_OBJECTS' 'Makefile.in' > +grep -w -- 'src_libzardoz_la_vala.stamp' 'Makefile.in' > +grep -w -- '--library=zardoz+-3.0' 'Makefile.in' > +grep -w -- 'src/zardoz-foo\.c' 'Makefile.in' > +grep -w -- 'src/zardoz-foo\.h' 'Makefile.in' > + > --- /dev/null 2007-09-29 14:18:42.220064750 +0200 > +++ tests/vala3.test 2007-09-30 06:48:21.000000000 +0200 [...] > + > +# Test to make sure compiling Vala code really works. > + > +required="libtool libtoolize pkg-config valac gcc" Is all this stuff (plus the PKG_CHECK_MODULES macro and gobject) all necessary for a minimal working test? [...] > +cat >> 'configure.in' << 'END' > +AC_PROG_CC > +AM_PROG_CC_C_O > +AC_PROG_LIBTOOL > + > +AM_PROG_VALAC > + > +PKG_CHECK_MODULES(GOBJECT,gobject-2.0 >= 2.10) > + > +AC_OUTPUT > +END > + > +cat > 'Makefile.am' <<'END' > +bin_PROGRAMS = src/zardoz > +src_zardoz_CFLAGS = $(GOBJECT_CFLAGS) > +src_zardoz_LDADD = $(GOBJECT_LIBS) > +src_zardoz_SOURCES = src/zardoz.vala > +END > + > +cat > 'src/zardoz.vala' <<'END' > +using GLib; > + > +public class Zardoz { > + public static void main () { > + stdout.printf ("Zardoz!\n"); > + } > +} > +END > + > +libtoolize > + > +$ACLOCAL > +$AUTOCONF > +$AUTOMAKE -a > + > +./configure > +make This needs to use $MAKE (maintainer-check warns about this). > +
