After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 788948 - Document Autotools best practices for genmarshal/mkenums
Document Autotools best practices for genmarshal/mkenums
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: docs
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2017-10-13 15:14 UTC by Emmanuele Bassi (:ebassi)
Modified: 2017-10-13 20:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
docs: Update glib-genmarshal man page (2.53 KB, patch)
2017-10-13 15:14 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
docs: Update glib-mkenums man page (8.22 KB, patch)
2017-10-13 15:14 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
docs: Update glib-genmarshal man page (2.54 KB, patch)
2017-10-13 15:42 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
docs: Update glib-mkenums man page (8.23 KB, patch)
2017-10-13 15:42 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
docs: Update glib-genmarshal man page (2.54 KB, patch)
2017-10-13 17:10 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
docs: Update glib-mkenums man page (8.31 KB, patch)
2017-10-13 17:10 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review

Description Emmanuele Bassi (:ebassi) 2017-10-13 15:14:08 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.
Comment 1 Emmanuele Bassi (:ebassi) 2017-10-13 15:14:12 UTC
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.
Comment 2 Emmanuele Bassi (:ebassi) 2017-10-13 15:14:16 UTC
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.
Comment 3 Simon McVittie 2017-10-13 15:35:17 UTC
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
Comment 4 Simon McVittie 2017-10-13 15:37:15 UTC
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
Comment 5 Emmanuele Bassi (:ebassi) 2017-10-13 15:42:33 UTC
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.
Comment 6 Emmanuele Bassi (:ebassi) 2017-10-13 15:42:38 UTC
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.
Comment 7 Emmanuele Bassi (:ebassi) 2017-10-13 15:43:55 UTC
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.
Comment 8 Philip Withnall 2017-10-13 16:07:08 UTC
(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?
Comment 9 Emmanuele Bassi (:ebassi) 2017-10-13 16:09:29 UTC
(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.
Comment 10 Philip Withnall 2017-10-13 16:10:13 UTC
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/
Comment 11 Emmanuele Bassi (:ebassi) 2017-10-13 16:12:50 UTC
Done: https://github.com/pkgconf/pkgconf/issues/147
Comment 12 Philip Withnall 2017-10-13 16:18:36 UTC
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/
Comment 13 Philip Withnall 2017-10-13 16:20:58 UTC
Despite my pernickety review comments, I am very much in favour of these patches. Thank you!
Comment 14 Emmanuele Bassi (:ebassi) 2017-10-13 17:10:33 UTC
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.
Comment 15 Emmanuele Bassi (:ebassi) 2017-10-13 17:10:38 UTC
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.
Comment 16 Emmanuele Bassi (:ebassi) 2017-10-13 17:11:56 UTC
(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.
Comment 17 Emmanuele Bassi (:ebassi) 2017-10-13 17:13:40 UTC
(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.
Comment 18 Philip Withnall 2017-10-13 17:22:12 UTC
Review of attachment 361544 [details] [review]:

woop!
Comment 19 Philip Withnall 2017-10-13 17:24:07 UTC
Review of attachment 361545 [details] [review]:

Looks good, thanks. Add the BUILT_SOURCES stuff to both patches before pushing.
Comment 20 Emmanuele Bassi (:ebassi) 2017-10-13 20:30:45 UTC
Attachment 361544 [details] pushed as bf4f825 - docs: Update glib-genmarshal man page
Attachment 361545 [details] pushed as cd97f93 - docs: Update glib-mkenums man page