GNOME Bugzilla – Bug 741288
Build System Improvements
Last modified: 2015-03-02 13:58:43 UTC
These are a series of small updates/improvements.
Created attachment 292353 [details] [review] Enable maintainer mode This seems to be recommended. http://blogs.gnome.org/desrt/2011/09/08/am_maintainer_mode-is-not-cool/
Created attachment 292354 [details] [review] Remove GNOME_MAINTAINER_MODE_DEFINES This is deprecated and is not needed.
Created attachment 292355 [details] [review] Move GNOME_DEBUG_CHECK This stops the warning AC_PROG_CC was called before AX_CHECK_ENABLE_DEBUG
Created attachment 292356 [details] [review] Use a build-aux directory This saves cluttering the root directory with so many files.
Created attachment 292357 [details] [review] Add release.mk This adds some sanity checks to make release and automates uploading and tagging a release.
Created attachment 292358 [details] [review] Update git.mk This new version ignores the generated help files.
Created attachment 292359 [details] [review] Update MAINTAINERCLEANFILES The new git.mk defines some variables to be used in MAINTAINERCLEANFILES.
Created attachment 292360 [details] [review] Stop tracking INSTALL INSTALL is managed by automake, tracking it makes git think it needs adding every time automake updates it.
Created attachment 292361 [details] [review] Use non recursive make Non recursive make is more efficient. This will make it simpler to use vala as we wont need to create a separate vapi file for the code in each subdirectory. Libtool is no longer used for the build (libjuicer needs some work to use opaque data structures and have its own translation domain before it can be installed as a shared library). Not using libtool means that the object file dependencies of tests need to be manually added but has the benefit that are only rebuilt if their specific dependencies have changed which means one can lazily rerun only the updated tests with env RECHECK_LOGS= make -e check Finally sound-juicer is now built in the root directory, data file paths have been updated to allow uninstalled running from that directory.
Review of attachment 292355 [details] [review]: ::: configure.ac @@ +11,3 @@ m4_ifdef([AM_SILENT_RULES],[AM_SILENT_RULES([yes])]) +GNOME_DEBUG_CHECK This is defined in /usr/share/aclocal/gnome-common.m4 and is marked as deprecated in favour of AX_CHECK_ENABLE_DEBUG, might be worth cleaning that up while you are at it.
Review of attachment 292357 [details] [review]: Maybe this (and git.mk) should go to some subdir too (build-aux ?).
Series looks good (I did not look in detail to the release.mk changes)
(In reply to comment #10) > Review of attachment 292355 [details] [review]: > > ::: configure.ac > @@ +11,3 @@ > m4_ifdef([AM_SILENT_RULES],[AM_SILENT_RULES([yes])]) > > +GNOME_DEBUG_CHECK > > This is defined in /usr/share/aclocal/gnome-common.m4 and is marked as > deprecated in favour of AX_CHECK_ENABLE_DEBUG, might be worth cleaning that up > while you are at it. I wondered about doing that but after reading the updates to https://tecnocode.co.uk/2014/09/17/long-live-gnome-common-macro-deprecation/ I decided to wait until there was a consensus on the best way forward as I'm not that keen on having to keep stuff in /m4 up to date manually.
(In reply to comment #11) > Review of attachment 292357 [details] [review]: > > Maybe this (and git.mk) should go to some subdir too (build-aux ?). Yes, that's a good idea.
(In reply to comment #12) > Series looks good (I did not look in detail to the release.mk changes) I've tested the release-check rules fairly carefully, it might be worth looking at the comments describing the checks to make sure you think the checks are a good idea.
(In reply to comment #13) > (In reply to comment #10) > > Review of attachment 292355 [details] [review] [details]: > > > > ::: configure.ac > > @@ +11,3 @@ > > m4_ifdef([AM_SILENT_RULES],[AM_SILENT_RULES([yes])]) > > > > +GNOME_DEBUG_CHECK > > > > This is defined in /usr/share/aclocal/gnome-common.m4 and is marked as > > deprecated in favour of AX_CHECK_ENABLE_DEBUG, might be worth cleaning that up > > while you are at it. > > I wondered about doing that but after reading the updates to > https://tecnocode.co.uk/2014/09/17/long-live-gnome-common-macro-deprecation/ I > decided to wait until there was a consensus on the best way forward as I'm not > that keen on having to keep stuff in /m4 up to date manually. gnome-common installs ax_check_enable_debug.m4 at the moment so the burden of updating it falls on gnome-common, but keeping GNOME_DEBUG_CHECK until the situation is clearer (if ever ;) does not hurt.
Review of attachment 292357 [details] [review]: Checks err on the strict side, but fine with me. They can always be adjusted if they prove to be not enouh or too annoying. ::: release.mk @@ +23,3 @@ +release: release-check distcheck + @branch=$$(git branch --no-color --no-column | grep '^\*'); \ + branch="$${branch#* }"; \ This looks like git symbolic-ref HEAD @@ +41,3 @@ + @echo "Running pre-release checks"; \ + branch=$$(git branch --no-color --no-column | grep '^\*'); \ + branch="$${branch#* }"; \ ditto, git symbolic-ref (probably with the --short option) @@ +61,3 @@ + { echo "Error: Last commit should be 'Release $(VERSION)'"; \ + git log -n1 --oneline; exit 1; }; \ + x=$$(git log --format=%H\ %s --grep $(VERSION) -F HEAD^ | \ git log --oneline --grep ... would be very similar. Since you force the format of the Release commit, why not grep grep on that in the --grep argument? You want to catch older release commits not following that format?
(In reply to comment #17) > Review of attachment 292357 [details] [review]: > > Checks err on the strict side, but fine with me. They can always be adjusted if > they prove to be not enouh or too annoying. > > ::: release.mk > @@ +23,3 @@ > +release: release-check distcheck > + @branch=$$(git branch --no-color --no-column | grep '^\*'); \ > + branch="$${branch#* }"; \ > > This looks like git symbolic-ref HEAD > > @@ +41,3 @@ > + @echo "Running pre-release checks"; \ > + branch=$$(git branch --no-color --no-column | grep '^\*'); \ > + branch="$${branch#* }"; \ > > ditto, git symbolic-ref (probably with the --short option) Thanks, I thought there should be a better way of doing it but I didn't know about symbolic-ref > @@ +61,3 @@ > + { echo "Error: Last commit should be 'Release $(VERSION)'"; \ > + git log -n1 --oneline; exit 1; }; \ > + x=$$(git log --format=%H\ %s --grep $(VERSION) -F HEAD^ | \ > > git log --oneline --grep ... would be very similar. Since you force the format > of the Release commit, why not grep grep on that in the --grep argument? You > want to catch older release commits not following that format? git log --grep seems to grep the whole commit message irrespective of the format. For instance the first three matches for 'git log --oneline --grep gst' are 004a2d9 Fix memory leak in rb_gst_media_type_matches_profile 7d725ce Remove unused helper e260e81 Remove taglib based id3mux plugin (Closes: #522411) So the second grep makes sure we just check the subject line of the commit. I thought it would be more efficient to limit the number of commits piped to grep by filtering git log with --grep. The idea of this check is to check that there are no other release commits with the current $(VERSION). I'm not sure it's really necessary as the check to make sure the $(VERSION) tag doesn't exist should be enough, so maybe I should just remove it - what do you think?
(In reply to comment #18) > > > @@ +61,3 @@ > > + { echo "Error: Last commit should be 'Release $(VERSION)'"; \ > > + git log -n1 --oneline; exit 1; }; \ > > + x=$$(git log --format=%H\ %s --grep $(VERSION) -F HEAD^ | \ > > > > git log --oneline --grep ... would be very similar. Since you force the format > > of the Release commit, why not grep grep on that in the --grep argument? You > > want to catch older release commits not following that format? > > git log --grep seems to grep the whole commit message irrespective of the > format. For instance the first three matches for 'git log --oneline --grep gst' > are > 004a2d9 Fix memory leak in rb_gst_media_type_matches_profile > 7d725ce Remove unused helper > e260e81 Remove taglib based id3mux plugin (Closes: #522411) > So the second grep makes sure we just check the subject line of the commit. I > thought it would be more efficient to limit the number of commits piped to grep > by filtering git log with --grep. Ah right, --grep is not narrow enough. I don't really mind about the grepping though, my main gripe was --format VS --oneline (and I don't care _that much_, either are fine for me ;) > > The idea of this check is to check that there are no other release commits with > the current $(VERSION). I'm not sure it's really necessary as the check to make > sure the $(VERSION) tag doesn't exist should be enough, so maybe I should just > remove it - what do you think? It does not hurt to check for both imo, if you ever get in an inconsistent state because of network failures, script failures, ... it's nice to have something which can detect and warn about it.
Created attachment 294410 [details] [review] Use a build-aux directory Updated to move git.mk into build-aux/ otherwise unchanged. This saves cluttering the root directory with so many files.
Created attachment 294412 [details] [review] Add release.mk I've updated this to use 'git symbolic-ref' to get the current branch name and 'git log --oneline' rather than 'git log --format' as you suggested. I have also improved the branch checking so that it correctly handles a.b.0.d releases with a gnome-a-b branch and added a basic check that the version number has at least three parts as the branch logic assumes that it does. This adds some sanity checks to make release and automates uploading and tagging a release.
Created attachment 294413 [details] [review] Update git.mk Updated to be in build-aux/ rather than / This new version ignores the generated help files.
Created attachment 294414 [details] [review] Use non recursive make Updated to fix a typo the path for the uninstalled profiles, otherwise unchanged Non recursive make is more efficient. This will make it simpler to use vala as we wont need to create a separate vapi file for the code in each subdirectory. Libtool is no longer used for the build (libjuicer needs some work to use opaque data structures and have its own translation domain before it can be installed as a shared library). Not using libtool means that the object file dependencies of tests need to be manually added but has the benefit that are only rebuilt if their specific dependencies have changed which means one can lazily rerun only the updated tests with env RECHECK_LOGS= make -e check Finally sound-juicer is now built in the root directory, data file paths have been updated to allow uninstalled running from that directory.
Created attachment 295431 [details] [review] Use non recursive make I've updated this to include data/ as well, otherwise it's unchanged. Non recursive make is more efficient. This will make it simpler to use vala as we wont need to create a separate vapi file for the code in each subdirectory. Libtool is no longer used for the build (libjuicer needs some work to use opaque data structures and have its own translation domain before it can be installed as a shared library). Not using libtool means that the object file dependencies of tests need to be manually added but has the benefit that are only rebuilt if their specific dependencies have changed which means one can lazily rerun only the updated tests with env RECHECK_LOGS= make -e check Finally sound-juicer is now built in the root directory, data file paths have been updated to allow uninstalled running from that directory.
Created attachment 297012 [details] [review] Use non recursive make Updated to apply cleanly to master. Non recursive make is more efficient. This will make it simpler to use vala as we wont need to create a separate vapi file for the code in each subdirectory. Libtool is no longer used for the build (libjuicer needs some work to use opaque data structures and have its own translation domain before it can be installed as a shared library). Not using libtool means that the object file dependencies of tests need to be manually added but has the benefit that are only rebuilt if their specific dependencies have changed which means one can lazily rerun only the updated tests with env RECHECK_LOGS= make -e check Finally sound-juicer is now built in the root directory, data file paths have been updated to allow uninstalled running from that directory.
Christophe - are you happy with these patches now?, it would be good to get them merged for 3.16 if they're ok
Review of attachment 294410 [details] [review]: Missing 'git mv git.mk build-aux/git.mk' in that commit? Or is it part of another commit? (sorry, I don't have a tree with all the patches applied, makes checking more complicated). If this goes before the non-recursive make patch, data/Makefile.am and help/Makefile.am both reference git.mk too.
Review of attachment 294412 [details] [review]: ::: build-aux/release.mk @@ +28,3 @@ + git push origin tag '$(VERSION)' + scp '$(PACKAGE)-$(VERSION).tar.xz master.gnome.org:' + ssh master.gnome.org ftpadmin install '$(PACKAGE)-$(VERSION).tar.xz' Given that if you are running a ssh agent, the push/scp/ssh will be all automatic, I'd tend to ask y/n questions to make sure everything is fine before anything gets uploaded. Can be added later though if ever ;)
Review of attachment 297012 [details] [review]: Fine with me, I assume make distcheck is still working after these changes? ::: libjuicer/rb-gst-media-types.c @@ +35,3 @@ #include "rb-gst-media-types.h" +#define SOURCE_ENCODING_TARGET_FILE "data/rhythmbox.gep" Fwiw, I think this is broken with builddir!=srcdir, but this was already broken before this change anyway. ::: src/Makefile.am @@ +46,3 @@ $(GSTREAMER_LIBS) \ $(BURN_LIBS) \ $(UI_LIBS) these CPPFLAGS/CFLAGS/LDADD are needed both in libjuicer/ and src/, it might be better to directly set them in the toplevel rather than relying in the order the include are done in the main Makefile ?
Created attachment 297359 [details] [review] Use a build-aux directory In reply to comment 27 >Review of attachment 294410 [details] [review] [review]: > >Missing 'git mv git.mk build-aux/git.mk' in that commit? Or is it part >of another commit? (sorry, I don't have a tree with all the patches >applied, makes checking more complicated). The move is in the header but git-bz uses 'git format-patch -M' and splinter doesn't show the move as there are no diff hunks in the patch (bug 744795). I'll push a branch wip/non-recursive-make with all these patches in it. >If this goes before the non-recursive make patch, data/Makefile.am and >help/Makefile.am both reference git.mk too. Good call, I've updated the patch to do this and also I had also forgotten to change git.mk to reflect it's new location as well. I've tested the new patch on a clean tree and it works properly. This saves cluttering the root directory with so many files.
Created attachment 297360 [details] [review] Add release.mk In reply to comment 28 >Review of attachment 294412 [details] [review] [review]: > >::: build-aux/release.mk >@@ +28,3 @@ >+ git push origin tag '$(VERSION)' >+ scp '$(PACKAGE)-$(VERSION).tar.xz master.gnome.org:' >+ ssh master.gnome.org ftpadmin install '$(PACKAGE)-$(VERSION).tar.xz' > >Given that if you are running a ssh agent, the push/scp/ssh will be >all automatic, I'd tend to ask y/n questions to make sure everything >is fine before anything gets uploaded. Can be added later though if >ever ;) That's a good idea, I've updated it to show what would be pushed and ask for confirmation and also to ask if it's ok to upload the tarball. This adds some sanity checks to make release and automates uploading and tagging a release.
Created attachment 297361 [details] [review] Use non recursive make In reply to comment 29 >Review of attachment 297012 [details] [review] [review]: > >Fine with me, I assume make distcheck is still working after these >changes? Yes it does (so long as you don't use automake 1.15 but that's due to a bug in intltool - https://bugs.launchpad.net/intltool/+bug/1117944) >:: libjuicer/rb-gst-media-types.c >@ +35,3 @@ >#include "rb-gst-media-types.h" > >#define SOURCE_ENCODING_TARGET_FILE "data/rhythmbox.gep" > >Fwiw, I think this is broken with builddir!=srcdir, but this was >already broken before this change anyway. Yes I tested it and it is broken, I've posted a possible solution to bug 744845. >::: src/Makefile.am >@@ +46,3 @@ > $(GSTREAMER_LIBS) \ > $(BURN_LIBS) \ > $(UI_LIBS) > >these CPPFLAGS/CFLAGS/LDADD are needed both in libjuicer/ and src/, it >might be better to directly set them in the toplevel rather than >relying in the order the include are done in the main Makefile ? I've moved most of the flags into the top level Makefile as they're used by everything. Non recursive make is more efficient. This will make it simpler to use vala as we wont need to create a separate vapi file for the code in each subdirectory. Libtool is no longer used for the build (libjuicer needs some work to use opaque data structures and have its own translation domain before it can be installed as a shared library). Not using libtool means that the object file dependencies of tests need to be manually added but has the benefit that are only rebuilt if their specific dependencies have changed which means one can lazily rerun only the updated tests with env RECHECK_LOGS= make -e check Finally sound-juicer is now built in the root directory, data file paths have been updated to allow uninstalled running from that directory.
Attachment 292353 [details] pushed as fe7fe61 - Enable maintainer mode Attachment 292354 [details] pushed as 234b0ef - Remove GNOME_MAINTAINER_MODE_DEFINES Attachment 292355 [details] pushed as 6d30c02 - Move GNOME_DEBUG_CHECK Attachment 292359 [details] pushed as fbd907e - Update MAINTAINERCLEANFILES Attachment 292360 [details] pushed as d83d78e - Stop tracking INSTALL Attachment 294413 [details] pushed as 6ea3fe8 - Update git.mk Attachment 297359 [details] pushed as 098034a - Use a build-aux directory Attachment 297360 [details] pushed as f434b73 - Add release.mk Attachment 297361 [details] pushed as 1d4e922 - Use non recursive make