diff mbox series

[2/3] build: allow picking the env values for compiler variables

Message ID 20190726133331.91482-3-roger.pau@citrix.com (mailing list archive)
State Superseded
Headers show
Series build: honor toolchain related environment vars | expand

Commit Message

Roger Pau Monné July 26, 2019, 1:33 p.m. UTC
Don't force the usage of the hardcoded compiler values if those are
already set on the environment. This allows the Xen build system to
correctly pick CC/CXX values present on the environment, and fixes the
usage of those by the Gitlab CI test system.

Note that without this fix the Xen build system will completely ignore
any CC or CXX values set on the environment, and the only way to pass
a different CC or CXX is to overwrite it on the make command line.

Due to this change, Travis CI needs to be updated in order to pass a
CC and CXX that also contains the CROSS_COMPILE path, since Xen will
no longer overwrite the CC or CXX value if those are set on the
environment.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wl@xen.org>
---
 config/StdGNU.mk     | 35 +++++++++++++++++++----------------
 scripts/travis-build |  8 ++++++++
 2 files changed, 27 insertions(+), 16 deletions(-)

Comments

Jan Beulich July 29, 2019, 3:35 p.m. UTC | #1
On 26.07.2019 15:33, Roger Pau Monne wrote:
> Don't force the usage of the hardcoded compiler values if those are
> already set on the environment. This allows the Xen build system to
> correctly pick CC/CXX values present on the environment, and fixes the
> usage of those by the Gitlab CI test system.
> 
> Note that without this fix the Xen build system will completely ignore
> any CC or CXX values set on the environment, and the only way to pass
> a different CC or CXX is to overwrite it on the make command line.

Now the question is: Do we possibly want it to be that way? I've always
been of the opinion that inheriting something that happens to be (left?)
set in the environment is not a good idea. Hence I've been welcoming all
changes that removed dependencies on settings possibly coming from the
environment. (Exceptions of course are XEN_* environment variables we
specifically evaluate.)

As a result I'm inclined to nak this patch, but I'm open to arguments.

Jan
Roger Pau Monné Aug. 20, 2019, 7:58 a.m. UTC | #2
On Mon, Jul 29, 2019 at 03:35:36PM +0000, Jan Beulich wrote:
> On 26.07.2019 15:33, Roger Pau Monne wrote:
> > Don't force the usage of the hardcoded compiler values if those are
> > already set on the environment. This allows the Xen build system to
> > correctly pick CC/CXX values present on the environment, and fixes the
> > usage of those by the Gitlab CI test system.
> > 
> > Note that without this fix the Xen build system will completely ignore
> > any CC or CXX values set on the environment, and the only way to pass
> > a different CC or CXX is to overwrite it on the make command line.
> 
> Now the question is: Do we possibly want it to be that way? I've always
> been of the opinion that inheriting something that happens to be (left?)
> set in the environment is not a good idea.

Then what's the point in having an environment with stuff anyway if
Xen build is just going to ignore it?

I don't have things 'left' in the environment, neither have most build
systems AFAIK. I do have things set that I want the build to
acknowledge, instead of fight against it.

> Hence I've been welcoming all
> changes that removed dependencies on settings possibly coming from the
> environment. (Exceptions of course are XEN_* environment variables we
> specifically evaluate.)
> 
> As a result I'm inclined to nak this patch, but I'm open to arguments.

IMO doing things like this is going to make Xen harder to package for
everyone, since distro build systems and test systems (like Travis or
Gitlab) expect the build system to pick the relevant
compiler/toolchain variables from the environment. Not doing it this
way means we need to adapt each build system to work with Xen.

Thanks, Roger.
Jan Beulich Aug. 27, 2019, 9:17 a.m. UTC | #3
On 20.08.2019 09:58, Roger Pau Monné  wrote:
> On Mon, Jul 29, 2019 at 03:35:36PM +0000, Jan Beulich wrote:
>> On 26.07.2019 15:33, Roger Pau Monne wrote:
>>> Don't force the usage of the hardcoded compiler values if those are
>>> already set on the environment. This allows the Xen build system to
>>> correctly pick CC/CXX values present on the environment, and fixes the
>>> usage of those by the Gitlab CI test system.
>>>
>>> Note that without this fix the Xen build system will completely ignore
>>> any CC or CXX values set on the environment, and the only way to pass
>>> a different CC or CXX is to overwrite it on the make command line.
>>
>> Now the question is: Do we possibly want it to be that way? I've always
>> been of the opinion that inheriting something that happens to be (left?)
>> set in the environment is not a good idea.
> 
> Then what's the point in having an environment with stuff anyway if
> Xen build is just going to ignore it?
> 
> I don't have things 'left' in the environment, neither have most build
> systems AFAIK. I do have things set that I want the build to
> acknowledge, instead of fight against it.

