Re: Updated Vala support for automake

[Ralf Wildenhues]

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 address@hidden";
> +}
> +
> +# 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.
>  
>  
> address@hidden Vala Support
> address@hidden  node-name,  next,  previous,  up
> address@hidden Vala Support
> +
> address@hidden Vala Support
> address@hidden Support for Vala
> +
> +Automake provides support for Vala compilation.

Maybe this could use an URL?  I've never heard of Vala before.

> +
> address@hidden
> +foo_SOURCES = foo.vala bar.vala zardoc.c
> address@hidden 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.
> +
> address@hidden AM_PROG_VALAC (address@hidden)

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.
> +
> address@hidden
> +AM_PROG_VALAC([0.1.3])
> address@hidden example
> +
> address@hidden defmac
> +
> +There are a few variables that are used when compiling Vala sources:
> +
> address@hidden @code
> +
> address@hidden VALAC
> +Path to the the Vala compiler.
> +
> address@hidden VALAFLAGS
> +Additional arguments for the Vala compiler.
> +
> address@hidden 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.

> +
> address@hidden
> +lib_LTLIBRARIES = libfoo.la
> +libfoo_la_PKGNAME = foo-2.0
> +libfoo_la_SOURCES = foo.vala
> address@hidden example
> +
> address@hidden 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 <address@hidden>

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).

> +