GNOME Bugzilla – Bug 788948
Document Autotools best practices for genmarshal/mkenums
Last modified: 2017-10-13 20:30:57 UTC
Friends, Romans, countrymen, lend me your ears. I come to bury Autotools, not to praise them. The evil that men do lives after them; The good is oft interred with their bones. In the hope that fewer people will end up cargo-culting rules written in the late '90s and never updated, while we move away from Autotools altogether.
Created attachment 361532 [details] [review] docs: Update glib-genmarshal man page We should show how to properly use glib-genmarshal with Autotools, in the hope that fewer people will be caught cargo-culting rules written in the late '90s.
Created attachment 361533 [details] [review] docs: Update glib-mkenums man page We should show how to properly use glib-mkenums with Autotools, in the hope that fewer people will be caught cargo-culting rules written in the late '90s.
Review of attachment 361532 [details] [review]: ::: docs/reference/gobject/glib-genmarshal.xml @@ +423,3 @@ +PKG_PROG_PKG_CONFIG + +GLIB_GENMARSHAL=`$(PKG_CONFIG) --variable=glib_genmarshal glib-2.0` Or this? PKG_CHECK_VAR([GLIB_GENMARSHAL], [glib-2.0], [glib_genmarshal]) (Requires pkg-config 0.28, new in 2014) That lets the user do "./configure GLIB_GENMARSHAL=/opt/bin/glib-genmarshal" or something, to get a native glib-genmarshal when cross-compiling or whatever. @@ +437,3 @@ + +marshal.c: marshal.list marshal.h + $(AM_V_GEN) $(GLIN_GENMARSHAL) \ GLIB
Review of attachment 361533 [details] [review]: ::: docs/reference/gobject/glib-mkenums.xml @@ +393,3 @@ +PKG_PROG_PKG_CONFIG + +GLIB_MKENUMS=`$(PKG_CONFIG) --variable=glib_mkenums glib-2.0` This could also benefit from PKG_CHECK_VAR
Created attachment 361536 [details] [review] docs: Update glib-genmarshal man page We should show how to properly use glib-genmarshal with Autotools, in the hope that fewer people will be caught cargo-culting rules written in the late '90s.
Created attachment 361537 [details] [review] docs: Update glib-mkenums man page We should show how to properly use glib-mkenums with Autotools, in the hope that fewer people will be caught cargo-culting rules written in the late '90s.
Thanks Simon; I never remember to use PKG_CHECK_VAR — and Fedora switching to pkgconf means that the pkg-config man page doesn't list m4 macros any more.
(In reply to Emmanuele Bassi (:ebassi) from comment #7) > means that the pkg-config man page doesn't list m4 macros any > more. Would it be appropriate to file a bug against pkgconf?
(In reply to Philip Withnall from comment #8) > (In reply to Emmanuele Bassi (:ebassi) from comment #7) > > means that the pkg-config man page doesn't list m4 macros any > > more. > > Would it be appropriate to file a bug against pkgconf? Possibly. Need to figure out where their bug tracker lives.
Review of attachment 361536 [details] [review]: Looks good to me, modulo a few nitpicks. Feel free to push with these changes. ::: docs/reference/gobject/glib-genmarshal.xml @@ +416,3 @@ +In order to use <command>glib-genmarshal</command> in your project when using +Autotools as the build system, you will first need to modify your +<literal>configure.ac</literal> file to ensure you find the appropriate s/literal/filename/ (DocBook pedantry, sorry). @@ +426,3 @@ +</programlisting></informalexample> +<para> +In your <literal>Makefile.am</literal> file you will typically need very s/literal/filename/ @@ +431,3 @@ +<informalexample><programlisting> +marshal.h: marshal.list + $(AM_V_GEN) $(GLIB_GENMARSHAL) \ Nitpick: The convention seems to be to put no space between $(AM_V_GEN) and the actual command. @@ +448,3 @@ +<para> +In the example above, the first rule generates the header file and depends on +a <literal>marshal.list</literal> file in order to regenerate the result in s/literal/filename/ @@ +450,3 @@ +a <literal>marshal.list</literal> file in order to regenerate the result in +case the marshallers list is updated. The second rule generates the source file +for the same <literal>marshal.list</literal>, and includes the file generated s/literal/filename/
Done: https://github.com/pkgconf/pkgconf/issues/147
Review of attachment 361537 [details] [review]: A few anally-retentive nitpicks and a question about `project_headers` and the dependencies. ::: docs/reference/gobject/glib-mkenums.xml @@ +386,3 @@ +In order to use <command>glib-mkenums</command> in your project when using +Autotools as the build system, you will first need to modify your +<literal>configure.ac</literal> file to ensure you find the appropriate s/literal/filename/ @@ +396,3 @@ +</programlisting></informalexample> +<para> +In your <literal>Makefile.am</literal> file you will typically use rules s/literal/filename/ @@ +401,3 @@ +<informalexample><programlisting> +# A list of headers to inspect +project_headers = ... Maybe include an example list here, just to make it really obvious that this should be a list of .h files? Or maybe this should be cleverer and do something like the following, to avoid people ending up with multiple partially overlapping lists of headers? project_headers = $(filter %.h,$(project_SOURCES) $(project_HEADERS)) That’s probably getting too clever to be generally useful for non-autotools experts, though. @@ +404,3 @@ + +enum-types.h: $(project_headers) enum-types.h.in + $(AM_V_GEN) $(GLIB_MKENUMS) \ Nitpick: As with the previous patch, the convention seems to be to have no space after $(AM_V_GEN). @@ +409,3 @@ + $(project_headers) + +enum-types.c: $(project_headers) enum-types.c.in enum-types.h Does generation of enum-types.c really depend on enum-types.h already existing? @@ +451,3 @@ +<para> +The <literal>enum-types.c</literal> rule is similar to the rule for the +header file, but will use a different <literal>enum-types.c.in</literal> template s/literal/filename/
Despite my pernickety review comments, I am very much in favour of these patches. Thank you!
Created attachment 361544 [details] [review] docs: Update glib-genmarshal man page We should show how to properly use glib-genmarshal with Autotools, in the hope that fewer people will be caught cargo-culting rules written in the late '90s.
Created attachment 361545 [details] [review] docs: Update glib-mkenums man page We should show how to properly use glib-mkenums with Autotools, in the hope that fewer people will be caught cargo-culting rules written in the late '90s.
(In reply to Philip Withnall from comment #10) > Review of attachment 361536 [details] [review] [review]: > > Looks good to me, modulo a few nitpicks. Feel free to push with these > changes. Fixed all the literal/filename tags. > @@ +431,3 @@ > +<informalexample><programlisting> > +marshal.h: marshal.list > + $(AM_V_GEN) $(GLIB_GENMARSHAL) \ > > Nitpick: The convention seems to be to put no space between $(AM_V_GEN) and > the actual command. I don't particularly like it, because it makes it harder to read, but I've kept to the convention.
(In reply to Philip Withnall from comment #12) > @@ +401,3 @@ > +<informalexample><programlisting> > +# A list of headers to inspect > +project_headers = ... > > Maybe include an example list here, just to make it really obvious that this > should be a list of .h files? I've used the evergreen `project-foo.h project-bar.h project-baz.h` names. > Or maybe this should be cleverer and do something like the following, to > avoid people ending up with multiple partially overlapping lists of headers? > > project_headers = $(filter %.h,$(project_SOURCES) $(project_HEADERS)) > > That’s probably getting too clever to be generally useful for non-autotools > experts, though. Yeah, that's a bit too hardcore. > @@ +409,3 @@ > + $(project_headers) > + > +enum-types.c: $(project_headers) enum-types.c.in enum-types.h > > Does generation of enum-types.c really depend on enum-types.h already > existing? Given that the enum-types.c includes enum-types.h, it must, otherwise GCC will complain. Actually, that reminds me that I need to add the files to `BUILT_SOURCES` as well.
Review of attachment 361544 [details] [review]: woop!
Review of attachment 361545 [details] [review]: Looks good, thanks. Add the BUILT_SOURCES stuff to both patches before pushing.
Attachment 361544 [details] pushed as bf4f825 - docs: Update glib-genmarshal man page Attachment 361545 [details] pushed as cd97f93 - docs: Update glib-mkenums man page