GNOME Bugzilla – Bug 744309
remove use of deprecated gnome-common
Last modified: 2015-02-12 16:46:23 UTC
See: https://wiki.gnome.org/Projects/GnomeCommon/Migration We are currently using : GNOME_DEBUG_CHECK GNOME_COMPILE_WARNINGS GNOME_MAINTAINER_MODE_DEFINES These must be removed.
alongwith other migrations as mentioned in the link in above comment
Created attachment 296565 [details] [review] autogen.sh: ported away from gnome-autogen.sh See: https://wiki.gnome.org/Projects/GnomeCommon/Migration
Created attachment 296567 [details] [review] Use AX_REQUIRE_DEFINED to check for m4 macros
Created attachment 296568 [details] [review] Use AX_IS_RELEASE
Created attachment 296569 [details] [review] Use AX_CHECK_ENABLE_DEBUG ... instead of deprecated GNOME_DEBUG_CHECK.
Created attachment 296570 [details] [review] Remove obsolete GNOME_MAINTAINER_MODE_DEFINES
Created attachment 296571 [details] [review] Use AX_COMPILER_FLAGS
(In reply to Pranav Kant from comment #2) > From 0a171fb7a495c847be59888f4d817ffa85849e19 Mon Sep 17 00:00:00 2001 > From: Pranav Kant <pranav913@gmail.com> > Date: Wed, 11 Feb 2015 11:35:06 +0530 > Subject: [PATCH] autogen.sh: ported away from gnome-autogen.sh Nit pick. It should be "build: Port away from ..." We have been mostly using the "build: " prefix for build system related things, and "Port away ..." because it matches with "Revert ..." when you do a "git revert". > See: https://wiki.gnome.org/Projects/GnomeCommon/Migration > > https://bugzilla.gnome.org/show_bug.cgi?id=744309 > --- > autogen.sh | 50 +++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 35 insertions(+), 15 deletions(-) > > diff --git a/autogen.sh b/autogen.sh > index aac383a..b4e0d5f 100755 > --- a/autogen.sh > +++ b/autogen.sh > @@ -4,24 +4,44 @@ > srcdir=`dirname $0` > test -z "$srcdir" && srcdir=. Otherwise looks good. Please commit after adjusting the subject.
(In reply to Pranav Kant from comment #3) > From 446230123adc491e6f58f57c0ec30e9004627dbe Mon Sep 17 00:00:00 2001 > From: Pranav Kant <pranav913@gmail.com> > Date: Wed, 11 Feb 2015 12:03:22 +0530 > Subject: [PATCH] Use AX_REQUIRE_DEFINED to check for m4 macros Nitpick. It should be "build: Use ..." > https://bugzilla.gnome.org/show_bug.cgi?id=744309 > --- > configure.ac | 4 ++++ > m4/ax_require_defined.m4 | 37 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 41 insertions(+) > create mode 100644 m4/ax_require_defined.m4 > > diff --git a/configure.ac b/configure.ac > index 9b79a12..6740925 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -39,6 +39,7 @@ GNOME_COMPILE_WARNINGS([maximum]) > GNOME_DEBUG_CHECK > GNOME_MAINTAINER_MODE_DEFINES > > +AX_REQUIRE_DEFINED([IT_PROG_INTLTOOL]) > IT_PROG_INTLTOOL([0.50.1]) Otherwise looks good. Please commit after adjusting the subject.
(In reply to Pranav Kant from comment #4) > From a4eb76ccf3b9bf9faefca71c1da39fdb6da1e672 Mon Sep 17 00:00:00 2001 > From: Pranav Kant <pranav913@gmail.com> > Date: Wed, 11 Feb 2015 12:22:49 +0530 > Subject: [PATCH] Use AX_IS_RELEASE Nitpick. It should be "build: Use ..." > https://bugzilla.gnome.org/show_bug.cgi?id=744309 > --- > configure.ac | 2 ++ > m4/ax_is_release.m4 | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 62 insertions(+) > create mode 100644 m4/ax_is_release.m4 > > diff --git a/configure.ac b/configure.ac > index 6740925..4478829 100644 > --- a/configure.ac > +++ b/configure.ac Otherwise looks good. Please commit after adjusting the subject.
Created attachment 296577 [details] [review] build: Port away from gnome-autogen.sh See: https://wiki.gnome.org/Projects/GnomeCommon/Migration
Created attachment 296578 [details] [review] build: Use AX_REQUIRE_DEFINED to check for m4 macros
Created attachment 296579 [details] [review] build: Use AX_IS_RELEASE
Created attachment 296580 [details] [review] build: Use AX_CHECK_ENABLE_DEBUG ... instead of deprecated GNOME_DEBUG_CHECK.
Created attachment 296581 [details] [review] build: Remove obsolete GNOME_MAINTAINER_MODE_DEFINES
Created attachment 296583 [details] [review] build: Use AX_COMPILER_FLAGS
Review of attachment 296577 [details] [review]: Perfect.
Review of attachment 296578 [details] [review]: Perfect. Thanks, Pranav.
Review of attachment 296579 [details] [review]: Looks good.
Review of attachment 296580 [details] [review]: ::: configure.ac @@ +19,3 @@ +AX_CHECK_ENABLE_DEBUG(,,,[$ax_is_release]) + https://wiki.gnome.org/Projects/GnomeCommon/Migration says that if you use this form then you also need ENABLE_DEBUG in Makefile.am. Since we were not using GNOME_ENABLE_DEBUG in Makefile.am, it would be better to just use the other form. ie. AX_CHECK_ENABLE_DEBUG([no],[GNOME_ENABLE_DEBUG],,[$ax_is_release])
Review of attachment 296581 [details] [review]: Thanks, Pranav.
(In reply to Debarshi Ray from comment #20) > Review of attachment 296580 [details] [review] [review]: > > ::: configure.ac > @@ +19,3 @@ > > +AX_CHECK_ENABLE_DEBUG(,,,[$ax_is_release]) > + > > https://wiki.gnome.org/Projects/GnomeCommon/Migration says that if you use > this form then you also need ENABLE_DEBUG in Makefile.am. Since we were not > using GNOME_ENABLE_DEBUG in Makefile.am, it would be better to just use the > other form. ie. > AX_CHECK_ENABLE_DEBUG([no],[GNOME_ENABLE_DEBUG],,[$ax_is_release]) We don't use neither GNOME_ENABLE_DEBUG nor ENABLE_DEBUG in our Makefiles because we don't have that kind of debug setup in place. I guess the link : * by second option means to change from GNOME_ENABLE_DEBUG to ENABLE_DEBUG in makefiles for those projects which have been using GNOME_ENABLE_DEBUG in their makefiles or * by first option they can continue to use GNOME_ENABLE_DEBUG in their makefiles by defining it in AX_CHECK_ENABLE_DEBUG macro.
Review of attachment 296583 [details] [review]: ::: configure.ac @@ +20,3 @@ AX_CHECK_ENABLE_DEBUG(,,,[$ax_is_release]) +AX_COMPILER_FLAGS([], [], [$ax_is_release]) Umm... aren't you missing the names of the variables? Shouldn't this be: AX_COMPILER_FLAGS([WARN_CFLAGS],[WARN_LDFLAGS],[$ax_is_release]) ? Nitpick. I think we should just club this with the AX_CHECK_ENABLE_DEBUG. Both work with ax_is_release, so no need to add an extra newline.
(In reply to Debarshi Ray from comment #23) > Review of attachment 296583 [details] [review] [review]: > > ::: configure.ac > @@ +20,3 @@ > AX_CHECK_ENABLE_DEBUG(,,,[$ax_is_release]) > > +AX_COMPILER_FLAGS([], [], [$ax_is_release]) > > Umm... aren't you missing the names of the variables? Shouldn't this be: > AX_COMPILER_FLAGS([WARN_CFLAGS],[WARN_LDFLAGS],[$ax_is_release]) ? > > Nitpick. I think we should just club this with the AX_CHECK_ENABLE_DEBUG. > Both work with ax_is_release, so no need to add an extra newline. From http://www.gnu.org/software/autoconf-archive/ax_compiler_flags.html "CFLAGS-VARIABLE defaults to WARN_CFLAGS, and LDFLAGS-VARIABLE defaults to WARN_LDFLAGS. Both variables are AC_SUBST-ed by this macro, but must be manually added to the CFLAGS and LDFLAGS variables for each target in the code base. "
(In reply to Pranav Kant from comment #24) > From http://www.gnu.org/software/autoconf-archive/ax_compiler_flags.html > > "CFLAGS-VARIABLE defaults to WARN_CFLAGS, and LDFLAGS-VARIABLE defaults to > WARN_LDFLAGS. Both variables are AC_SUBST-ed by this macro, but must be > manually added to the CFLAGS and LDFLAGS variables for each target in the > code base. " Ok, then it is fine.
(In reply to Debarshi Ray from comment #23) > Review of attachment 296583 [details] [review] [review]: > > ::: configure.ac > @@ +20,3 @@ > AX_CHECK_ENABLE_DEBUG(,,,[$ax_is_release]) > > +AX_COMPILER_FLAGS([], [], [$ax_is_release]) > > Umm... aren't you missing the names of the variables? Shouldn't this be: > AX_COMPILER_FLAGS([WARN_CFLAGS],[WARN_LDFLAGS],[$ax_is_release]) ? > > Nitpick. I think we should just club this with the AX_CHECK_ENABLE_DEBUG. > Both work with ax_is_release, so no need to add an extra newline. From IRC: 18:00 < pwithnall> Note: wrt comment #23, I would recommend “AX_COMPILER_FLAGS([WARN_CFLAGS],[WARN_LDFLAGS],[$ax_is_release])” because then grepping your code base for WARN_CFLAGS will lead you to the definition 18:00 < pwithnall> otherwise it apparently appears from nowhere, magically Makes sense. Lets follow this advice.
(In reply to Debarshi Ray from comment #20) > Review of attachment 296580 [details] [review] [review]: > > ::: configure.ac > @@ +19,3 @@ > > +AX_CHECK_ENABLE_DEBUG(,,,[$ax_is_release]) > + > > https://wiki.gnome.org/Projects/GnomeCommon/Migration says that if you use > this form then you also need ENABLE_DEBUG in Makefile.am. Since we were not > using GNOME_ENABLE_DEBUG in Makefile.am, it would be better to just use the > other form. ie. > AX_CHECK_ENABLE_DEBUG([no],[GNOME_ENABLE_DEBUG],,[$ax_is_release]) From IRC : 17:57 < pwithnall> pranavk: The second argument to AX_CHECK_ENABLE_DEBUG specifies the name of the variable which is defined as the macro’s output, for you to use in your Makefile.ams 17:58 < pwithnall> *in your code, sorry 17:58 < pwithnall> If you’re not currently using ENABLE_DEBUG or GNOME_ENABLE_DEBUG, it doesn’t matter what you specify in the second argument, so I’d leave it empty to use the default 17:58 < pwithnall> Leaving it empty means a comma, rather than ‘[],’ 17:59 < pwithnall> So I would go with “AX_CHECK_ENABLE_DEBUG(,,,[$ax_is_release])” ... 18:02 < pranavk> but why did you remove the [no] in second option. 18:03 < pwithnall> pranavk: Because the first option exactly reproduces what the old GNOME_DEBUG_CHECK macro did 18:03 < pwithnall> The second option is the ‘modern’ recommended behaviour, with debugging defaulting to being enabled
Created attachment 296590 [details] [review] build: Use AX_COMPILER_FLAGS After Comment 26
Review of attachment 296590 [details] [review]: Looks good.
Review of attachment 296580 [details] [review]: This is good to go then. I never knew what GNOME_DEBUG_CHECK did because it sneaked in from some other configure.ac when setting up the project. The joys of cargo cult programming.
Review of attachment 296590 [details] [review]: Unless gnome-photos already uses C++, this introduces a dependency on a C++ compiler, which is probably not desired. I will try to fix this upstream (in autoconf-archive) over the weekend, but it might take more time.
(In reply to David King from comment #31) > Review of attachment 296590 [details] [review] [review]: > > Unless gnome-photos already uses C++, this introduces a dependency on a C++ > compiler, which is probably not desired. I will try to fix this upstream (in > autoconf-archive) over the weekend, but it might take more time. Oh, that is unfortunate. I don't mind keep it in its current form for the time being until the C++ dependency can be solved. In the worst case we can revert the change before tagging the next release. Thanks for the heads up, David.
Created attachment 296690 [details] [review] build: Use AX_CHECK_ENABLED_DEBUG This patch includes the updated macro file from autoconf-archive. Old patch contained the outdated macro file.
Created attachment 296691 [details] [review] build: Use AX_COMPILER_FLAGS Follow up from previous comment.