GNOME Bugzilla – Bug 766621
Improve Vala m4
Last modified: 2017-12-01 23:54:53 UTC
Created attachment 328151 [details] GXml valac.m4 version Vala requires some directory detection, in order to install correctly VAPI generated files, then is necesary to add macros to vala.m4 in order to do so. As for GXml, it has valac.m4 to check for vala compiler, versioned and unversioned and sets VAPI dirs. May is a good idea to call: VALAC_CHECK instead of: VAPIGEN_CHECK The last is useful just for C libraries to create Vala bidings, but useless for pure Vala projects.
Seems reasonable. You mind adding a #serial line to the .m4 file so that we know when we can update it? At some point I'd like to auto-update .m4 scripts in autotools packages and we'll rely on #serial to do so. Before long, we need to add plumbing for "advanced features" so we can toggle things like G-I, Vala bindings, etc. Until then I think we can just write this out for all projects like we do now. I am worried that the VALAC_CHECK will fail when vala is not installed, and we should ensure we can support C libraries without vala bindings. So we might need to do something like this in the configure.ac: {{if template == 'shared-library'}} {{if enable_c}} VAPIGEN_CHECK {{end}} {{end}} In writing the above, I realized I still need to implement && and || for template-glib (which I'll do now). It also might be good to get this into autoconf-archive.
So you do for G-I. Because makes no same for non-library projects, unless it is written in Python, JavaScript or any other languages supporting G-I.
G-I is useful for programs too that do plugins too, even when a library isn't used.
(In reply to Christian Hergert from comment #1) > Seems reasonable. You mind adding a #serial line to the .m4 file so that we > know when we can update it? At some point I'd like to auto-update .m4 > scripts in autotools packages and we'll rely on #serial to do so. > > Before long, we need to add plumbing for "advanced features" so we can > toggle things like G-I, Vala bindings, etc. Until then I think we can just > write this out for all projects like we do now. > > I am worried that the VALAC_CHECK will fail when vala is not installed, and > we should ensure we can support C libraries without vala bindings. So we > might need to do something like this in the configure.ac: > > {{if template == 'shared-library'}} > {{if enable_c}} > VAPIGEN_CHECK > {{end}} > {{end}} > There are additional issues with VAPIGEN_CHECK for C libraries not using GLib/GObject. I've filed bug #768585, to track it. > In writing the above, I realized I still need to implement && and || for > template-glib (which I'll do now). > > It also might be good to get this into autoconf-archive. Your
Created attachment 331104 [details] [review] valac.m4 and fixing VAPI installation With this patch I managed to fix VAPI installation for shared libraries written in Vala. While Makefile.am is different when using Vala, no fixes is required on ones for C. On configure.ac, there was an already existing pre-condition when Vala is enable, then VALAC_CHECK() will be called in that case, witch is correct for Vala projects, and correct too if Vala is not installed. May be the only issue here, when compiling a tar.gz package from a Vala project. It has all C generated code, then no Vala is required at all, but with this macro you need Vala installed at configure time. May be a way to fix this is to force VALAC_CHECK() to not fail but warning. Then any one can build a tar.gz package without Vala at all.
Review of attachment 331104 [details] [review]: sure
Oh weird, why is the patch only a couple line patch? It doesn't seem to be inline with comment #5
Created attachment 331261 [details] [review] new valac.m4 and fixing VAPI installation Sorry, for last. This is a patch for fixing VAPI installation and adding new valac.m4 macro
Review of attachment 331261 [details] [review]: Couple quick fixes. Just to double check, can you use the gnome-builder-cli (formally ide) to ensure that templates build for various combinations of c/c++/vala if you have not yet done so. We need a way to get that implemented as part of our test suite... ::: plugins/autotools-templates/autotools_templates/__init__.py @@ +144,3 @@ scope.get('enable_gtk_doc').assign_boolean(False) scope.get('enable_gobject_introspection').assign_boolean(True) + scope.get('enable_vapi').assign_boolean(False) rebase to get this line from the other patch ::: plugins/autotools-templates/autotools_templates/resources/m4/valac.m4 @@ +16,3 @@ +dnl License along with this library; if not, write to the Free Software +dnl Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + Add a #serial so we know when we need to update the file in the future.
Sorry, but what do you mean about #serial?
(In reply to Daniel Espinosa from comment #10) > Sorry, but what do you mean about #serial? https://git.gnome.org/browse/gnome-builder/tree/build/autotools/ax_append_compile_flags.m4#n59
We can use that to update the source files automatically in users projects in the future.
Created attachment 331280 [details] [review] Adding valac.m4, serial to it, fixed VAPI installation and better Vala example This is my last patch to apply, adding valac.m4, with a serial number 8, fixes VAPI installation and improves Vala code example. This patch has been tested this way: Using ide tool, I've created three projects: 1) btest, my base Vala library project test, it was installed in the system 2) ltest, my dependant Vala library project test. It depends on btest, use a class in its Vala code, compiles and links against btest, to check installation of VAPI, library and header bars 3) ctest, my dependant C library project test. It depends on btest, use a class in its C code, compiles and links against btest, to check installation of library and header bars All tests passed.
Created attachment 331281 [details] ltest library test to compile against btest Vala library project I'm attaching my ltest Vala library project used to test ide tool generated btest. In automatic tests, may we can uncompress it, configure && make, then we can check if btest it correctly installed and detected by ltest project.
Created attachment 331282 [details] ctest library test project for btest project tests I'm attaching my ctest C library project used to test ide tool generated btest. In automatic tests, may we can uncompress it, configure && make, then we can check if btest it correctly installed and detected by ctest project.
I'm using GNU Make 4.1 under Debian Testing and ctest project fails on make distcheck due to Automake Vala support bugs. make tries to delete vala stamp on read only directory, which is incorrect from make point of view, but automake Vala implementation change to $(srdir) to create C and stamp files, while they should be created on $(builddir). I've sent a patch to bug #13002 on GNU bug tracker system to fix Vala implementation, then if we can't get this bug fixed, we need to use GXml's workarounds; doing so, will require to add some temporary M4 macros into Builder's Autotools templates.
Please make your patch against latest commit: https://git.gnome.org/browse/gxml/commit/?id=936565d0e32a16a5b03f5dac0835574f2c33a5f8 Thanks again for all your help.
(In reply to Daniel Espinosa from comment #17) > Please make your patch against latest commit: > > https://git.gnome.org/browse/gxml/commit/ > ?id=936565d0e32a16a5b03f5dac0835574f2c33a5f8 > > Thanks again for all your help. This is a mistake. Please avoid this comment.
Created attachment 332957 [details] [review] Autotools template: fix make discheck for Vala libraries templates This patch in one go: Autotools-templates: Fixed Vala shared library make distcheck This work do the following: * Add vala.mk for general Vala projects build defining basic variables to be set to get your code compiled * Add valalib.mk for Vala libraries builds adding all basic variables to be set to get you library generating a header and a VAPI, along with required installation setup * Add valalib_introspection.mk for GObject Introspection support for Vala libraries All above make a clean Makefile.am because all details and work around to Automake Bug #13002 are included by external makefiles, each one have enough documentation to help others to create their own Autotools based Vala libraries, there's a project example at: https://github.com/esodan/automakevala autogen.sh, make and make distcheck should work now, as it was tested using gnome-builder-cli command line program.
*** Bug 768541 has been marked as a duplicate of this bug. ***
Created attachment 332993 [details] [review] Added support for unit tests to Vala libraries templates, plus last improvements In addition of above improvements to Vala libraries templates, this patch add support to generate: * Unit Tests for Vala libraries, making easy to add this vital feature
Review of attachment 332993 [details] [review]: The files have a hardcoded license header with your name. I guess that's not correct.
Created attachment 333030 [details] [review] Updated patch removing hard coded license This patch include all latest patches, from my local repository history, against master. I've removed all hard coded licenses.
The link at the top of gtester.mk appears to be broken.
(In reply to Matthew Leeds from comment #24) > The link at the top of gtester.mk appears to be broken. Yes it is, but it still works, I really don't know where this mk comes from, I tried to find it on GLib but no success. Once approved to commit, I'll delete this URL and push my local work history.
Review of attachment 333030 [details] [review]: Sorry it took so long to get a review, but this was a very technically oriented problem and it takes a significant amount of time to get through. My overall thought is that this is trying to be a bit too clever for what we want in our templates. Using the included .mk files today requires a bit of knowledge on which variables need to be defined and when things should be included. Additionally, they don't work if the .mk files are outside the top_srcdir. Since we want to keep our top-level directories rather sparse, we need those moved into something like build-aux (which conveniently allows us to use $ac_aux_dir). I know that _SOURCES has it's issues (we don't even use it in Builder's vala code) because it locks you into a Vala version with `make dist`, but it is much more approachable from a template standpoint. For developers that know they need to care about that, changing their Makefile is something we can rely on them to do. Additionally, it simplifies the process for us to be able to modify the existing makefiles in Builder to add sources to appropriate targets (when we land that Build System feature). ::: contrib/tmpl/tmpl-branch-node.c @@ +72,3 @@ TMPL_ERROR, TMPL_ERROR_SYNTAX_ERROR, + "Template: Syntax error: Unexpected end-of-file reached"); Please break the template-glib patches into a separate commit. ::: plugins/autotools-templates/Makefile.am @@ -19,2 +19,3 @@ autotools_templates/resources/data/package.pc.in \ autotools_templates/resources/git.mk \ + autotools_templates/resources/gtester.mk \ I want to see these in build-aux/ and then use $ac_aux_dir instead of $top_srcdir. It's important we keep the top directory relatively sparse. ::: plugins/autotools-templates/autotools_templates/__init__.py @@ -179,2 +179,3 @@ 'resources/configure.ac': 'configure.ac', 'resources/git.mk': 'git.mk', + 'resources/gtester.mk': 'gtester.mk', Again, build-aux here. ::: plugins/autotools-templates/autotools_templates/resources/data/package.pc.in @@ -0,0 +1,11 @@ +prefix=@prefix@ +exec_prefix=@exec_prefix@ +libdir=@libdir@ ... 8 more ... gio-2.0? I think we use gio for our PKG_CHECK_MODULES() line. ::: plugins/autotools-templates/autotools_templates/resources/src/Makefile.shared-library-vala @@ +1,1 @@ +include $(top_srcdir)/valalib.mk -include $(ac_aux_dir)/valalib.mk Although, I really would prefer that we do this like INTROSPECTION_MAKEFILE and use the m4 files to define the path to include. Something like: -include $(VALALIB_MAKEFILE) @@ -26,1 +35,14 @@ --include $(top_srcdir)/git.mk +VALA_CHEADER = {{prefix}}.h +VALA_INCLUDEDIR = {{Prefix}}-@API_VERSION@ + ... 11 more ... Same here, I'd like to see: -include $(VALALIB_INTROSPECTION_MAKEFILE) ::: plugins/autotools-templates/autotools_templates/resources/src/package.vala @@ +5,3 @@ namespace {{Prefix}} { + public class A { + public bool foo() { return false; } Probably just leave the namespace empty. If the users need a vala tutorial, we should make a tutorial template. ::: plugins/autotools-templates/autotools_templates/resources/tests/Makefile.shared-library-vala @@ +5,3 @@ +noinst_PROGRAMS = $(VALA_TARGET) + +VALASOURCES = \ I don't like how there is a single VALASOURCES here. I want to see the ability to have multiple targets defined in the Makefile.am without all the .c/.h files showing up in all targets. Personally, I'd say we should just use _SOURCES today. There are obviously shortcomings in automake's handling of _SOURCES, but lets keep it simple for our templates. ::: plugins/autotools-templates/autotools_templates/resources/tests/package-case-test.vala @@ +6,3 @@ + public static void add_funcs () + { + Test.add_func ("/atest/case/one", I don't want to nit too much on a language I don't write daily, but this doesn't seem to follow the vala code style I've seen in the past. If this is actually the suggested vala style (as used in the vala codebase itself), then we should change our default editor settings for vala sources. ::: plugins/autotools-templates/autotools_templates/resources/vala.mk @@ +24,3 @@ +# you have to set your target sources to target_SOURCES = $(VALA_CSOURCES). +# +# Use standard Autotools variables to set CFLAGS, LDFLAGS and LIBADD, as required. This file needs to handle some variables not being set (or already being set) better. But if we end up with just using _SOURCES I assume it can be dropped altogether. ::: plugins/autotools-templates/autotools_templates/resources/vala_tester.mk @@ +1,1 @@ +include $(top_srcdir)/gtester.mk Use $(ac_aux_dir) and put gtester.mk in build-aux/ @@ +1,3 @@ +include $(top_srcdir)/gtester.mk + +include $(top_srcdir)/vala.mk Same. @@ +5,3 @@ +TEST_PROGS += $(VALA_TARGET) + +DISTCLEANFILES = \ Seems very weird to me that the Makefile.am would have to put the executable into DISTCLEANFILES. ::: plugins/autotools-templates/autotools_templates/resources/valalib.mk @@ +27,3 @@ +# and INTROSPECTION_TYPELIBS. VALA_TARGET should be a library. + +include $(top_srcdir)/vala.mk Use $(ac_aux_dir) and put this in build-aux/ @@ +30,3 @@ + + +VALAFLAGS += \ I don't like how this can only be used once per Makefile.am. I'd rather we use _SOURCES in our templates despite the shortcomings.
Created attachment 334408 [details] [review] Improve error messages in template parser In attention to comment #26, about to split patch. Thanks for all your recommendations. This is part of the work. Next patch should attend all other comments.
(In reply to Christian Hergert from comment #26) > ::: plugins/autotools-templates/Makefile.am > @@ -19,2 +19,3 @@ > autotools_templates/resources/data/package.pc.in \ > autotools_templates/resources/git.mk \ > + autotools_templates/resources/gtester.mk \ > > I want to see these in build-aux/ and then use $ac_aux_dir instead of > $top_srcdir. It's important we keep the top directory relatively sparse. > > ::: plugins/autotools-templates/autotools_templates/__init__.py > @@ -179,2 +179,3 @@ > 'resources/configure.ac': 'configure.ac', > 'resources/git.mk': 'git.mk', > + 'resources/gtester.mk': 'gtester.mk', > > Again, build-aux here. > For this I'm moving mk files to build-aux/ > ::: > plugins/autotools-templates/autotools_templates/resources/data/package.pc.in > @@ -0,0 +1,11 @@ > +prefix=@prefix@ > +exec_prefix=@exec_prefix@ > +libdir=@libdir@ > ... 8 more ... > > gio-2.0? I think we use gio for our PKG_CHECK_MODULES() line. > Changed to gio-2.0 > ::: > plugins/autotools-templates/autotools_templates/resources/src/Makefile. > shared-library-vala > @@ +1,1 @@ > +include $(top_srcdir)/valalib.mk > > -include $(ac_aux_dir)/valalib.mk > > Although, I really would prefer that we do this like INTROSPECTION_MAKEFILE > and use the m4 files to define the path to include. Something like: > > -include $(VALALIB_MAKEFILE) > Where is the better place to define this macros. While we have already a m4/valac.m4, where this definitions could be; they could be also in configure.ac. For valac.m4 I can add a VALAC_MAKEFILES macro to be called in configure.ac for Vala projects, but they should be defined, for example, as: VALA_MAKEFILE="@top_srcdir@/build-aux/vala.mk" AC_SUBST(VALA_MAKEFILE) if defined in configure.ac, then @top_srcdir@. > @@ -26,1 +35,14 @@ > --include $(top_srcdir)/git.mk > +VALA_CHEADER = {{prefix}}.h > +VALA_INCLUDEDIR = {{Prefix}}-@API_VERSION@ > + > ... 11 more ... > > Same here, I'd like to see: > > -include $(VALALIB_INTROSPECTION_MAKEFILE) > > ::: > plugins/autotools-templates/autotools_templates/resources/src/package.vala > @@ +5,3 @@ > namespace {{Prefix}} { > + public class A { > + public bool foo() { return false; } > > Probably just leave the namespace empty. If the users need a vala tutorial, > we should make a tutorial template. > This is important, as they should be the prefix for all definitions and for Introspection GIR. In future improvements, GNOME Builder should include fields for namespace. > ::: > plugins/autotools-templates/autotools_templates/resources/tests/Makefile. > shared-library-vala > @@ +5,3 @@ > +noinst_PROGRAMS = $(VALA_TARGET) > + > +VALASOURCES = \ > > I don't like how there is a single VALASOURCES here. I want to see the > ability to have multiple targets defined in the Makefile.am without all the > .c/.h files showing up in all targets. > > Personally, I'd say we should just use _SOURCES today. There are obviously > shortcomings in automake's handling of _SOURCES, but lets keep it simple for > our templates. > I'm using this solution because, there is a bug in Automake handling _SOURCES, I've proposed some patches, but no response at this time, and they should not be ready for 3.22. This bug prevent make distcheck to be called sucessfully, as current implementation tries to create a stamp on a read only directory, when this stamp should be created on buildir, but sure valac version 0.7, as used when added support to Vala, don't supports VPATH building and automake don't use a read only directory when make distcheck is called. When Automake is fixed, then GNOME Builder templates should be either. > ::: > plugins/autotools-templates/autotools_templates/resources/tests/package-case- > test.vala > @@ +6,3 @@ > + public static void add_funcs () > + { > + Test.add_func ("/atest/case/one", > > I don't want to nit too much on a language I don't write daily, but this > doesn't seem to follow the vala code style I've seen in the past. If this is > actually the suggested vala style (as used in the vala codebase itself), > then we should change our default editor settings for vala sources. > This is the one used just for unit tests. I think you don't need to change any thing. Just src/ should be in like on common Vala code style. If you see an issue here, then we can discuss more. > ::: plugins/autotools-templates/autotools_templates/resources/vala.mk > @@ +24,3 @@ > +# you have to set your target sources to target_SOURCES = $(VALA_CSOURCES). > +# > +# Use standard Autotools variables to set CFLAGS, LDFLAGS and LIBADD, as > required. > > This file needs to handle some variables not being set (or already being > set) better. > > But if we end up with just using _SOURCES I assume it can be dropped > altogether. > > ::: plugins/autotools-templates/autotools_templates/resources/vala_tester.mk > @@ +1,1 @@ > +include $(top_srcdir)/gtester.mk > > Use $(ac_aux_dir) and put gtester.mk in build-aux/ > > @@ +1,3 @@ > +include $(top_srcdir)/gtester.mk > + > +include $(top_srcdir)/vala.mk > > Same. > As for last comments, _SOURCES should be used and almost all mk files should be removed once Automake is fixed. > @@ +5,3 @@ > +TEST_PROGS += $(VALA_TARGET) > + > +DISTCLEANFILES = \ > > Seems very weird to me that the Makefile.am would have to put the executable > into DISTCLEANFILES. May this is redundant if make clean removes it, even if not in DISTCLEANFILES. > > ::: plugins/autotools-templates/autotools_templates/resources/valalib.mk > @@ +27,3 @@ > +# and INTROSPECTION_TYPELIBS. VALA_TARGET should be a library. > + > +include $(top_srcdir)/vala.mk > > Use $(ac_aux_dir) and put this in build-aux/ See above. > > @@ +30,3 @@ > + > + > +VALAFLAGS += \ > > I don't like how this can only be used once per Makefile.am. I'd rather we > use _SOURCES in our templates despite the shortcomings. The problem is that is not just shortcomings, current Vala support in Automake, fails on make distcheck because timestamp writing in VPATH read only source directory.
In order to acomplish your request to provide an GObject Introspection like solution for Vala, may you wish to file a bug to Vala product and include this macros upstream. This will help to distribute them along with Vala, included in GNOME Builder auto-generated projects from Vala's .pc file variables. Including this macros to Vala's installation, makes your project dependent on Vala installation, which can be avoided if all necessary files to build, are distribute with your tarball.
This is obsolete going forward since we use meson now and will likely drop our autotools templates before long.
Meson is not ready for Windows, not for MinGW not for it's VCS backend. Si I have to add autotools to GSVG recently, after Meson fails to be a substitute. May you want how autotools and Meson live live side by side, on GSVG. May can help to include both in your templates. Not doing so, just makes Vala projects unable to be compiled on MinGW, out of the box for new comers.
This is GSVG repository https://gitlab.com/pwmc/gsvg
I'm not interested in maintaining this in Builder. I don't want to maintain custom m4/mk code that isn't upstream. I also don't want custom make targets in generated files. Things should get fixed upstream in automake and not put on the shoulders of Builder to maintain. I'd rather have the annoying pattern of .vala files in _SOURCES than maintain more autotools integration code that will never get tested. At least in that case I know things will work (even if it means you need a given vala version). Nor is Builder a battlefield for improving Vala support on Windows, as it is outside of our current mission. If we are going to maintain autotools templates (we don't even build them in our suggested configuration), they should probably even be simplified to their most simple state. I'm almost certain that if your mingw issues are reported to Meson, they'll be fixed faster than we can ever ship improved vala templates for autotools. This should in no way discourage you from improving the situation of Vala, autotools, and Windows support. I just don't think Builder is the right place to fix those issues.