GNOME Bugzilla – Bug 616425
Makefile.introspection: use $^, simplifying non-srcdir builds
Last modified: 2015-02-07 16:49:15 UTC
In the .gir-building rule, use "$^" to refer to the source files, since that automatically looks in both $(srcdir) and $(builddir). This is particularly important since certain generated files will be in $(builddir) when building from git, but in $(srcdir) when building from tarballs If you were previously prefixing $(srcdir) to the Foo_gir_FILES members by hand, you should stop now. (Also, fix a typoed variable name.)
Created attachment 159278 [details] [review] Makefile.introspection: use $^, simplifying non-srcdir builds
Comment on attachment 159278 [details] [review] Makefile.introspection: use $^, simplifying non-srcdir builds >-$(1): $$($(_gir_name)_FILES) $(INTROSPECTION_PARSER) >+$(1): $$($(_gir_name)_FILES) $(INTROSPECTION_SCANNER) bizarrely, although this works fine when building *other* packages, it breaks gobject-introspection's own build: Making all in gir make[2]: Entering directory `/home/danw/gshell/gobject-introspection/gir' Makefile:983: *** multiple target patterns. Stop. (where line 983 is $(foreach gir,$(INTROSPECTION_GIRS),$(eval $(call introspection-scanner,$(gir)))) ) There doesn't seem to be any way to ask gmake to expand the macros for you so you can figure out what it's complaining about though. If we just removed $(INTROSPECTION_PARSER) rather than fixing it to actually be $(INTROSPECTION_SCANNER), then the rest of the patch would be simplified (you wouldn't need the $(filter-out) call), and the functionality would still be the same as it was before, since "$(INTROSPECTION_PARSER)" is just ""...
Review of attachment 159278 [details] [review]: ::: Makefile.introspection @@ +115,3 @@ # sure these are built before running the scanner. Libraries and programs # needs to be added manually. +$(1): $$($(_gir_name)_FILES) $(INTROSPECTION_SCANNER) Oh, good catch, just a typo. @@ +126,3 @@ $($(_gir_name)_SCANNERFLAGS) \ $($(_gir_name)_CFLAGS) \ + $$(filter-out $$(INTROSPECTION_SCANNER), $$^) \ Hmm, it's a bit messy fixing this. Since we need to make a new release and then fix all introspection builds. Is there a way to archive backwards compatiblity? @@ -126,3 @@ $($(_gir_name)_SCANNERFLAGS) \ $($(_gir_name)_CFLAGS) \ - $($(_gir_name)_FILES) \ You can't just remove the xxx_FILES variable.
(In reply to comment #3) > You can't just remove the xxx_FILES variable. I'm not: +$(1): $$($(_gir_name)_FILES) $(INTROSPECTION_SCANNER) ... - $($(_gir_name)_FILES) \ + $$(filter-out $$(INTROSPECTION_SCANNER), $$^) \ "$^" expands to the rule's dependencies (except properly path-prefixed), and xxx_FILES is part of that. So the only thing this changes is that instead of outputting the contents of xxx_FILES literally, it now outputs it with the proper path prefixed to each element. > Hmm, it's a bit messy fixing this. Since we need to make a new release and then > fix all introspection builds. Is there a way to archive backwards compatiblity? It should already be compatible: if you were previously manually-prefixing the elements of xxx_FILES, then "$^" will pass them through unchanged, so you'll get exactly the same behavior as with the old macro.
(In reply to comment #2) > >+$(1): $$($(_gir_name)_FILES) $(INTROSPECTION_SCANNER) > > bizarrely, although this works fine when building *other* packages, it breaks > gobject-introspection's own build: > > Making all in gir > make[2]: Entering directory `/home/danw/gshell/gobject-introspection/gir' > Makefile:983: *** multiple target patterns. Stop. OK, the reason for this is in the gobject-introspection build, $(INTROSPECTION_SCANNER) is: env LPATH=.libs env PYTHONPATH=..:.. UNINSTALLED_INTROSPECTION_SRCDIR=.. UNINSTALLED_INTROSPECTION_BUILDDIR=.. ../tools/g-ir-scanner and so it parses the ":" in PYTHONPATH as being a second target-vs-dependency separator. We could fix this by adding a new variable, $(INTROSPECTION_SCANNER_DEPENDENCY), which would normally be set to $(INTROSPECTION_SCANNER), but in g-i's own build, common.mk would set it to $(top_builddir)/tools/g-ir-scanner, and then we'd use that rather than $(INTROSPECTION_SCANNER) as the dep for .gir files. But that STILL wouldn't be useful, because g-ir-scanner is just a wrapper script that only depends on g-ir-scanner.in, and so it wouldn't have the presumably-intended effect, which is that the girs get rebuilt if the scanner's internal logic changes. Given that, and given the fact that *it has never actually worked correctly up until this point* (because it previously incorrectly used $(INTROSPECTION_PARSER), which was ""), and given that we don't have dependencies on the "compiler" for other file types (eg, "foo.o: foo.c /usr/bin/gcc"), I think we should just remove the dependency on the scanner.
Created attachment 161547 [details] [review] Makefile.introspection: use $^, simplifying non-srcdir builds In the .gir-building rule, use "$^" to refer to the source files, since that automatically looks in both $(srcdir) and $(builddir). This is particularly important since certain generated files will be in $(builddir) when building from git, but in $(srcdir) when building from tarballs If you were previously prefixing $(srcdir) to the Foo_gir_FILES members by hand, you should stop now. (Also, removed the dependencies on $(INTROSPECTION_SCANNER) and $(INTROSPECTION_COMPILER) for the .gir/.typelib rules, since the scanner one was broken anyway, and we don't have that kind of dependency for other rules (eg, making .o files depend on /usr/bin/gcc).)
Created attachment 161549 [details] [review] [gir] remove unnecessary $(srcdir)s from Makefile.am
Review of attachment 161547 [details] [review]: Oh, I couldn't quite figure out what $$^ meant in the manual. Yes, this looks good. Would be great if you could check it with a few modules that uses it, such as pango and gtk+.
Review of attachment 161549 [details] [review]: Looks good.
(In reply to comment #5) > > But that STILL wouldn't be useful, because g-ir-scanner is just a wrapper > script that only depends on g-ir-scanner.in, and so it wouldn't have the > presumably-intended effect, which is that the girs get rebuilt if the scanner's > internal logic changes. But it did work (AIUI) for the "make install" case because the timestamps on both files got updated. It was useful during the development of introspection, but I agree it's fine to drop the dependency now.
The following fixes have been pushed: 0597fc0 remove unnecessary $(srcdir)s from Makefile.am 3e0b443 Makefile.introspection: use $^, simplifying non-srcdir builds
Created attachment 161553 [details] [review] remove unnecessary $(srcdir)s from Makefile.am
Created attachment 161554 [details] [review] Makefile.introspection: use $^, simplifying non-srcdir builds In the .gir-building rule, use "$^" to refer to the source files, since that automatically looks in both $(srcdir) and $(builddir). This is particularly important since certain generated files will be in $(builddir) when building from git, but in $(srcdir) when building from tarballs If you were previously prefixing $(srcdir) to the Foo_gir_FILES members by hand, you should stop now. (Also, removed the dependencies on $(INTROSPECTION_SCANNER) and $(INTROSPECTION_COMPILER) for the .gir/.typelib rules, since the scanner one was broken anyway, and we don't have that kind of dependency for other rules (eg, making .o files depend on /usr/bin/gcc).)
(In reply to comment #8) > Would be great if you could > check it with a few modules that uses it, such as pango and gtk+. OK, tested with gtk+. The existing state was: - srcdir == builddir build from git works - srcdir != builddir build from git fails ("No rule to make target `../../gdk-pixbuf/gdk-pixbuf-enum-types.h'") - srcdir == builddir build from tarball works - srcdir != builddir build from tarball works And using the new g-i without changing gtk+ doesn't change that. After removing the $(addprefix $(srcdir), ...) calls and using new g-i, all four cases work. Of course, if you remove the $(addprefix $(srcdir), ...) calls from gtk+ but then use the *old* g-i, then *both* of the srcdir!=builddir cases are broken. So, when people get rid of their $(addprefix) calls, they need to make sure to require an up-to-date g-i at the same time.
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]