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

supporting crossbuild all components #3515

Merged
merged 1 commit into from
Dec 29, 2021
Merged

supporting crossbuild all components #3515

merged 1 commit into from
Dec 29, 2021

Conversation

fisherxu
Copy link
Member

@fisherxu fisherxu commented Dec 28, 2021 •
edited

Signed-off-by: fisherxu xufei40@huawei.com

What type of PR is this?

What this PR does / why we need it:
Supporting crossbuild all components, now we can crossbuild all components as below:

#   make crossbuild
#   make crossbuild HELP=y
#   make crossbuild WHAT=edgecore
#   make crossbuild WHAT=edgecore GOARM=GOARM7
#   make crossbuild WHAT=keadm GOARM=GOARM8

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
cc @chendave @zhu733756 @gy95

Does this PR introduce a user-facing change?:

NONE

@kubeedge-bot kubeedge-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 28, 2021
fisherxu
fisherxu commented Dec 28, 2021
View reviewed changes
Makefile Outdated
#
#
# Example:
# make crossbuild
# make crossbuild HELP=y
# make crossbuild WHAT=edgecore
# make crossbuild WHAT=edgecore GOARM=GOARM7
# make crossbuild WHAT=edgecore ARM=GOARM7
Copy link
Member Author

Choose a reason for hiding this comment

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

GOARM is a golang env value, so use ARM here to avoid conflicting with golang. Besides, if I set GOARM=GOARM8, will exit with err like: Invalid GOARM value. Must be 5, 6, or 7.

Copy link
Member

Choose a reason for hiding this comment

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

But why it works before? Let me have a try.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I set GOARM=GOARM8, will exit with err Invalid GOARM value. Must be 5, 6, or 7., before for armv8, just haven't set the GOARM value, default is armv8

Copy link
Member

Choose a reason for hiding this comment

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

agree, but the code should not crush if we pass the value. I create a simple quick fix for this, pls let me know whether that make sense to you: #3522

Copy link
Member

Choose a reason for hiding this comment

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

And how about define the variable as "ARM_VERSION" instead of "ARM"? so it is more descriptive.

Copy link
Member Author

Choose a reason for hiding this comment

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

With your pr, I think we can still use GOARM :) PTAL @chendave

@gy95
Copy link
Member

gy95 commented Dec 28, 2021

make crossbuild WHAT=edgecore error

hack/make-rules/clean.sh
hack/make-rules/crossbuild.sh edgecore 
edgecore does not support cross build
make: *** [Makefile:178: crossbuild] Error 1

gy95
gy95 reviewed Dec 28, 2021
View reviewed changes
hack/lib/golang.sh Outdated
KUBEEDGE_ALL_CROSS_BINARIES=(
edgecore
)

kubeedge::golang::is_cross_build_binary() {
local key=$1
for bin in "${KUBEEDGE_ALL_CROSS_BINARIES[@]}" ; do
Copy link
Member

Choose a reason for hiding this comment

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

here also use this deleted KUBEEDGE_ALL_CROSS_BINARIES variable, also need fix

Copy link
Member

Choose a reason for hiding this comment

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

to fix make crossbuild WHAT=edgecore error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM test OK

@kubeedge-bot kubeedge-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 28, 2021
zhu733756
zhu733756 reviewed Dec 28, 2021
View reviewed changes
Makefile
@@ -155,18 +155,18 @@ define CROSSBUILD_HELP_INFO
# cross build components.
#
# Args:
# WHAT: Component names to be lint check. support: $(CROSSBUILD_COMPONENTS)
# WHAT: Component names to be lint check. support: $(BINARIES)
Copy link
Member

@zhu733756 zhu733756 Dec 28, 2021 •
edited

Choose a reason for hiding this comment

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

Use $(BINARIES) here, CROSSBUILD_COMPONENTS seems can be removed? Line 150

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Have removed :)

chendave
chendave reviewed Dec 29, 2021
View reviewed changes
Makefile Outdated
# GOARM: go arm value, now support:$(GOARM_VALUES)
# If not specified ,default use GOARM=GOARM8
# GOARM: arm version for go crossbuild, now support:$(GOARM_VALUES)
# If not specified, default use ARM=GOARM8
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... GOARM is the original args, but you have updated this to "ARM", so it would be something like,

ARM: arm version....

But I still think using ARM here is a little misleading.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I also think ARM maybe misleading.. ;-)

@fisherxu
add all components to crossbuild
59b2aa9 
Signed-off-by: fisherxu <xufei40@huawei.com>
@kubeedge-bot kubeedge-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 29, 2021
@kubeedge-bot
Copy link
Collaborator

@gy95: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@chendave
Copy link
Member

/lgtm
/approve

thanks!

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 29, 2021
@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chendave, gy95

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubeedge-bot kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 29, 2021
@kubeedge-bot kubeedge-bot merged commit 24113a5 into kubeedge:master Dec 29, 2021
@fisherxu fisherxu deleted the cross-build branch December 29, 2021 09:21
@gy95 gy95 mentioned this pull request Mar 14, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chendave chendave chendave left review comments

@zhu733756 zhu733756 zhu733756 left review comments

@gy95 gy95 gy95 approved these changes

@kadisi kadisi Awaiting requested review from kadisi

@sids-b sids-b Awaiting requested review from sids-b

Assignees

@chendave chendave

@zc2638 zc2638

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@fisherxu @gy95 @kubeedge-bot @chendave @zhu733756 @zc2638