GNOME Bugzilla – Bug 609205
Modernize autotools configuration
Last modified: 2010-02-23 15:52:04 UTC
See http://live.gnome.org/GnomeGoals/ModernAutotools
Created attachment 153176 [details] [review] Modernize autotools configuration New requirements: autoconf >= 2.63.2 libtool >= 2.2.6 intltool >= 0.40.0 pkg-config >= 0.22 All these packages are available in Debian stable
Created attachment 153177 [details] [review] Modernize autotools configuration.v2 Sorry, I forgot to add ACLOCAL_AMFLAGS to Makefile.am
Review of attachment 153177 [details] [review]: Is this fixing any specific bug? I guess it's OK to commit (after fixing one comment below) even if not, looks like we have all of this in F11 at least. ::: configure.ac @@ +4,2 @@ +AC_CONFIG_SRCDIR([src/shell-drawing.c]) +AC_CONFIG_HEADERS([config.h]) This should use src/shell-global.c
Created attachment 153571 [details] [review] Modernize autotools configuration.v2 Relaxed the autoconf requirement to 2.63 and change the AC_CONFIG_SRCDIR var
Review of attachment 153571 [details] [review]: ok
Comment on attachment 153571 [details] [review] Modernize autotools configuration.v2 commit d62b4cf130abca975f16be987c51279037020a50
Thank you Colin, My first gnome-shell contribution :)
Review of attachment 153571 [details] [review]: A of useless stuff was added to the configure.in in this patch. There is zero point in adding AC_CHECK_FUNCS for stuff we don't have fallbacks for in the code. And there is zero point in having fallbacks for a C89 function like memset! I think it's really important to keep configure.ac only having stuff that makes sense and is understood and needed and not stuff that got added because it was copied from somewhere else, or because some automated script suggested it. Otherwise, repeated cut-and-paste results in configure.ac files that are multiple times longer than needed and are completely unmaintainable. ::: configure.ac @@ +4,2 @@ +AC_CONFIG_HEADERS([config.h]) +AC_CONFIG_SRCDIR([src/shell-global.c]) IMO, not worth cluttering up the configure.ac, but OK @@ +10,2 @@ AM_MAINTAINER_MODE +m4_ifdef([AM_SILENT_RULES],[AM_SILENT_RULES([yes])]) Spacing is wrong here, the last line isn't related to the two before it @@ +21,3 @@ + +# Checks for header files. +AC_PATH_X This should not have been added @@ +22,3 @@ +# Checks for header files. +AC_PATH_X +AC_CHECK_HEADERS([fcntl.h float.h libintl.h paths.h stdlib.h string.h unistd.h]) These should not have been added @@ +27,3 @@ +AC_C_INLINE +AC_TYPE_SIZE_T +AC_TYPE_UID_T None of this should have been added @@ +31,2 @@ +# Checks for library functions. +AC_CHECK_FUNCS([floor getusershell memset strchr strdup strrchr strstr strtol strtoul]) Nor this @@ +35,3 @@ +IT_PROG_INTLTOOL([0.40.6]) +AC_SUBST([GETTEXT_PACKAGE], [gnome-shell]) +AM_GNU_GETTEXT_VERSION([0.17]) It doesn't make any sense to me to leave AM_GLIB_GNU_GETTEXT here, then also add this. AM_GLIB_GNU_GETTEXT definitely will already be checking for intltool. @@ -95,4 +110,3 @@ # minimum/yes/maximum are the same, however. AC_ARG_ENABLE(compile_warnings, - AC_HELP_STRING([--enable-compile-warnings=@<:@no/minimum/yes/maximum/error@:>@], - [Turn on compiler warnings]),, + AS_HELP_STRING([--enable-compile-warnings=@<:@no/minimum/yes/maximum/error@:>@],[Turn on compiler warnings]),, Don't know why this was rewrapped, but not worth fixing
Created attachment 153686 [details] [review] Clean configure.ac Hello Owen, yeah, you catch me ;), I've used autoscan and autoupdate to make the patch (but some manual work too). Here a new patch with your comments. About the gettext support, maybe I misunderstood your comment but I read here: http://bazaar.launchpad.net/~intltool/intltool/trunk/annotate/head:/doc/I18N-HOWTO#L97 that at least a call to AM_GLIB_GNU_GETTEXT or AM_GNU_GETTEXT is required
(In reply to comment #9) > About the gettext support, maybe I misunderstood your comment but I read here: > http://bazaar.launchpad.net/~intltool/intltool/trunk/annotate/head:/doc/I18N-HOWTO#L97 > that at least a call to AM_GLIB_GNU_GETTEXT or AM_GNU_GETTEXT is required What was there before: === GETTEXT_PACKAGE=gnome-shell IT_PROG_INTLTOOL(0.26) AM_GLIB_GNU_GETTEXT === What you replaced it with: === # i18n IT_PROG_INTLTOOL([0.40.6]) AC_SUBST([GETTEXT_PACKAGE], [gnome-shell]) AM_GNU_GETTEXT_VERSION([0.17]) AM_GLIB_GNU_GETTEXT AC_DEFINE([GETTEXT_PACKAGE], [PACKAGE_TARNAME], [The prefix for our gettext translation domains.]) === Can you change it back to what it was before? You'll need to also change back the Makefile.am change in 11656ebd, since that was dependent on the AC_DEFINE that you added. (The AC_DEFINE there isn't wrong, but it's longer and more complex than what was there before, and I don't see a reason to switch over to doing it that way.)
Review of attachment 153686 [details] [review]: Marking patch as needs-work
I pushed a cleanup fix based on your patch yesterday, unfortunately I missed a few lines for the gettext portion and broke .mo file installation (bug 610787), that should be fixed now as well.