* [PATCH] Fix building xfsprogs on 32-bit platforms
@ 2016-11-28 22:42 Eric Biggers
2016-11-29 20:09 ` Christoph Hellwig
2016-11-29 21:44 ` Dave Chinner
0 siblings, 2 replies; 6+ messages in thread
From: Eric Biggers @ 2016-11-28 22:42 UTC (permalink / raw)
To: linux-xfs; +Cc: Eric Biggers
Since commit 7fda99a0c297 ("xfs.h: require transparent LFS for all
users"), building xfsprogs fails on a 32-bit platform because it does
not define _FILE_OFFSET_BITS=64 itself. Fix it by adding this to CFLAGS
in builddefs.in.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
include/builddefs.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/builddefs.in b/include/builddefs.in
index aeb2905..5219071 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -25,7 +25,7 @@ OPTIMIZER = @opt_build@
MALLOCLIB = @malloc_lib@
LOADERFLAGS = @LDFLAGS@
LTLDFLAGS = @LDFLAGS@
-CFLAGS = @CFLAGS@
+CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64
LIBRT = @librt@
LIBUUID = @libuuid@
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] Fix building xfsprogs on 32-bit platforms 2016-11-28 22:42 [PATCH] Fix building xfsprogs on 32-bit platforms Eric Biggers @ 2016-11-29 20:09 ` Christoph Hellwig 2016-11-29 21:44 ` Dave Chinner 1 sibling, 0 replies; 6+ messages in thread From: Christoph Hellwig @ 2016-11-29 20:09 UTC (permalink / raw) To: Eric Biggers; +Cc: linux-xfs Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix building xfsprogs on 32-bit platforms 2016-11-28 22:42 [PATCH] Fix building xfsprogs on 32-bit platforms Eric Biggers 2016-11-29 20:09 ` Christoph Hellwig @ 2016-11-29 21:44 ` Dave Chinner 2016-11-29 22:21 ` Eric Biggers 1 sibling, 1 reply; 6+ messages in thread From: Dave Chinner @ 2016-11-29 21:44 UTC (permalink / raw) To: Eric Biggers; +Cc: linux-xfs On Mon, Nov 28, 2016 at 02:42:35PM -0800, Eric Biggers wrote: > Since commit 7fda99a0c297 ("xfs.h: require transparent LFS for all > users"), building xfsprogs fails on a 32-bit platform because it does > not define _FILE_OFFSET_BITS=64 itself. Fix it by adding this to CFLAGS > in builddefs.in. > > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > include/builddefs.in | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/builddefs.in b/include/builddefs.in > index aeb2905..5219071 100644 > --- a/include/builddefs.in > +++ b/include/builddefs.in > @@ -25,7 +25,7 @@ OPTIMIZER = @opt_build@ > MALLOCLIB = @malloc_lib@ > LOADERFLAGS = @LDFLAGS@ > LTLDFLAGS = @LDFLAGS@ > -CFLAGS = @CFLAGS@ > +CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64 So, umm, why aren't these lines in configure.ac setting this already on your system? AC_SYS_LARGEFILE AC_NEED_LFS Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix building xfsprogs on 32-bit platforms 2016-11-29 21:44 ` Dave Chinner @ 2016-11-29 22:21 ` Eric Biggers 2016-11-29 23:25 ` Dave Chinner 0 siblings, 1 reply; 6+ messages in thread From: Eric Biggers @ 2016-11-29 22:21 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs On Wed, Nov 30, 2016 at 08:44:56AM +1100, Dave Chinner wrote: > On Mon, Nov 28, 2016 at 02:42:35PM -0800, Eric Biggers wrote: > > Since commit 7fda99a0c297 ("xfs.h: require transparent LFS for all > > users"), building xfsprogs fails on a 32-bit platform because it does > > not define _FILE_OFFSET_BITS=64 itself. Fix it by adding this to CFLAGS > > in builddefs.in. > > > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > --- > > include/builddefs.in | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/builddefs.in b/include/builddefs.in > > index aeb2905..5219071 100644 > > --- a/include/builddefs.in > > +++ b/include/builddefs.in > > @@ -25,7 +25,7 @@ OPTIMIZER = @opt_build@ > > MALLOCLIB = @malloc_lib@ > > LOADERFLAGS = @LDFLAGS@ > > LTLDFLAGS = @LDFLAGS@ > > -CFLAGS = @CFLAGS@ > > +CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64 > > So, umm, why aren't these lines in configure.ac setting this already > on your system? > > AC_SYS_LARGEFILE > AC_NEED_LFS > > Cheers, > AC_SYS_LARGEFILE puts the _FILE_OFFSET_BITS definition in the generated config header, not in CFLAGS. Normally projects use a config.h.in template that's generated from configure.ac using autoheader. xfsprogs doesn't do this but rather uses a non-generated file include/platform_defs.h.in. Since there is no "#undef _FILE_OFFSET_BITS" in platform_defs.h.in, '#define _FILE_OFFSET_BITS 64' doesn't gets included in platform_defs.h. Also, even if it was, then I think it wouldn't always work because not all .c files in xfsprogs have platform_defs.h as their first #include. The reason AC_NEED_LFS still passes is that 'configure' includes a temporary header confdefs.h rather than the final generated header, and the temporary header includes the _FILE_OFFSET_BITS definition. Eric ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix building xfsprogs on 32-bit platforms 2016-11-29 22:21 ` Eric Biggers @ 2016-11-29 23:25 ` Dave Chinner 2016-11-30 19:12 ` Eric Biggers 0 siblings, 1 reply; 6+ messages in thread From: Dave Chinner @ 2016-11-29 23:25 UTC (permalink / raw) To: Eric Biggers; +Cc: linux-xfs On Tue, Nov 29, 2016 at 02:21:35PM -0800, Eric Biggers wrote: > On Wed, Nov 30, 2016 at 08:44:56AM +1100, Dave Chinner wrote: > > On Mon, Nov 28, 2016 at 02:42:35PM -0800, Eric Biggers wrote: > > > Since commit 7fda99a0c297 ("xfs.h: require transparent LFS for all > > > users"), building xfsprogs fails on a 32-bit platform because it does > > > not define _FILE_OFFSET_BITS=64 itself. Fix it by adding this to CFLAGS > > > in builddefs.in. > > > > > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > > --- > > > include/builddefs.in | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/include/builddefs.in b/include/builddefs.in > > > index aeb2905..5219071 100644 > > > --- a/include/builddefs.in > > > +++ b/include/builddefs.in > > > @@ -25,7 +25,7 @@ OPTIMIZER = @opt_build@ > > > MALLOCLIB = @malloc_lib@ > > > LOADERFLAGS = @LDFLAGS@ > > > LTLDFLAGS = @LDFLAGS@ > > > -CFLAGS = @CFLAGS@ > > > +CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64 > > > > So, umm, why aren't these lines in configure.ac setting this already > > on your system? > > > > AC_SYS_LARGEFILE > > AC_NEED_LFS > > > > Cheers, > > > > AC_SYS_LARGEFILE puts the _FILE_OFFSET_BITS definition in the generated config > header, not in CFLAGS. <sigh> It's times like this that I really hate software. This is not what the documentation implies occurs: https://www.gnu.org/software/autoconf/manual/autoconf-2.65/html_node/System-Services.html Macro: AC_SYS_LARGEFILE Arrange for 64-bit file offsets, known as large-file support. On some hosts, one must use special compiler options to build programs that can access large files. Append any such options to the output variable CC. Define _FILE_OFFSET_BITS and _LARGE_FILES if necessary. i.e. it implies that the required LFS options are automatically appended to CC variable. > Normally projects use a config.h.in template that's generated from > configure.ac using autoheader. xfsprogs doesn't do this but > rather uses a non-generated file include/platform_defs.h.in. We didn't add any hoop jumping, because the autoconf documentation implies it is not necessary. I trusted that the patch submitter actually tested it on 32 bit systems and that it all worked. > Since there is no "#undef _FILE_OFFSET_BITS" in > platform_defs.h.in, '#define _FILE_OFFSET_BITS 64' doesn't gets > included in platform_defs.h. platform_defs.h should not need to do this - we handle build environment specific defines in builddefs.h.... > Also, even if it was, then I think > it wouldn't always work because not all .c files in xfsprogs have > platform_defs.h as their first #include. .... precisely for this reason. So, builddefs.in should really detect if autoconf set a "large file required" flag, and in that case add the _FILE_OFFSET_BITS value that autoconf has generated to the PCFLAGS variable, right? (PCFLAGS are "Platform CFLAGS") Or, if we are just going to unconditionally define -D_FILE_OFFSET_BITS=64 in CFLAGS, then the autoconf checks are completely useless and should be removed? > The reason AC_NEED_LFS still passes is that 'configure' includes a > temporary header confdefs.h rather than the final generated > header, and the temporary header includes the _FILE_OFFSET_BITS > definition. IOWs, stupid code backed by ambiguous documentation means that developers that try to do the right thing still end up with broken builds. Eric, to speed things up a bit next time, can you try to explain how something is broken in the commit message, not just say "it's broken, fix it". This avoids the need to go back and forth multiple times just so we can understand what the problem is that you are fixing.... Cheers, -Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix building xfsprogs on 32-bit platforms 2016-11-29 23:25 ` Dave Chinner @ 2016-11-30 19:12 ` Eric Biggers 0 siblings, 0 replies; 6+ messages in thread From: Eric Biggers @ 2016-11-30 19:12 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs Hi Dave, On Wed, Nov 30, 2016 at 10:25:49AM +1100, Dave Chinner wrote: > > So, builddefs.in should really detect if autoconf set a "large file > required" flag, and in that case add the _FILE_OFFSET_BITS value > that autoconf has generated to the PCFLAGS variable, right? (PCFLAGS > are "Platform CFLAGS") > > Or, if we are just going to unconditionally define > -D_FILE_OFFSET_BITS=64 in CFLAGS, then the autoconf checks are > completely useless and should be removed? > I don't believe it's possible to use the results of AC_SYS_LARGEFILE without using a config header that is included *first* in all .c files. So the other option is to define _FILE_OFFSET_BITS=64 ourselves in CFLAGS. (Or PCFLAGS for just the platforms that need it; I know it's needed on 32-bit Linux, but I don't know about other platforms.) And yes, in that case the autoconf macros are useless and should be removed. Sorry for not explaining more in the commit message --- I hadn't looked into the details of what was going on with the autoconf macros (or that they were recently added, for that matter) until you pointed them out. Eric > > The reason AC_NEED_LFS still passes is that 'configure' includes a > > temporary header confdefs.h rather than the final generated > > header, and the temporary header includes the _FILE_OFFSET_BITS > > definition. > > IOWs, stupid code backed by ambiguous documentation means that > developers that try to do the right thing still end up with broken > builds. > > Eric, to speed things up a bit next time, can you try to explain how > something is broken in the commit message, not just say "it's > broken, fix it". This avoids the need to go back and forth multiple > times just so we can understand what the problem is that you are > fixing.... > > Cheers, > > -Dave. > -- > Dave Chinner > david@fromorbit.com ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-30 19:12 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-28 22:42 [PATCH] Fix building xfsprogs on 32-bit platforms Eric Biggers 2016-11-29 20:09 ` Christoph Hellwig 2016-11-29 21:44 ` Dave Chinner 2016-11-29 22:21 ` Eric Biggers 2016-11-29 23:25 ` Dave Chinner 2016-11-30 19:12 ` Eric Biggers
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.