My view is this: XEN_* things coming from the environment are fine.
Generic (i.e. project agnostic) variables (like CC) otoh would
better be ignored, as it's not clear for what purpose they've been
set. (Istr a case where some FORTIFY option was set by RPM build
environments, breaking our build.) They may well have been meant
for some other project.

Question is whether to take the above just for the hypervisor part
of the build, or also the tool stack etc ones.

>> Hence I've been welcoming all
>> changes that removed dependencies on settings possibly coming from the
>> environment. (Exceptions of course are XEN_* environment variables we
>> specifically evaluate.)
>>
>> As a result I'm inclined to nak this patch, but I'm open to arguments.
> 
> IMO doing things like this is going to make Xen harder to package for
> everyone, since distro build systems and test systems (like Travis or
> Gitlab) expect the build system to pick the relevant
> compiler/toolchain variables from the environment. Not doing it this
> way means we need to adapt each build system to work with Xen.

Well, what you describe means customizing the environment. What I
suggest means customizing the make command line. But it's customization
in either case. It _may_ happen that customizing the environment for
Xen means doing nothing, and the same settings being useful for other
projects as well. But this doesn't have to be this way and, as said
above, has been causing problems.

Furthermore - what do you do if different parts of the tree (xen/,
tools/, stubdom/) want to have different settings in effect? To me
it's quite a bit more natural to have three different but specific
make invocations, rather than fiddling with various env vars between
any two of them.

Jan
Ian Jackson Aug. 27, 2019, 10:59 a.m. UTC | #4
Jan Beulich writes ("Re: [PATCH 2/3] build: allow picking the env values for compiler variables"):
> On 20.08.2019 09:58, Roger Pau Monné  wrote:
> > I don't have things 'left' in the environment, neither have most build
> > systems AFAIK. I do have things set that I want the build to
> > acknowledge, instead of fight against it.
> 
> My view is this: XEN_* things coming from the environment are fine.
> Generic (i.e. project agnostic) variables (like CC) otoh would
> better be ignored, as it's not clear for what purpose they've been
> set. (Istr a case where some FORTIFY option was set by RPM build
> environments, breaking our build.) They may well have been meant
> for some other project.

CC is set *specifically to cause build systems's like Xen's to use
that compiler*.  That is its purpose.  It should be honoured, not
ignored.

Likewise FORTIFY, even though it broke something.  FORTIFY_SOURCE was
set specifically to cause the changes it did.  The people who set it
didn't see all the implications, but that change *was* their design
intent and the bugs were real bugs in what they were trying to do.

> Question is whether to take the above just for the hypervisor part
> of the build, or also the tool stack etc ones.

*Definitely* for the toolstack build, we should honour CC et al.

The hypervisor is a more subtle question because people who set these
variables often forget to think about kernel code, embedded code,
etc.  That's what happened with FORTIFY_SOURCE.  However, I would
argue that it is better, in such a situation, to honour the variable
and break the build, than it is to simply ignore it.

