GNOME Bugzilla – Bug 742548
configure.ac: stay out of autoconf's namespace
Last modified: 2017-11-07 17:13:21 UTC
Our self-declared cache variables should be prefixed with glib_ (as most of them already are) instead of ac_, which is reserved to autoconf.
Created attachment 294057 [details] [review] configure.ac: stay out of autoconf's namespace
Created attachment 294059 [details] [review] configure: assume alloca exists in alloca.h Assume that a usable alloca() macro is defined in <alloca.h>. There are not a lot of systems around these days for which that is untrue. Note that we no longer define GLIB_HAVE_ALLOCA_H in glibconfig.h anymore, which could be considered an API change. I don't consider that to have ever been a public API because it doesn't start with G_, and even if someone is using it then, by definition, it means that they have a fallback for the case where it's not defined.
Review of attachment 294059 [details] [review]: Actually, some research shows that there is no alloca.h under MSVC, and we must still use _alloca() from <malloc.h> (although apparently there is a new _malloca which is supposed to be better). Can someone confirm that?
Review of attachment 294057 [details] [review]: Lots of crufty code touched here... Do we really care about this enough to poke it? Does it fix an actual bug? ::: configure.ac @@ -762,3 @@ # check for bytesex stuff AC_C_BIGENDIAN -if test x$ac_cv_c_bigendian = xuniversal ; then Won't this break, since this variable is set by AC_C_BIGENDIAN, and you're changing it to glib? Basically I think you're going to need to more carefully audit this patch for cv's which are set by autoconf but read by glib.
Review of attachment 294057 [details] [review]: My motivation here is that I want to start using autoconf's site configuration caching in order to speed up builds, possibly even adding support for that to jhbuild. From the jhbuild standpoint, I don't think we can blindly reuse all cached values since a good chunk of what jhbuild does is to install things that change the results of various configure tests, so I want to make a filter for "safe" things. These abuses of the autoconf namespace would get in the way of that effort. ::: configure.ac @@ -767,3 @@ #error Not a big endian. #endif], - ac_cv_c_bigendian=yes Good catch. We set it here, which is why I assumed that we were the ones responsible for setting it. I did go to efforts to make sure that I didn't catch the "simply reading" cases, but clearly this one particular case will require a closer look.
Created attachment 294064 [details] [review] configure.ac: reject 'universal' builds AC_C_BIGENDIAN can return 'universal' as the result in the case that we are trying to do a universal build on Mac OS. This has to be opted into explicitly by using multiple -arch CFLAGS. Previously, we detected this result and fell back to doing our own check based on the endianness of the build machine, hardcoding that. This means that universal builds might successfully build, but the binaries would never actually run correctly on the 'opposite' arch. This check was added because of a bug in the intial implementation of this detection in autoconf, which was inappropriately identifying non-macos compilers as 'universal'. That was hitting ppc64 systems. See https://bugzilla.redhat.com/show_bug.cgi?id=449944 for more info. Commit b0e687ef42e21b1eb7af18c4eaebcd41b0bd5632 in autoconf ("Limit AC_C_BIGENDIAN univeral checks to Mac OS X") solved this issue in 2008, so let's remove our workaround. For good measure, if we detect "universal" in the result, error out.
Created attachment 294065 [details] [review] configure.ac: stay out of autoconf's namespace Our self-declared cache variables should be prefixed with glib_ (as most of them already are) instead of ac_, which is reserved to autoconf.
Review of attachment 294064 [details] [review]: Nice work on the commit message! It's probably worth adding a link to this bugzilla in the commit so that any possible future code motion doesn't make it harder for someone to find the rationale using git blame. I.e.: AC_MSG_ERROR([Universal builds not supported; see http://bugzilla.gnome.org/742548])
Review of attachment 294065 [details] [review]: I saw one other error, but honestly I can't bring myself to read more m4 code, so... ::: configure.ac @@ +955,3 @@ AC_CHECK_FUNCS(lchmod lchown fchmod fchown utimes getresuid) AC_CHECK_FUNCS(getmntent_r setmntent endmntent hasmntopt getfsstat getvfsstat fallocate) +case $host_os in aix*) glib_cv_func_splice=no ;; esac # AIX splice() is something else Nope, this case is intentionally overriding the splice check below.
Review of attachment 294059 [details] [review]: Why are you changing this? Does it fix some other bug? Just trying to kill lots of nasty code in galloca.h?
(In reply to comment #10) > Just trying to kill lots of nasty code in galloca.h? Yes. That one will have to wait until we figure out the MSVC thing, in any case.
Review of attachment 294065 [details] [review]: There are more problems with this patch. The statfs/statvfs detection is also messed up. This is even worse than I thought -- not only are we setting variables, in autoconf's namespace, but we're hijacking existing variables and resetting them to our own values.
Comment on attachment 294064 [details] [review] configure.ac: reject 'universal' builds Attachment 294064 [details] pushed as 84a1efe - configure.ac: reject 'universal' builds
Created attachment 294252 [details] [review] configure: assume alloca exists in alloca.h Updated for Windows.
Review of attachment 294252 [details] [review]: Hi, As we aren't using the /Za ("Disable Language Extensions", which is not used by default in MSVC) switch when building GLib or other GLib-using stuff, including malloc.h for Visual Studio builds will do for the alloca() case. At quick glance, otoh, I think it is already required for GLib and GLib-using items to *not* define the /Za switch :). About _malloca(), there is the need to match those with _freea() when freeing, which probably needs further confirmation on the usage of g_alloca(). I am definitely not an export in the autotools parts though, sorry, so can't say much about that part. With blessings.
(In reply to comment #15) > About _malloca(), there is the need to match those with _freea() when freeing, > which probably needs further confirmation on the usage of g_alloca(). This is why I skipped the _malloca(). We really can't require users of g_alloca() to call a free function (nor would I want to). Thanks for the review.
Hello, I am the maintainer of glib in MacPorts. I cannot update our development-version "glib2-devel" port past 2.43.2 because 2.43.3 fails to configure universally with: checking whether byte ordering is bigendian... universal configure: error: Universal builds not supported: see https://bugs.gnome.org/742548 It is necessary for glib to support universal builds, because OS X machines have been using 64-bit Intel processors since 2006, and compilers have defaulted to 64-bit compiles since 2009, but wine, which eventually depends on glib, does not support 64-bit on OS X, and apparently never will: https://forum.winehq.org/viewtopic.php?t=12398 So we need at least to be able to install a 2-way (32-bit/64-bit Intel) universal version of glib. I understand that glib as shipped from you didn't actually work universally. For years MacPorts has been patching glib; we believe with our patches it does work universally. We use this patch before configuring: https://trac.macports.org/browser/trunk/dports/devel/glib2-devel/files/patch-configure.diff And we use this ed script on config.h after configuring: https://trac.macports.org/browser/trunk/dports/devel/glib2-devel/files/config.h.ed Hopefully there is a way for you to incorporate the spirit of these modifications into the wine source so that working universal builds of wine are possible without modifications.
By the way, the URL in the error message (https://bugs.gnome.org/742548) now elicits a 404 not found error.
If I'm understanding correctly it seems like the 'universal' return value from AC_C_BIGENDIAN isn't very informative. If you build with -arch i386 -arch x86_64 then you're still just little endian, but AC_C_BIGENDIAN will return 'universal' and configure will fail.
That's true. I guess autoconf could go the extra mile of checking each of the universal archs specified, and if they're all the same endianness, give you the right AC_C_BIGENDIAN value. But it seems they just took the simpler tactic of not returning any information if any universal build is attempted. This may be the safer course of action. If a configure script is checking endianness, there's a good chance it's also checking bitness, which can also be problematic for universal builds, depending on the combination of archs. And indeed glib's configure script does both. Ideally, don't check endianness or bitness at configure time. Instead, use preprocessor directives at build time. To check for a big-endian system, use `#ifdef __BIG_ENDIAN__`. To check for a 64-bit system, use `#ifdef __LP64__`. See my patches. MacPorts has moved to GitHub and the new URLs for the patches are: https://github.com/macports/macports-ports/blob/master/devel/glib2/files/patch-configure.diff https://github.com/macports/macports-ports/blob/master/devel/glib2/files/config.h.ed Of course I realize you have to support many operating systems, and while this works on macOS, I don't know what other systems' compilers implement these preprocessor directives. BTW, the URL in the universal-not-supported error message is still broken. You could at least fix that.
Review of attachment 294065 [details] [review]: Given that the original aim of this patch was to move towards a faster build with autoconf, I’m going to reject it and close the bug, since we now have Meson for faster builds. Eventually the autotools build system in GLib will be entirely replaced with Meson; so I don’t fancy the risk of breaking it in the mean time by applying this patch. It can always be resurrected later if really necessary (though I hope that m4 dies in a pit of hellfire somewhere soon).
(In reply to Ryan Schmidt from comment #20) > See my patches. MacPorts has moved to GitHub and the new URLs for > the patches are: > > https://github.com/macports/macports-ports/blob/master/devel/glib2/files/ > patch-configure.diff > > https://github.com/macports/macports-ports/blob/master/devel/glib2/files/ > config.h.ed If you could split those up into atomic patches and submit them as separate bug reports here, I would be happy to review and incorporate them, so you don’t have to maintain and rebase them separately in future. > BTW, the URL in the universal-not-supported error message is still broken. > You could at least fix that. Fixed in commit cfe41f4cedfec6de09561411246529886ec9e7cc, thanks. --- Resolving the original bug as WONTFIX as per comment #21.
They are already a single atomic patch, but not suitable for inclusion upstream as-is because they patch the configure script (not the configure.ac template) and then edit the config.h file after running configure. I don't know enough about autotools and how your build is put together to be able to write a patch that you could apply. I had hoped that someone more familiar with that could incorporate the spirit of these changes. You mentioned switching to meson. Is that ready to go now? Does it support universal builds?
(In reply to Ryan Schmidt from comment #24) > They are already a single atomic patch, but not suitable for inclusion > upstream as-is because they patch the configure script (not the configure.ac > template) and then edit the config.h file after running configure. I don't > know enough about autotools and how your build is put together to be able to > write a patch that you could apply. I had hoped that someone more familiar > with that could incorporate the spirit of these changes. Get glib.git, edit configure.ac, run `autogen.sh` to regenerate configure, then run `make all install` to test the build works. I cannot (and do not have time to) rework your patch to apply to git myself, since I have no way of testing it. > You mentioned switching to meson. Is that ready to go now? Does it support > universal builds? The Meson build system is present in glib.git already, and is ready to use, although might have some bugs still (compared to the autotools build). I have no idea if it supports universal builds, but you could try it. If it doesn’t, or if there are bugs in its support for universal builds, please file separate bugs against GLib. I kind of suspect that it won’t support universal builds, since it’s focussed on building for modern systems, and AIUI universal builds have been deprecated for a few years since Apple went 64-bit-only.