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.

Sign up for GitHub

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

Jump to bottom

new-kernel:fix cross build #3132

Merged
merged 1 commit into from
May 15, 2023
Merged

new-kernel:fix cross build #3132

merged 1 commit into from
May 15, 2023

Conversation

famleebob
Copy link
Contributor

@famleebob famleebob commented Mar 31, 2023 •
edited
Loading

EVE_TARGET_ARCH must be correctly and consistenly set to correclty build expected targets. Further, EVE_BUILD_ARCH must be set to reflect the curren build host architecture. The correct value for this is provided by buildkit as BUILDARCH, only used in one place.

Validated by building arm64 on arm64, amd64 on arm64, and riscv64 on arm64.

Need to have this change validated with arm64 on amd64, and amd64 on amd64, and riscv64 on amd64.

@eriknordmark eriknordmark requested review from rene and mikem-zed April 3, 2023 00:44
@famleebob famleebob force-pushed the new_kern_x_build branch 2 times, most recently from fea1f7e to a937237 Compare April 4, 2023 16:18
rene
rene reviewed Apr 24, 2023
View reviewed changes
pkg/new-kernel/Dockerfile Outdated
@@ -176,7 +180,7 @@ RUN ./autogen.sh && \
--with-linux-obj=/linux \
--with-config=kernel \
--enable-linux-builtin \
--host="${EVE_TARGET_ARCH}"-linux-musl --build="${EVE_BUILD_ARCH}"-linux-musl && \
Copy link
Contributor

Choose a reason for hiding this comment

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

@famleebob , did you figure out why EVE_BUILD_ARCH is not reflecting the right architecture here? In theory you shouldn't need to change this variable....

@rene
Copy link
Contributor

rene commented Apr 24, 2023

@famleebob , I will test on my x86 host and post results here...

@rene
Copy link
Contributor

rene commented Apr 25, 2023

@famleebob , I will test on my x86 host and post results here...

I've performed the following builds (all successfully):

host target
x86 x86
x86 aarch64
x86 riscv64

@famleebob , note, however, that you will need to rebase your PR....

@famleebob
Copy link
Contributor Author

I also built all three architectures on a Mac M1 (aarch64). that is amd64, aarch64 (arm64), and riscv64. after the re-base.

@famleebob
Copy link
Contributor Author

repeated the three builds on my Mac M1, after the last rebase.

eriknordmark
eriknordmark approved these changes Apr 28, 2023
View reviewed changes
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGT Rene

@eriknordmark
Copy link
Contributor

@famleebob you need a DCO signature on each commit. See instructions.

@famleebob
Copy link
Contributor Author

The problem with the DCO check was the email address I used. Corrected that with a commit. Looks like all is ready to go.

eriknordmark
eriknordmark approved these changes May 2, 2023
View reviewed changes
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Run eden again

rene
rene approved these changes May 2, 2023
View reviewed changes
Copy link
Contributor

@rene rene left a comment

Choose a reason for hiding this comment

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

LGTM.

rene
rene requested changes May 2, 2023
View reviewed changes
pkg/new-kernel/Dockerfile Outdated
@@ -93,6 +96,7 @@ ENV KERNEL_DEFCONFIG=defconfig
# build for all arches
# hadolint ignore=DL3006
FROM kernel-target-${TARGETARCH} AS kernel-build
ARG BUILDARCH
Copy link
Contributor

Choose a reason for hiding this comment

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

@famleebob, you forgot this ARG from the first version of the PR, we don't need it anymore (correctly pointed out by @christoph-zededa)

@rene rene mentioned this pull request May 2, 2023
Merged
@famleebob famleebob force-pushed the new_kern_x_build branch from 625d4e1 to 43d257e Compare May 3, 2023 08:58
@famleebob
Copy link
Contributor Author

Updated per comments (thanks Christoph).

@famleebob
Copy link
Contributor Author

Re-ran builds on all three architectures (amd64, aarch64, and riscv64)

@rene
Copy link
Contributor

rene commented May 8, 2023

@famleebob , just a final touch, since you already ran the tests, you can remove the following excerpt from your commit message:

Validated by building `arm64` on `arm64`, `amd64` on `arm64`, and
`riscv64` on `arm64`.

Need to have this change validated with `arm64` on `amd64`, and
`amd64` on `amd64`, and `riscv64` on `amd64`.

maybe just let a sample phrase telling the changes were validated for all (arm64, amd64 and riscv64)...

@famleebob
Copy link
Contributor Author

famleebob commented May 12, 2023 •
edited
Loading

Tested build on x86_64 machine for (amd64, arm64, and riscv64) native/cross builds.

Also cleaned up commit comment as suggested.

@eriknordmark
Copy link
Contributor

@famleebob seems like you have 29 more commits than you should in this PR. Please rebase on master and then make sure you have a single commit for your changes (and no merge commits).
FWIW I setup my workspaces with git remote add upstream https://github.com/lf-edge/eve, and then I can do git fetch upstream master; git rebase -i upstream/master.
That way I can ensure I don't get any merge commits.

@famleebob
new-kernel:fix cross build
78ec1a1 
`EVE_TARGET_ARCH` must be correctly and consistenly set to correclty
build expected targets.  Further, `EVE_BUILD_ARCH` must be set to
reflect the curren build host architecture.  The correct value for
this is provided by `buildkit` as `BUILDARCH`, only used in one
place.

Validated cross build of `arm64`, `amd64`,and  `arm64`.

Signed-off-by: Gerald (Bob) Lee <bob@famleehouse.net>
@eriknordmark eriknordmark merged commit 07b7122 into lf-edge:master May 15, 2023
@famleebob famleebob deleted the new_kern_x_build branch August 4, 2023 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rene rene rene requested changes

@eriknordmark eriknordmark eriknordmark approved these changes

@rvs rvs Awaiting requested review from rvs

@mikem-zed mikem-zed Awaiting requested review from mikem-zed

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@famleebob @rene @eriknordmark