Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 host for debian buster to use cross-comp… #8035
Ported Marvell armhf build on amd64 host for debian buster to use cross-comp… #8035
Changes from 8 commits
b8e21f6
c090db1
0575054
eba55b6
1acb980
3da08d9
fcb2b8b
1e079dc
c66756d
e153741
a8e69e5
b4376b2
6a912bb
3e32281
a5c6ba3
cedf62c
3ef6968
4f69553
80446dd
c095ecd
e44472e
bdc6c47
6d37a00
c38b5d0
d10c812
db50dbf
ec639e8
9d7ab8c
544993d
e7d1fa5
844a2d9
ca07e21
5290e07
ca03d67
f6ef895
26f479e
c20e081
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 2 variable names are confusing:
CROSS_BUILD_ENVIRON
vsCROSS_BLDENV
. What is the difference? Can we use one? #ClosedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CROSS_BLDENV is used only on the building/configuring command line to trigger cross-compilation build instead of the default qemu build. Like this:
NOJESSIE=1 NOSTRETCH=1 BLDENV=buster CROSS_BLDENV=1 make target/sonic-marvell-armhf.bin
CROSS_BUILD_ENVIRON is internal variabled used everywhere in the Makefiles.
Please see the PR description above for more info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @gregshpit,
Why should the slave container be using
/var/run/march/docker.sock, /var/run/march/docker.pid
instead of/var/run/docker.sock & /var/run/docker.pid
as it seems you were pulling the slave dockerFROM debian:buster
as opposed to the qemu compilation which usesFROM multiarch/debian-debootstrap:arm64-buster
My build server (amd64), which i've traditionally used to compile amd64 targets doesn't have
/var/run/march/docker.sock & /var/run/march/docker.pid
and is failing when trying to build any DOCKER_TARGETS using cross compilation.But it is working, when i'm restoring this condition to the original i.e. using the
/var/run/docker.sock & /var/run/docker.pid
even when doing the cross compilationThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/var/run/march/docker.sock docker daemon should start automatically. But there is a bug in Makefile.work. To make the automation work in cross-compilation mode please replace line 297 in Makefile.work:
-ifeq ($(MULTIARCH_QEMU_ENVIRON), y)
with$(filter y, $ (MULTIARCH_QEMU_ENVIRON) $(CROSS_BUILD_ENVIRON)),)
+ifneq (
This should start the march docker daemon automatically.
Let me know if it helped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my case though, it didn't work with the new condition. i.e using this command:
docker run --rm=true --privileged --init -v <build_dir>/sonic-buildimage:/sonic -w /sonic -e http_proxy= -e https_proxy= -e no_proxy= -it -v /var/cache/sonic/artifacts:/dpkg_cache:rw -v /var/run/march/docker.sock:/var/run/docker.sock -v /var/run/march/docker.pid:/var/run/docker.pid -v /var/run/march/docker:/var/run/docker -v /var/lib/march/docker:/var/lib/docker
probably because i didn't have the /var/run/march/docker.sock setup. I've raised a query in the community. if you have some pointers on this, please do share.
#8898
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: can these words be on the same line as a previous line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be updated for Bullseye, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is to implement reproducible feature sonic-net/SONiC#684. Why you add here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to copy prebuilt (cross-compiled) python wheels to the dockers directory in order to be installed in the dockers Docker.j2 file. See for example dockers/docker-config-engine-buster/Dockerfile.j2.
Please suggest what and where to do it if this file is inappropriate.
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you treat them as normal Makefile target and normal dependencies of others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using
CROSS_CC
/CROSS_CXX
/CROSS_AR
, canCC
/CXX
/AR
be used directly, and exported as environment variables? That way, multiple places later on won't need lines likeCC=$(CROSS_CC)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be libperl-dev:$arch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems an extra
)
.This embeded if-else-endif is so difficult to read. Could you improve? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is super long. Could you reformat? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some parts in this long line same as above branch. Suggest fine granularity reuse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why treat these packages differently? Possible no special action. #WontFix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why hardcode version 2.7? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why hardcode version 3.7? It will change along time. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why treat package differently? slave just provides a common environment to build any packages in general. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is wrong here that python3 version appears explicitly or that I do some specific handling for SONIC_CONFIG_ENGINE_PY3 in slave.mk ?
SONIC_CONFIG_ENGINE_PY3 and also SONIC_YANG_MODELS_PY3 are special because components using them expect them to be installed in /usr/local/bin/sonic-cfggen and /usr/local/yang-models respectively. However when using cross-compilation these python scripts are installed in some other python virtual environment location. In order to keep them accessible through expected /usr/local/... location I linked the built scripts to these locations.
This should be done every time SONIC_CONFIG_ENGINE_PY3 or SONIC_YANG_MODELS_PY3 wheels are installed.
I could have done it inside these wheels building environment but I did not find any hook for "wheel installation" there except for that in slave.mk .
Please advise how to improve this implementation.
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that you only need virtual env in build, not runtime. How about buiding the python wheel with virtual env, but let slave container install the wheel outside virtual env.
Not an expert on the solution. The concern is that this is not general and not scalable.