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