Created attachment 133923 [details] [review]
Use auto vars in make rules

Hrm, looking at some other uses of glib-mkenums (gnome-panel, gtk+, glib), nearly everybody is doing it this way. So why is it only broken for libwnck?

(also, the second part of the patch is wrong: we don't want to pass wnck-enum-types.h to glib-mkenums)

For gnome-panel, the header files are referenced with $(top_srcdir)/... prefix. So, it's OK for invoking from builddir. For example:

libpanel-applet/Makefile.am:

---8<---
panel_applet_enum_headers = $(top_srcdir)/libpanel-applet/panel-applet.h

panel-applet-enums.c: @REBUILD@ $(panel_applet_enum_headers)
        $(AM_V_GEN)glib-mkenums \
                     --fhead "#include <glib-object.h>\n" \
                     --fhead "#include \"panel-applet-enums.h\"\n\n" \
                     --fprod "\n/* enumerations from \"@filename@\" */" \
                     --fprod "\n#include \"@filename@\"\n" \
                     --vhead "static const G@Type@Value _@enum_name@_values[] = {" \
                     --vprod "  { @VALUENAME@, \"@VALUENAME@\", \"@valuenick@\" }," \
                     --vtail "  { 0, NULL, NULL }\n};\n\n" \
                     --vtail "GType\n@enum_name@_get_type (void)\n{\n" \
                     --vtail "  static GType type = 0;\n\n" \
                     --vtail "  if (!type)\n" \
                     --vtail "    type = g_@type@_register_static (\"@EnumName@\", _@enum_name@_values);\n\n" \
                     --vtail "  return type;\n}\n\n" \
                $(panel_applet_enum_headers) > $@
---8<---

$(panel_applet_enum_headers) refers to a header file in the source dir. So, it works.

Similarly for applets/clock/Makefile.am:

---8<---
clock_enum_headers =                                    \
        $(top_srcdir)/applets/clock/clock-utils.h
---8<---

and gnome-panel/Makefile.am:

---8<---
panel_enum_headers =                                    \
        $(top_srcdir)/gnome-panel/panel-enums.h         \
        $(top_srcdir)/gnome-panel/panel-types.h         \
        GNOME_Panel.h
---8<---

GNOME_Panel.h here is a file generated from $(ORBIT_IDL) command under the $(CORBA_SRCLIST) target. So, it's already in the build dir, and can be referenced without dirname prefix.

For glib, gio/Makefile.am in particular:

---8<---
gioenumtypes.h: $(gio_headers) gioenumtypes.h.template
        ( top_builddir=`cd $(top_builddir) && pwd`; \
          cd $(srcdir) && $$top_builddir/gobject/glib-mkenums --template gioenumtypes.h.template $(gio_headers) ) > \
            gioenumtypes.h.tmp && mv gioenumtypes.h.tmp gioenumtypes.h

gioenumtypes.c: $(gio_headers) gioenumtypes.c.template
        ( top_builddir=`cd $(top_builddir) && pwd`; \
          cd $(srcdir) && $$top_builddir/gobject/glib-mkenums --template gioenumtypes.c.template $(gio_headers) ) > \
            gioenumtypes.c.tmp && mv gioenumtypes.c.tmp gioenumtypes.c
---8<---

It works because the glib-mkenums command is invoked under the source dir!

Now come to libwnck itself:

---8<---
wnck_headers=                   \
        libwnck.h               \
        pager.h                 \
        application.h           \
        class-group.h           \
        screen.h                \
        selector.h              \
        tasklist.h              \
        util.h                  \
        window.h                \
        window-action-menu.h    \
        workspace.h
---8<---

There is no dirname prefix for the headers. So, the following make rules try to access them in the current directory, the build directory:

---8<---
wnck-enum-types.h: $(wnck_headers)
        $(AM_V_GEN)glib-mkenums \
                --fhead "#ifndef __WNCK_ENUM_TYPES_H__\n" \
                --fhead "#define __WNCK_ENUM_TYPES_H__\n\n" \
                --fhead "G_BEGIN_DECLS\n\n" \
                --ftail "G_END_DECLS\n\n" \
                --ftail "#endif /* __WNCK_ENUM_TYPES_H__ */\n" \
                --fprod "\n/* enumerations from \"@filename@\" */\n" \
                --eprod "#define WNCK_TYPE_@ENUMSHORT@ @enum_name@_get_type()\n" \
                --eprod "GType @enum_name@_get_type (void);\n" \
                $(wnck_headers) > $@

wnck-enum-types.c: $(wnck_headers) wnck-enum-types.h
        $(AM_V_GEN)glib-mkenums \
                --fhead "#include <libwnck/libwnck.h>\n" \
                --fprod "\n/* enumerations from \"@filename@\" */" \
                --vhead "static const GEnumValue _@enum_name@_values[] = {" \
                --vprod "  { @VALUENAME@, \"@VALUENAME@\", \"@valuenick@\" }," \
                --vtail "  { 0, NULL, NULL }\n};\n\n" \
                --vtail "GType\n@enum_name@_get_type (void)\n{\n" \
                --vtail "  static GType type = 0;\n\n" \
                --vtail "  if (!type)\n" \
                --vtail "    type = g_enum_register_static (\"@EnumName@\", _@enum_name@_values);\n\n" \
                --vtail "  return type;\n}\n\n" \
                $(wnck_headers) > $@
---8<---

A possible fix can be:

1. add $(top_srcdir)/... prefixes to the header files, just like what's done in gnome-panel, or

2. cd to $(srcdir) and invoking gtk-mkenums there, just like what glib does, or

3. refer to the headers with automatic make variables $^, which will be replaced with the rule's dependencies as found via VPATH search, as I proposed in the patch.

As I did't see the code in other packages when I prepared the fix, I chose the one with minimum change. Honestly speaking, solution 2, glib's method, was out of my imagination at that time, and I don't think it's suitable for libwnck, as the generated enum header is listed under the CLEANFILES. It will be out of reach of 'make clean' as it appeared to be originally designed.

> (also, the second part of the patch is wrong: we don't want to pass
> wnck-enum-types.h to glib-mkenums)

You're right. So, solution 1 may be the best in this case.

(In reply to comment #3)
> > (also, the second part of the patch is wrong: we don't want to pass
> > wnck-enum-types.h to glib-mkenums)
> 
> You're right. So, solution 1 may be the best in this case.

Or alternatively, why not just remove wnck-enum-types.h from wnck-enum-types.c dependencies? Is wnck-enum-types.h expected to be manually changed?

Just a little note about style: $(wnck_headers) is also used in libwnckinclude_HEADERS, unlike other packages examined here. If adding dirname prefixes is also OK in that place, solution 1 is OK.

Created attachment 139447 [details] [review]
Updated patch for auto vars method

Updated patch for solution 3 (auto vars method), without passing wnck-enum-types.h to glib-mkenums. $(wnck_built_headers) is added to $(libwnck_1_la_OBJECTS) dependencies to guarantee the built headers are still built.

*** Bug 605410 has been marked as a duplicate of this bug. ***

Thanks, committed!