| Message ID | 20160802051434.17067-1-cyril.bur@au1.ibm.com |
|---|---|
| State | Rejected |
| Headers | show |
On Tue, Aug 02, 2016 at 03:14:34PM +1000, Cyril Bur wrote: > diff --git a/external/common/rules.mk b/external/common/rules.mk > index bb12fd5..59e8905 100644 > --- a/external/common/rules.mk > +++ b/external/common/rules.mk > @@ -1,5 +1,8 @@ > -CC ?= $(CROSS_COMPILE)gcc > -LD ?= $(CROSS_COMPILE)ld > +ifdef CROSS_COMPILE > +CC = $(CROSS_COMPILE)gcc > +LD = $(CROSS_COMPILE)ld > +endif > + > ARCH := $(shell $(GET_ARCH) "$(CROSS_COMPILE)") I don't see myself as a make expert, nor do I know what the best practices are in this regard. Ultimately, you guys need to decide what is best for these makefiles but, with my current understanding, unless there is a corresponding change in both op-build and openbmc, this change will not work. I've already spent a lot of time in the last month on getting libflash / pflash makefiles set so we can do cross-compiling on ARM. I know Joel, Brad and Stewart have also put in time in this area. If this change goes in, I would prefer that someone volunteer to also do the corresponding updates to the op-build and openbmc distros. Now for my observations... There is currently a CROSS and a CROSS_COMPILE variable in skiboot. What is each for? The ARCH variable is only set if CROSS_COMPILE is set with a proper prefix and the ARCH is critical for compiling libflash correctly. Thus, with this change it will not be possible to set both a custom CC and allow the makefiles to continue to derive the ARCH from the prefix. As I have previously stated, having CC overwritten by CROSS_COMPILE breaks a non-simple CC and ties you to gcc (precluding clang). We don't expect distros to determine ARCH themselves, do we? Even the op-build recipe for libflash sets both CC and CROSS_COMPILE, so this isn't just a Yocto "problem". https://github.com/open-power/op-build/blob/master/openpower/package/libflash/libflash.mk#L20
On Tue, 2 Aug 2016 14:10:23 -0500 Patrick Williams <patrick@stwcx.xyz> wrote: > On Tue, Aug 02, 2016 at 03:14:34PM +1000, Cyril Bur wrote: > > diff --git a/external/common/rules.mk b/external/common/rules.mk > > index bb12fd5..59e8905 100644 > > --- a/external/common/rules.mk > > +++ b/external/common/rules.mk > > @@ -1,5 +1,8 @@ > > -CC ?= $(CROSS_COMPILE)gcc > > -LD ?= $(CROSS_COMPILE)ld > > +ifdef CROSS_COMPILE > > +CC = $(CROSS_COMPILE)gcc > > +LD = $(CROSS_COMPILE)ld > > +endif > > + > > ARCH := $(shell $(GET_ARCH) "$(CROSS_COMPILE)") > > I don't see myself as a make expert, nor do I know what the best > practices are in this regard. Ultimately, you guys need to decide what > is best for these makefiles but, with my current understanding, unless > there is a corresponding change in both op-build and openbmc, this change > will not work. What exists doesn't make sense, I'm quite sure I hit what Oliver said would happen. CC is always defined my make its self so ?= can't do what was intended. This meant what while I used CROSS_COMPILE=/my/arm/cross-compiler, I actually compiled for x86 as make defaulted to my system CC. > > I've already spent a lot of time in the last month on getting libflash > / pflash makefiles set so we can do cross-compiling on ARM. I know Joel, > Brad and Stewart have also put in time in this area. If this change > goes in, I would prefer that someone volunteer to also do the > corresponding updates to the op-build and openbmc distros. > A lot of people have put a lot of effort in and it seems that each people had their own use case they wanted to care about, heres mine: As a somewhat frequent manual compiler of pflash and gard I don't want to jump through hoops on the command line in order to get these to compile. The README says to either include compilers is your PATH or to set CROSS. I feel this should extend to the external/ binaries as well. So, while I'm all for building skiboot and external/ tools in an automated way, I actually do have a personal buildroot which builds pflash into an initramfs, which I now suspect will have bitrotted as it only sets CROSS_COMPILE... It appears we broke manual compiling somewhere along the way. Admittedly I should have been paying more attention to the list. We cannot be 'fixing' skiboot for op-build or openbmc build processes which results in breaking manual compilation. > Now for my observations... > > There is currently a CROSS and a CROSS_COMPILE variable in skiboot. > What is each for? I think they're the same, we should absolutely try to consolidate to one. The README suggests using CROSS to specify a cross compiler, if we do all agree, I'm more than happy to send a s/CROSS_COMPILE/CROSS/ patch to external/ tools > > The ARCH variable is only set if CROSS_COMPILE is set with a proper > prefix and the ARCH is critical for compiling libflash correctly. Thus, > with this change it will not be possible to set both a custom CC and > allow the makefiles to continue to derive the ARCH from the prefix. As Makes sense, so how about: -ARCH := $(shell $(GET_ARCH) "$(CROSS_COMPILE)") +ARCH ?= $(shell $(GET_ARCH) "$(CROSS_COMPILE)") > I have previously stated, having CC overwritten by CROSS_COMPILE breaks > a non-simple CC and ties you to gcc (precluding clang). We don't expect > distros to determine ARCH themselves, do we? > Both openbmc and op-build should know what arch they're building so setting ARCH can't be a big deal? And with ARCH ?= could that work? > Even the op-build recipe for libflash sets both CC and CROSS_COMPILE, so > this isn't just a Yocto "problem". > https://github.com/open-power/op-build/blob/master/openpower/package/libflash/libflash.mk#L20 >
On Wed, Aug 3, 2016 at 5:10 AM, Patrick Williams <patrick@stwcx.xyz> wrote: > On Tue, Aug 02, 2016 at 03:14:34PM +1000, Cyril Bur wrote: >> diff --git a/external/common/rules.mk b/external/common/rules.mk >> index bb12fd5..59e8905 100644 >> --- a/external/common/rules.mk >> +++ b/external/common/rules.mk >> @@ -1,5 +1,8 @@ >> -CC ?= $(CROSS_COMPILE)gcc >> -LD ?= $(CROSS_COMPILE)ld >> +ifdef CROSS_COMPILE >> +CC = $(CROSS_COMPILE)gcc >> +LD = $(CROSS_COMPILE)ld >> +endif >> + >> ARCH := $(shell $(GET_ARCH) "$(CROSS_COMPILE)") > > I don't see myself as a make expert, nor do I know what the best > practices are in this regard. Ultimately, you guys need to decide what > is best for these makefiles but, with my current understanding, unless > there is a corresponding change in both op-build and openbmc, this change > will not work. > > I've already spent a lot of time in the last month on getting libflash > / pflash makefiles set so we can do cross-compiling on ARM. I know Joel, > Brad and Stewart have also put in time in this area. If this change > goes in, I would prefer that someone volunteer to also do the > corresponding updates to the op-build and openbmc distros. > > Now for my observations... > > There is currently a CROSS and a CROSS_COMPILE variable in skiboot. > What is each for? They do the same thing. Originally there was just CROSS, but linux uses CROSS_COMPILE to set the toolchain prefix so it was added for consistency. > > The ARCH variable is only set if CROSS_COMPILE is set with a proper > prefix and the ARCH is critical for compiling libflash correctly. Thus, > with this change it will not be possible to set both a custom CC and > allow the makefiles to continue to derive the ARCH from the prefix. As > I have previously stated, having CC overwritten by CROSS_COMPILE breaks > a non-simple CC and ties you to gcc (precluding clang). We don't expect > distros to determine ARCH themselves, do we? Sounds like this is the actual problem. There's nothing wrong with having CROSS_COMPILE setting CC, it just needs to be opt-in and using CROSS_COMPILE to find the target architecture forces you to set it. > > Even the op-build recipe for libflash sets both CC and CROSS_COMPILE, so > this isn't just a Yocto "problem". > https://github.com/open-power/op-build/blob/master/openpower/package/libflash/libflash.mk#L20 Yeah, my bad. This is really a skiboot externals problem that op-build and openbmc are working around. > > -- > Patrick Williams > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot >
diff --git a/external/common/rules.mk b/external/common/rules.mk index bb12fd5..59e8905 100644 --- a/external/common/rules.mk +++ b/external/common/rules.mk @@ -1,5 +1,8 @@ -CC ?= $(CROSS_COMPILE)gcc -LD ?= $(CROSS_COMPILE)ld +ifdef CROSS_COMPILE +CC = $(CROSS_COMPILE)gcc +LD = $(CROSS_COMPILE)ld +endif + ARCH := $(shell $(GET_ARCH) "$(CROSS_COMPILE)") ifeq ($(ARCH),ARCH_ARM)
As pointed out by Oliver O'Halloran: make automatically defines CC so the ?= assignment is never done. You can get the desired behaviour with and ifdef block though. This patch adds that ifdef block. Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> --- external/common/rules.mk | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)