Ian.
Roger Pau Monné Aug. 29, 2019, 10:30 a.m. UTC | #5
On Tue, Aug 27, 2019 at 11:59:33AM +0100, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH 2/3] build: allow picking the env values for compiler variables"):
> > On 20.08.2019 09:58, Roger Pau Monné  wrote:
> > > I don't have things 'left' in the environment, neither have most build
> > > systems AFAIK. I do have things set that I want the build to
> > > acknowledge, instead of fight against it.
> > 
> > My view is this: XEN_* things coming from the environment are fine.
> > Generic (i.e. project agnostic) variables (like CC) otoh would
> > better be ignored, as it's not clear for what purpose they've been
> > set. (Istr a case where some FORTIFY option was set by RPM build
> > environments, breaking our build.) They may well have been meant
> > for some other project.
> 
> CC is set *specifically to cause build systems's like Xen's to use
> that compiler*.  That is its purpose.  It should be honoured, not
> ignored.
> 
> Likewise FORTIFY, even though it broke something.  FORTIFY_SOURCE was
> set specifically to cause the changes it did.  The people who set it
> didn't see all the implications, but that change *was* their design
> intent and the bugs were real bugs in what they were trying to do.
> 
> > Question is whether to take the above just for the hypervisor part
> > of the build, or also the tool stack etc ones.
> 
> *Definitely* for the toolstack build, we should honour CC et al.
> 
> The hypervisor is a more subtle question because people who set these
> variables often forget to think about kernel code, embedded code,
> etc.  That's what happened with FORTIFY_SOURCE.  However, I would
> argue that it is better, in such a situation, to honour the variable
> and break the build, than it is to simply ignore it.

I fully agree. It's true that some build systems will likely break
bulding Xen, but that's a build system issue, and we shouldn't try to
work around this in Xen. As said before, what build systems (like
travis or gitlab for instance) set in the environment should be
acknowledged by Xen, or else we are forcing everyone to have custom
workarounds for building Xen.

Thanks, Roger.
diff mbox series

Patch

diff --git a/config/StdGNU.mk b/config/StdGNU.mk
index 7a6159021b..b3072f5b13 100644
--- a/config/StdGNU.mk
+++ b/config/StdGNU.mk
@@ -1,28 +1,31 @@ 
 # Use Clang/LLVM instead of GCC?
 clang     ?= n
 
-# If we are not cross-compiling, default HOSTC{C/XX} to C{C/XX}
-ifeq ($(XEN_TARGET_ARCH), $(XEN_COMPILE_ARCH))
-HOSTCC    ?= $(CC)
-HOSTCXX   ?= $(CXX)
-endif
-
 AS         = $(CROSS_COMPILE)as
 LD         = $(CROSS_COMPILE)ld
 ifeq ($(clang),y)
 gcc       := n
-CC         = $(CROSS_COMPILE)clang
-CXX        = $(CROSS_COMPILE)clang++
-LD_LTO     = $(CROSS_COMPILE)llvm-ld
-HOSTCC    ?= clang
-HOSTCXX   ?= clang++
+DEF_CC     = clang
+DEF_CXX    = clang++
+LD_LTO    ?= $(CROSS_COMPILE)llvm-ld
 else
 gcc       := y
-CC         = $(CROSS_COMPILE)gcc
-CXX        = $(CROSS_COMPILE)g++
-LD_LTO     = $(CROSS_COMPILE)ld
-HOSTCC    ?= gcc
-HOSTCXX   ?= g++
+DEF_CC     = gcc
+DEF_CXX    = g++
+LD_LTO    ?= $(CROSS_COMPILE)ld
+endif
+
+CC        ?= $(CROSS_COMPILE)$(DEF_CC)
+CXX       ?= $(CROSS_COMPILE)$(DEF_CXX)
+
+# If we are not cross-compiling, default HOSTC{C/XX} to C{C/XX}
+# else use the default values if unset
+ifeq ($(XEN_TARGET_ARCH), $(XEN_COMPILE_ARCH))
+HOSTCC    ?= $(CC)
+HOSTCXX   ?= $(CXX)
+else
+HOSTCC    ?= $(DEF_CC)
+HOSTCXX   ?= $(DEF_CXX)
 endif
 
 CPP        = $(CC) -E
diff --git a/scripts/travis-build b/scripts/travis-build
index 0cb15a89e4..a264e286b2 100755
--- a/scripts/travis-build
+++ b/scripts/travis-build
@@ -1,6 +1,14 @@ 
 #!/bin/bash -ex
 
+# Set HOST{CC/CXX} in case we are cross building
+export HOSTCC=${CC}
+export HOSTCXX=${CXX}
+# Prefix environment CC/CXX with CROSS_COMPILE if present
+export CC=${CROSS_COMPILE}${CC}
+export CXX=${CROSS_COMPILE}${CXX}
+
 $CC --version
+[[ "${CC}" != "${HOSTCC}" ]] && $HOSTCC --version
 
 # random config or default config
 if [[ "${RANDCONFIG}" == "y" ]]; then