Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ported Marvell armhf build on amd64 for debian buster to use cross-comp… #1267

Merged
merged 1 commit into from
Jul 9, 2021

Conversation

gregshpit
Copy link
Contributor

…ilation instead of qemu emulation

…ilation instead of qemu emulation

Motivation:
Current armhf Sonic build on amd64 host uses qemu emulation. Due to the nature of the emulation it takes a very long time, about 22-24 hours to complete the build. The change I did to improve the building time ports Sonic armhf build on amd64 host for Marvell platform for debian buster to use cross-compilation on arm64 host for armhf target. The overall Sonic armhf building time using cross-compilation is about 6 hours.

The Sonic configure and build for the armhf cross-compilation is as following:
NOJESSIE=1 NOSTRETCH=1 BLDENV=buster CROSS_BLDENV=1 make configure PLATFORM=marvell-armhf PLATFORM_ARCH=armhf
NOJESSIE=1 NOSTRETCH=1 BLDENV=buster CROSS_BLDENV=1 make target/sonic-marvell-armhf.bin

Sonic module should check if $CROSS_BUILD_ENVIRON is 'y' to make sure that it is cross-compilation build.

meta/Makefile Outdated
@@ -68,6 +68,14 @@ $(foreach bin,$(BINS),$(if $(shell which $(bin)),,$(error "Missing $(bin) in PAT

CFLAGS += -I../inc -I../experimental $(WARNINGS)

ifeq ($(CROSS_BUILD_ENVIRON),y)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CROSS_BUILD_ENVIRON is that your internal variable name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This environment variable is set in Sonic slave.mk when Sonic build uses cross-complation.

Copy link
Collaborator

@kcudnik kcudnik Jul 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i grepped sonic-buildimage and i could not find it, can you give a link in github where this is set up? it also names seems to be truncated, should be CROSS_BUILD_ENVIRONMENT or CROSS_BUILD_ENV, this make more sense, also google returns nothing for "CROSS_BUILD_ENVIRON" :(

Copy link
Collaborator

@kcudnik kcudnik Jul 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, im just wondering, normally when we have configure.ac and ./configure, to specify cross compiler paramst are used from --host=HOST, this is standalone Makefile, so maybe it make more sense to make it similar like here: https://lists.gnu.org/archive/html/help-make/2005-05/msg00029.html and then passing

$ make ARCH=xxx CROSS_COMPILE=yyy

or whoever would set CROSS_COMPILE, or it could be even set CC and CXX before make, instead of doing all those if/else across all makefiles on the entire sonic

please do research on variable "CROSS_COMPILE" as it seems it's most common method to update Makefiles
take a look here:

and take a look how CROSS_COMPILE is used, ** and i would advise to go that direction **, i would like you also to update/refactor other repos that you made PRs to use CROSS_COMPILE pattern in them too, adding @lguohan for visibility

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sonic-buildimage PR sonic-net/sonic-buildimage#8035 is part of of the overall PR. You can find CROSS_BUILD_ENVIRON environment variable set in slave.mk that I've modified. The naming convention is taken from another variable MULTIARCH_QEMU_ENVIRON defined in this file. CROSS_BUILD_ENVIRON env. variable is used in hundreds files in the PR. I'll check the rest of your request tomorrow. Thanks for your comments.

meta/Makefile Outdated
@@ -68,6 +68,14 @@ $(foreach bin,$(BINS),$(if $(shell which $(bin)),,$(error "Missing $(bin) in PAT

CFLAGS += -I../inc -I../experimental $(WARNINGS)

ifeq ($(CROSS_BUILD_ENVIRON),y)
Copy link
Collaborator

@kcudnik kcudnik Jul 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, im just wondering, normally when we have configure.ac and ./configure, to specify cross compiler paramst are used from --host=HOST, this is standalone Makefile, so maybe it make more sense to make it similar like here: https://lists.gnu.org/archive/html/help-make/2005-05/msg00029.html and then passing

$ make ARCH=xxx CROSS_COMPILE=yyy

or whoever would set CROSS_COMPILE, or it could be even set CC and CXX before make, instead of doing all those if/else across all makefiles on the entire sonic

please do research on variable "CROSS_COMPILE" as it seems it's most common method to update Makefiles
take a look here:

and take a look how CROSS_COMPILE is used, ** and i would advise to go that direction **, i would like you also to update/refactor other repos that you made PRs to use CROSS_COMPILE pattern in them too, adding @lguohan for visibility

@gregshpit
Copy link
Contributor Author

Hi,

I think do pass CROSS_COMPILE variable to the Makefile. If it is true (I'll check it) then instead of:
+ifeq ($(CROSS_BUILD_ENVIRON),y)

  • CC=$(CROSS_CC)
  • CXX=$(CROSS_CXX)
    +else
  • CC=gcc
  • CXX=g++
    +endif

I can just write as you suggested in all modified files:
CC = $(CROSS_COMPILE)gcc
CXX = $(CROSS_COMPILE)g++

Is it OK with you.

Gregory

@kcudnik
Copy link
Collaborator

kcudnik commented Jul 4, 2021

I can just write as you suggested in all modified files:
CC = $(CROSS_COMPILE)gcc
CXX = $(CROSS_COMPILE)g++

Is it OK with you.

yes, this is exactly what i had in mind!

…ilation instead of qemu emulation

Signed-off-by: marvell <marvell@cpss-build3.marvell.com>
@lguohan lguohan merged commit 4a50910 into opencomputeproject:master Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants