Re: [Vala] Vala support for automake

[Ralf Wildenhues <Ralf Wildenhues gmx de>]

Hello Jürg,

very nice.  Thanks again for working on this.  At a first glance, all I
could find while reading the patch was a trivial s/_SOURCE/_SOURCES/ in
the manual.

Other than that, I haven't yet gotten around to get my system up to date
in order to support Vala 0.7 yet.  Would you be so nice as to post
verbose log output for all the vala tests?  That is, just
  cd tests
  for t in path/to/source/tree/tests/*vala*.test; do
    $t
  done 2>&1 | tee log

and post "log", preferably gzip'ed.  That will allow me to finish review
before finishing my system.

Some more comments inline.

* Jürg Billeter wrote on Sun, Apr 05, 2009 at 03:45:44PM CEST:
> On Tue, 2009-03-31 at 23:15 +0200, Ralf Wildenhues wrote:
> > * Jürg Billeter wrote on Tue, Mar 31, 2009 at 01:11:55PM CEST:

> Thanks for your prompt response. I finally found some time today to
> address your comments. I've rebased mh-vala-support branch and my patch
> against the 'next' branch and attached the full patch series. I've also
> made the necessary changes to get silent-rules mode working with valac.

Cool.

> > BTW, where can we read about the current semantics of valac?
>
> There is no formal documentation at the moment, the changes performed
> recently are described in bug 572536 [1].

OK; thanks.  I don't see any apparent issues with that.

> > More comments inline.  Some are rather notes to self, but the more you
> > can address the better.
>
> I've tried to address as many comments as possible, see my comments
> inline.

Very well, thanks!

> > > +# Vala
> > > +register_language ('name' => 'vala',
> > > +            'Name' => 'Vala',
> > > +            'config_vars' => ['VALAC'],
> > > +            'flags' => ['VALAFLAGS'],
> > > +            'compile' => '$(VALAC) $(AM_VALAFLAGS) $(VALAFLAGS)',
> > > +            'compiler' => 'VALACOMPILE',
> > > +            'extensions' => ['.vala'],
> > > +            'output_extensions' => sub { (my $ext = $_[0]) =~ s/vala$/c/;
> > > +                                         return ($ext,) },
> > > +            'rule_file' => 'vala',
> > > +            '_finish' => \&lang_vala_finish,
> > > +            '_target_hook' => \&lang_vala_target_hook,
> > > +            'nodist_specific' => 1);
> >
> > The nodist_specific setting should be exposed in some test (i.e., after
> > following recommendations below, there should be a test that fails if
> > this setting is removed).
>
> I'm not familiar enough with nodist_ sources, can you help here?

Sure.  This really was more of a note to self.  I will address this
later.  Right now, I'm not positively sure myself about this, but I
think the idea is that distributed sources are updated in the source
tree while nodist_ sources are updated in the build tree.  If both
distributed and non-distributed sources exist in the same makefile, only
one of the two sets may be treated by inference rules, but IIRC automake
can pick either one, while the other has to use per-target rules.
I think this is about choosing which way to treat with inference rules.

Note that here, the question of distribution or not is about whether
*.vala files are distributed or not; they could also be generated, say.
If they are not distributed, then of course it does not make sense to
distributed the generated *.c files either.  Conversely, if the *.c are
distributed, then their *.vala inputs should also be distributed.

> > > +  if ($var)
> > > +    {
> > > +      foreach my $file ($var->value_as_list_recursive)
> > > +        {
> > > +          $output_rules .= "$file: ${derived}_vala.stamp ;\n"
> >
> > Hmm.  This doesn't look like one of the approaches mentioned in
> >   info Automake "Multiple Outputs"
>
> I've added the recover from removal commands recommended in the
> documentation.

Good.

> > This renaming of outputs is necessary for something like
> >   bin_PROGRAMS = foo bar
> >   foo_SOURCES = baz.vala
> >   bar_SOURCES = baz.vala
> >   bar_VALAFLAGS = -bla
> >
> > where the -bla flag causes the .c files for foo and bar to be different.
> >
> > This should be exposed in the testsuite (as XFAIL if it is not fixed).
>
> Done (as XFAIL).

> > > +  # 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);
> > > +    }
> >
> > Is this exposed in the testsuite additions (i.e., if you remove this
> > loop, does a test fail)?
>
> No, the testsuite does not seem to test this. This loop appears to have
> been copied by Mathias from other places in automake.in. I don't know
> how to properly test this.

If you have bar_VALAFLAGS in Makefile.am, then that, instead of
AM_VALAFLAGS, should appear in the rule which runs valac for
bar_SOURCES.  The vala5.test uses this; but since the test is XFAIL,
it doesn't expose it yet.

> > The outputs need to be renamed (see above) if per-target flags are used.
>
> Per-target vala flags are marked as XFAIL as noted above.

Do you intend to work on this?  Are there issues impeding a resolution
of this outside of Automake; IOW, is this a problem for valac also, or
only within automake.in?  In the latter case I can probably fix it.
(The thing works quite similarly to how bison and yacc are treated.)

> > If this is distributed, does that mean you intend to always distribute
> > the generated ${derived}_SOURCES?  Are those .c files
> > system-independent?  What about accompanying .h files?
>
> Yes, the .c files are system-independent and distributed. The Vala
> compiler does not generate .h files by default. The user will need to
> use -H foo.h to generate a .h file for a library and have it distributed
> and installed by adding foo.h to _HEADERS.

Hmm, I don't see this being done in the tests anywhere.  Can we change
one (working) test so that it does more or less the way you intend for
it to be done by your users?

> > > +sub lang_vala_target_hook
> > > +{
> > > +  my ($self, $aggregate, $output, $input, %transform) = @_;
> > > +
> > > +  $clean_files{$output} = MAINTAINER_CLEAN;
> > > +}

Note to self: if nodist_ is implemented, then this should be adjusted
(nodist_ sources can be cleaned upon 'make clean', not only upon
maintainer-clean).

[ testsuite exposure ]

> > > +grep 'VALAC' Makefile.in
> > > +grep 'src_zardoz_OBJECTS' Makefile.in
> > > +grep 'src_libzardoz_la_OBJECTS' Makefile.in
> > > +grep 'src_zardoz_vala.stamp' Makefile.in
> > > +grep 'src_libzardoz_la_vala.stamp' Makefile.in
> > > +grep 'zardoz\.c' Makefile.in
> > > +grep 'src/zardoz-foo\.c' Makefile.in
> >
> > This is good, but we should really also have a test that builds this
> > stuff with subdir sources, i.e., including running valac and all.  Since
> > Vala that requires so many prerequisites, maybe it's better to do that
> > in a separate test; just copy this one, add stuff to $required.
>
> vala3.test tests full build with non-recursive make and subdirs.

Cool.

> > When all of that passes, then we are probably ok in this area.
> > Well, rebuilding could be tested: touch or remove one of the input files
> > or intermediates, ensure things are rebuilt as necessary.
>
> I didn't add a test for this, wasn't sure how to properly check this.
> Various manual tests worked fine.

Do *.vala files include other files?  IOW, do the generated *.c files
depend upon other files?  Note this question is not about ordinary C
"#include", which introduces dependencies of *.o files on header files.

Something like
  $sleep
  echo "a vala syntax error" > zardoz.vala
  $MAKE && Exit 1       # Ensure failure

and similarly, if there are inclusions,
  $sleep
  echo "a vala syntax error" > included-file.vala
  $MAKE && Exit 1

> [1] http://bugzilla.gnome.org/show_bug.cgi?id=572536

Thanks,
Ralf