| Message ID | 20190214233447.29005-1-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | honor CFLAGS & friends from environment | expand |
On 14/02/2019 23:34, Luc Van Oostenryck wrote: > From: Uwe Kleine-König <uwe@kleine-koenig.org> > > Debian build scripts pass CFLAGS in the environment. > However, this is ignored by Sparse's Makefile since 'CFLAGS' > is unconditionaly initialized. > > Fix this by initializing CFLAGS to their default value using '?='. > Do the same for PKG_CONFIG, CHECKER, CHECKER_FLAGS and > DESTDIR, BINDIR, MANDIR & MAN1DIR. > > Note: It's useless to try to do the same for CC, LD & AR since > they're builtin variables so '?= ...' is a no-op for them > (unless make is called with -R). > > Note: This makes sparse native builds reproducible for Debian. > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> > --- > Makefile | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/Makefile b/Makefile > index bd2b089f3..2d72be9e2 100644 > --- a/Makefile > +++ b/Makefile > @@ -6,19 +6,18 @@ OS = linux > > > CC = gcc > -CFLAGS = -O2 -g > -CFLAGS += -Wall -Wwrite-strings > +CFLAGS ?= -O2 -g -Wall -Wwrite-strings > LD = $(CC) > AR = ar > -PKG_CONFIG = pkg-config > -CHECKER = CHECK=./sparse ./cgcc -no-compile > -CHECKER_FLAGS = -Wno-vla > +PKG_CONFIG ?= pkg-config > +CHECKER ?= CHECK=./sparse ./cgcc -no-compile > +CHECKER_FLAGS ?= -Wno-vla Hmm, I don't think CHECKER and CHECKER_FLAGS should be included here. These are, to my mind anyway, internal to the makefile and should not be settable from the environment. Everything else looks OK. ;-) [Uwe - in an earlier email you said the you had 'other patches applied' (including setting CC=gcc-8 and LDFLAGS). An obvious question would be: What patches and why? :-D (I assume it has something to do with 'packaging', but the question remains).] ATB, Ramsay Jones > > -DESTDIR= > +DESTDIR ?= > PREFIX ?= $(HOME) > -BINDIR=$(PREFIX)/bin > -MANDIR=$(PREFIX)/share/man > -MAN1DIR=$(MANDIR)/man1 > +BINDIR ?= $(PREFIX)/bin > +MANDIR ?= $(PREFIX)/share/man > +MAN1DIR ?= $(MANDIR)/man1 > > # Allow users to override build settings without dirtying their trees > # For debugging, put this in local.mk: >
On 14/02/2019 23:34, Luc Van Oostenryck wrote: > From: Uwe Kleine-König <uwe@kleine-koenig.org> > > Debian build scripts pass CFLAGS in the environment. > However, this is ignored by Sparse's Makefile since 'CFLAGS' > is unconditionaly initialized. > > Fix this by initializing CFLAGS to their default value using '?='. > Do the same for PKG_CONFIG, CHECKER, CHECKER_FLAGS and > DESTDIR, BINDIR, MANDIR & MAN1DIR. > > Note: It's useless to try to do the same for CC, LD & AR since > they're builtin variables so '?= ...' is a no-op for them > (unless make is called with -R). BTW, I have seen (a long time ago) an ugly hack to try and get around this 'special behaviour' of the variables from make's built-in database. It is very ugly, ... Suppose you wanted to set 'CC ?= gcc' in the Makefile, but because CC is 'special', you don't get the 'use the value from the environment if set, else default to this value'. An ugly workaround would look something like this: $ git diff diff --git a/Makefile b/Makefile index 025bce2..a98a9fc 100644 --- a/Makefile +++ b/Makefile @@ -4,8 +4,9 @@ VERSION=0.6.0 # The following variables can be overwritten from the command line OS = linux - +ifeq ($(origin CC),default) CC = gcc +endif CFLAGS = -O2 -g CFLAGS += -Wall -Wwrite-strings LD = $(CC) @@ -20,6 +21,9 @@ BINDIR=$(PREFIX)/bin MANDIR=$(PREFIX)/share/man MAN1DIR=$(MANDIR)/man1 +fred: + @echo "CC is $(CC), origin: " $(origin CC) + # Allow users to override build settings without dirtying their trees # For debugging, put this in local.mk: # $ $ make fred Makefile:128: Your system does not have libxml, disabling c2xml Makefile:150: Your system does not have gtk3/gtk2, disabling test-inspect Makefile:183: Your system does not have llvm, disabling sparse-llvm CC is gcc, origin: file $ $ make CC=cmd fred make: cmd: Command not found Makefile:128: Your system does not have libxml, disabling c2xml Makefile:150: Your system does not have gtk3/gtk2, disabling test-inspect Makefile:183: Your system does not have llvm, disabling sparse-llvm CC is cmd, origin: command line $ $ CC=env make fred env: unrecognised option '--print-file-name=' Try 'env --help' for more information. Makefile:128: Your system does not have libxml, disabling c2xml Makefile:150: Your system does not have gtk3/gtk2, disabling test-inspect Makefile:183: Your system does not have llvm, disabling sparse-llvm CC is env, origin: environment $ [Note: the 'make: cmd: Command not found' and similar message about the 'env' invocation comes from the '$(shell $(CC) ...)' further down the makefile (see line 100, setting GCC_BASE).] Hmm, it may be possible to create a define ... :-P ATB, Ramsay Jones
On Fri, Feb 15, 2019 at 01:42:53AM +0000, Ramsay Jones wrote: > > > On 14/02/2019 23:34, Luc Van Oostenryck wrote: > > From: Uwe Kleine-König <uwe@kleine-koenig.org> > > > > Debian build scripts pass CFLAGS in the environment. > > However, this is ignored by Sparse's Makefile since 'CFLAGS' > > is unconditionaly initialized. > > > > Fix this by initializing CFLAGS to their default value using '?='. > > Do the same for PKG_CONFIG, CHECKER, CHECKER_FLAGS and > > DESTDIR, BINDIR, MANDIR & MAN1DIR. > > > > Note: It's useless to try to do the same for CC, LD & AR since > > they're builtin variables so '?= ...' is a no-op for them > > (unless make is called with -R). > > > > Note: This makes sparse native builds reproducible for Debian. > > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> > > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> > > --- > > Makefile | 17 ++++++++--------- > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index bd2b089f3..2d72be9e2 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -6,19 +6,18 @@ OS = linux > > > > > > CC = gcc > > -CFLAGS = -O2 -g > > -CFLAGS += -Wall -Wwrite-strings > > +CFLAGS ?= -O2 -g -Wall -Wwrite-strings > > LD = $(CC) > > AR = ar > > -PKG_CONFIG = pkg-config > > -CHECKER = CHECK=./sparse ./cgcc -no-compile > > -CHECKER_FLAGS = -Wno-vla > > +PKG_CONFIG ?= pkg-config > > +CHECKER ?= CHECK=./sparse ./cgcc -no-compile > > +CHECKER_FLAGS ?= -Wno-vla > > Hmm, I don't think CHECKER and CHECKER_FLAGS should be > included here. These are, to my mind anyway, internal > to the makefile and should not be settable from the > environment. > > Everything else looks OK. ;-) > > [Uwe - in an earlier email you said the you had 'other > patches applied' (including setting CC=gcc-8 and LDFLAGS). > An obvious question would be: What patches and why? :-D > (I assume it has something to do with 'packaging', but > the question remains).] I use CC = gcc-8 for the debian package because other it can happen (and it actually did) that /usr/bin/gcc was updated from gcc-X to gcc-(X+1) and then sparse started to fail because it still used /usr/lib/gcc/x86_64-linux-gnu/X/ (i.e. the output of gcc --print-file-name= ) which disappeard as it changes on major version bumps. So sparse depends explicitly on the current gcc version and uses this. The LDFLAGS change isn't strictly Debian specific, upstream could maybe benefit here, too. See https://sources.debian.org/src/sparse/0.6.0-3/debian/patches/ld-as-needed.patch/ for the actual change. Best regards Uwe
On Fri, Feb 15, 2019 at 02:18:44AM +0000, Ramsay Jones wrote: > On 14/02/2019 23:34, Luc Van Oostenryck wrote: > > From: Uwe Kleine-König <uwe@kleine-koenig.org> > > > > Debian build scripts pass CFLAGS in the environment. > > However, this is ignored by Sparse's Makefile since 'CFLAGS' > > is unconditionaly initialized. > > > > Fix this by initializing CFLAGS to their default value using '?='. > > Do the same for PKG_CONFIG, CHECKER, CHECKER_FLAGS and > > DESTDIR, BINDIR, MANDIR & MAN1DIR. > > > > Note: It's useless to try to do the same for CC, LD & AR since > > they're builtin variables so '?= ...' is a no-op for them > > (unless make is called with -R). > > BTW, I have seen (a long time ago) an ugly hack to try and > get around this 'special behaviour' of the variables from > make's built-in database. It is very ugly, ... Yes, for a few seconds I considered using $(origin) after having realized that "CC ?= gcc" was useless but indeed, it's very ugly. > Suppose you wanted to set 'CC ?= gcc' in the Makefile, but > because CC is 'special', you don't get the 'use the value > from the environment if set, else default to this value'. It seemed logical to allow CC, LD & AR to be configurable from the environment, like CFLAGS is now. But it's already possible to do it via the command line (and personnaly, I always do this via a local config file like local.mk). > $ make CC=cmd fred > make: cmd: Command not found > Makefile:128: Your system does not have libxml, disabling c2xml > Makefile:150: Your system does not have gtk3/gtk2, disabling test-inspect > Makefile:183: Your system does not have llvm, disabling sparse-llvm > CC is cmd, origin: command line > $ > > $ CC=env make fred > env: unrecognised option '--print-file-name=' > Try 'env --help' for more information. > Makefile:128: Your system does not have libxml, disabling c2xml > Makefile:150: Your system does not have gtk3/gtk2, disabling test-inspect > Makefile:183: Your system does not have llvm, disabling sparse-llvm > CC is env, origin: environment > $ > > [Note: the 'make: cmd: Command not found' and similar message > about the 'env' invocation comes from the '$(shell $(CC) ...)' > further down the makefile (see line 100, setting GCC_BASE).] > > Hmm, it may be possible to create a define ... :-P Well, a "2>/dev/null" could be added but I think it's preferable to have these kind of messages. Best regards, -- Luc
On Fri, Feb 15, 2019 at 01:42:53AM +0000, Ramsay Jones wrote: > On 14/02/2019 23:34, Luc Van Oostenryck wrote: > > From: Uwe Kleine-König <uwe@kleine-koenig.org> > > > > Debian build scripts pass CFLAGS in the environment. > > However, this is ignored by Sparse's Makefile since 'CFLAGS' > > is unconditionaly initialized. > > > > Fix this by initializing CFLAGS to their default value using '?='. > > Do the same for PKG_CONFIG, CHECKER, CHECKER_FLAGS and > > DESTDIR, BINDIR, MANDIR & MAN1DIR. > > > > Note: It's useless to try to do the same for CC, LD & AR since > > they're builtin variables so '?= ...' is a no-op for them > > (unless make is called with -R). > > > > Note: This makes sparse native builds reproducible for Debian. > > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> > > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> > > --- > > Makefile | 17 ++++++++--------- > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index bd2b089f3..2d72be9e2 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -6,19 +6,18 @@ OS = linux > > > > > > CC = gcc > > -CFLAGS = -O2 -g > > -CFLAGS += -Wall -Wwrite-strings > > +CFLAGS ?= -O2 -g -Wall -Wwrite-strings > > LD = $(CC) > > AR = ar > > -PKG_CONFIG = pkg-config > > -CHECKER = CHECK=./sparse ./cgcc -no-compile > > -CHECKER_FLAGS = -Wno-vla > > +PKG_CONFIG ?= pkg-config > > +CHECKER ?= CHECK=./sparse ./cgcc -no-compile > > +CHECKER_FLAGS ?= -Wno-vla > > Hmm, I don't think CHECKER and CHECKER_FLAGS should be > included here. These are, to my mind anyway, internal > to the makefile and should not be settable from the > environment. Yes, I see things like this too but I thought you added CHECKER explicitly for being able to override it. I've moved them now. Thanks, -- Luc
On Fri, Feb 15, 2019 at 09:34:55AM +0100, Uwe Kleine-König wrote: > On Fri, Feb 15, 2019 at 01:42:53AM +0000, Ramsay Jones wrote: > > [Uwe - in an earlier email you said the you had 'other > > patches applied' (including setting CC=gcc-8 and LDFLAGS). > > An obvious question would be: What patches and why? :-D > > (I assume it has something to do with 'packaging', but > > the question remains).] > > I use CC = gcc-8 for the debian package because other it can happen (and > it actually did) that /usr/bin/gcc was updated from gcc-X to gcc-(X+1) > and then sparse started to fail because it still used > /usr/lib/gcc/x86_64-linux-gnu/X/ (i.e. the output of > > gcc --print-file-name= > > ) which disappeard as it changes on major version bumps. > > So sparse depends explicitly on the current gcc version and uses this. > > The LDFLAGS change isn't strictly Debian specific, upstream could maybe > benefit here, too. See > > https://sources.debian.org/src/sparse/0.6.0-3/debian/patches/ld-as-needed.patch/ > > for the actual change. Problem is that '--as-needed' is not supported on all platforms (it doesn't for me on a not so old FreeBSD, for example). --Luc
On 15/02/2019 08:34, Uwe Kleine-König wrote: [snip] > I use CC = gcc-8 for the debian package because other it can happen (and > it actually did) that /usr/bin/gcc was updated from gcc-X to gcc-(X+1) > and then sparse started to fail because it still used > /usr/lib/gcc/x86_64-linux-gnu/X/ (i.e. the output of > > gcc --print-file-name= > > ) which disappeard as it changes on major version bumps. > > So sparse depends explicitly on the current gcc version and uses this. Hmm, yes, sparse may well depend a little too much on gcc. ;-) Having said that, these values built-in to sparse are just default values that can be set from the command-line of sparse (see -gcc-base-dir and -multiarch-dir). Indeed they _are_ set by cgcc by running the 'specified c compiler' at each invocation, if not _also_ specified on the cgcc command line. Hmm, do you remember how/what failed? (I suppose this was not a kernel usage). ATB, Ramsay Jones
On 2/15/19 6:22 PM, Ramsay Jones wrote: > > > On 15/02/2019 08:34, Uwe Kleine-König wrote: > [snip] >> I use CC = gcc-8 for the debian package because other it can happen (and >> it actually did) that /usr/bin/gcc was updated from gcc-X to gcc-(X+1) >> and then sparse started to fail because it still used >> /usr/lib/gcc/x86_64-linux-gnu/X/ (i.e. the output of >> >> gcc --print-file-name= >> >> ) which disappeard as it changes on major version bumps. >> >> So sparse depends explicitly on the current gcc version and uses this. > > Hmm, yes, sparse may well depend a little too much on gcc. ;-) > > Having said that, these values built-in to sparse are just > default values that can be set from the command-line of > sparse (see -gcc-base-dir and -multiarch-dir). Indeed they > _are_ set by cgcc by running the 'specified c compiler' at > each invocation, if not _also_ specified on the cgcc command > line. > > Hmm, do you remember how/what failed? (I suppose this was not > a kernel usage). Right. The problem was https://bugs.debian.org/906472 . In short, Makefile has: GCC_BASE := $(shell $(CC) --print-file-name=) cflags += -DGCC_BASE=\"$(GCC_BASE)\" which is then used in lib.c to find some header files. The version of sparse that #906472 was reported against was build with gcc-7, so sparse used /usr/lib/gcc/x86_64-linux-gnu/7/ on amd64. Then when gcc-7 wasn't installed (gcc was provided by gcc-8) sparse failed to check userspace code because the gcc headers were not found. Best regards Uwe
On 15/02/2019 20:17, Uwe Kleine-König wrote: > On 2/15/19 6:22 PM, Ramsay Jones wrote: >> >> >> On 15/02/2019 08:34, Uwe Kleine-König wrote: >> [snip] >>> I use CC = gcc-8 for the debian package because other it can happen (and >>> it actually did) that /usr/bin/gcc was updated from gcc-X to gcc-(X+1) >>> and then sparse started to fail because it still used >>> /usr/lib/gcc/x86_64-linux-gnu/X/ (i.e. the output of >>> >>> gcc --print-file-name= >>> >>> ) which disappeard as it changes on major version bumps. >>> >>> So sparse depends explicitly on the current gcc version and uses this. >> >> Hmm, yes, sparse may well depend a little too much on gcc. ;-) >> >> Having said that, these values built-in to sparse are just >> default values that can be set from the command-line of >> sparse (see -gcc-base-dir and -multiarch-dir). Indeed they >> _are_ set by cgcc by running the 'specified c compiler' at >> each invocation, if not _also_ specified on the cgcc command >> line. >> >> Hmm, do you remember how/what failed? (I suppose this was not >> a kernel usage). > > Right. The problem was https://bugs.debian.org/906472 . Oh, my word! So, the 'horst' package was using sparse on user-space code directly, not via 'cgcc -no-compile'? I am somewhat surprised that it worked at all! ;-) Also, it seems that the package was built in a chroot environment where the compiler was 'swapped out' mid build! What ??? No, no I must have misunderstood something. :( I have never built a debian package, so you must excuse my ignorance of package build requirements. 'sparse' has improved quite a bit recently, but I would still highly recommend using 'cgcc [-no-compile]' on user-space code! ATB, Ramsay Jones
On 2/15/19 10:10 PM, Ramsay Jones wrote: > > > On 15/02/2019 20:17, Uwe Kleine-König wrote: >> On 2/15/19 6:22 PM, Ramsay Jones wrote: >>> >>> >>> On 15/02/2019 08:34, Uwe Kleine-König wrote: >>> [snip] >>>> I use CC = gcc-8 for the debian package because other it can happen (and >>>> it actually did) that /usr/bin/gcc was updated from gcc-X to gcc-(X+1) >>>> and then sparse started to fail because it still used >>>> /usr/lib/gcc/x86_64-linux-gnu/X/ (i.e. the output of >>>> >>>> gcc --print-file-name= >>>> >>>> ) which disappeard as it changes on major version bumps. >>>> >>>> So sparse depends explicitly on the current gcc version and uses this. >>> >>> Hmm, yes, sparse may well depend a little too much on gcc. ;-) >>> >>> Having said that, these values built-in to sparse are just >>> default values that can be set from the command-line of >>> sparse (see -gcc-base-dir and -multiarch-dir). Indeed they >>> _are_ set by cgcc by running the 'specified c compiler' at >>> each invocation, if not _also_ specified on the cgcc command >>> line. >>> >>> Hmm, do you remember how/what failed? (I suppose this was not >>> a kernel usage). >> >> Right. The problem was https://bugs.debian.org/906472 . > > Oh, my word! So, the 'horst' package was using sparse on user-space > code directly, not via 'cgcc -no-compile'? I am somewhat surprised > that it worked at all! ;-) > > Also, it seems that the package was built in a chroot environment > where the compiler was 'swapped out' mid build! What ??? No, no I > must have misunderstood something. :( There happened nothing in the middle of the build. sparse 0.5.2-1 was built in April 2018 when gcc was still gcc-7, so it used the gcc-7 path. Later in August after gcc was bumped to gcc-8 to build horst the following packages were installed: sparse 0.5.2-1 gcc-8 and no gcc-7. As sparse was not rebuild from source but just the binary from April installed sparse failed to find stdarg.h. So the problem was that sparse hard-coded a path to the gcc-7 includes but didn't ensure gcc-7 to be around. Today sparse hardcodes the gcc-8 includes and depends on gcc-8 such that the used path is provided and available to pick includes from there. > 'sparse' has improved quite a bit recently, but I would still > highly recommend using 'cgcc [-no-compile]' on user-space code! Yeah, Luc's 949a29adb9f400ebdc15aed2dd874c17ffa839cb is a right step here. Best regards Uwe
On 15/02/2019 21:28, Uwe Kleine-König wrote: [snip] >>> Right. The problem was https://bugs.debian.org/906472 . >> >> Oh, my word! So, the 'horst' package was using sparse on user-space >> code directly, not via 'cgcc -no-compile'? I am somewhat surprised >> that it worked at all! ;-) >> >> Also, it seems that the package was built in a chroot environment >> where the compiler was 'swapped out' mid build! What ??? No, no I >> must have misunderstood something. :( > > There happened nothing in the middle of the build. > > sparse 0.5.2-1 was built in April 2018 when gcc was still gcc-7, so it > used the gcc-7 path. Later in August after gcc was bumped to gcc-8 to > build horst the following packages were installed: > > sparse 0.5.2-1 > gcc-8 > > and no gcc-7. As sparse was not rebuild from source but just the binary > from April installed sparse failed to find stdarg.h. Ah, yes, that makes sense. So, had the 'horst' Makefile used 'cgcc -no-compile', then it would likely have worked. (cgcc would have called 'gcc -print-file-name=', which would have passed the gcc-8 path to sparse). ATB, Ramsay Jones
diff --git a/Makefile b/Makefile index bd2b089f3..2d72be9e2 100644 --- a/Makefile +++ b/Makefile @@ -6,19 +6,18 @@ OS = linux CC = gcc -CFLAGS = -O2 -g -CFLAGS += -Wall -Wwrite-strings +CFLAGS ?= -O2 -g -Wall -Wwrite-strings LD = $(CC) AR = ar -PKG_CONFIG = pkg-config -CHECKER = CHECK=./sparse ./cgcc -no-compile -CHECKER_FLAGS = -Wno-vla +PKG_CONFIG ?= pkg-config +CHECKER ?= CHECK=./sparse ./cgcc -no-compile +CHECKER_FLAGS ?= -Wno-vla -DESTDIR= +DESTDIR ?= PREFIX ?= $(HOME) -BINDIR=$(PREFIX)/bin -MANDIR=$(PREFIX)/share/man -MAN1DIR=$(MANDIR)/man1 +BINDIR ?= $(PREFIX)/bin +MANDIR ?= $(PREFIX)/share/man +MAN1DIR ?= $(MANDIR)/man1 # Allow users to override build settings without dirtying their trees # For debugging, put this in local.mk: