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

Add ARM64 build targets for kops and nodeup #8922

Merged
merged 4 commits into from
Jun 3, 2020
Merged

Add ARM64 build targets for kops and nodeup #8922

merged 4 commits into from
Jun 3, 2020

Conversation

hakman
Copy link
Member

@hakman hakman commented Apr 16, 2020 •
edited
Loading

Adding the ARM64 build targets for kops and nodeup to see how build and CI is behaving.
This should be merged first to avoid issue with ARM64 assets not being found by kops apply cluster.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 16, 2020
@hakman hakman changed the title [WIP] Add ARM64 build targets for kops and nodeup Add ARM64 build targets for kops and nodeup Apr 16, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 16, 2020
@justinsb
Copy link
Member

This looks great. It would be great to rationalize some of the makefile tasks (I think we have bazel and non-bazel versions for everything!), but I agree with just adding arm64 and not trying to tackle that at the same time.

I'm going to mark this for 1.19, as I worry that e.g. changing the path to nodeup will break some things, and we're about to cut 1.18. This seems like a good reason to create the 1.18 branch also!

@justinsb justinsb added this to the v1.19 milestone Apr 17, 2020
@hakman
Copy link
Member Author

hakman commented Apr 17, 2020

Thanks @justinsb, sounds good to be a feature for 1.19!

@hakman hakman force-pushed the build-arm64 branch 2 times, most recently from be22ac3 to cc48042 Compare April 19, 2020 05:22
@hakman
Copy link
Member Author

hakman commented Apr 19, 2020

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 2, 2020
@hakman
Copy link
Member Author

hakman commented Jun 2, 2020

/assign @justinsb
/cc @mikesplain

@k8s-ci-robot k8s-ci-robot requested a review from mikesplain June 2, 2020 17:17
rifelpet
rifelpet reviewed Jun 3, 2020
View reviewed changes
Makefile Show resolved Hide resolved
rifelpet
rifelpet reviewed Jun 3, 2020
View reviewed changes
Makefile Show resolved Hide resolved
@rifelpet
Copy link
Member

rifelpet commented Jun 3, 2020

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 3, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman, rifelpet

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 3, 2020
johngmyers
johngmyers reviewed Jun 3, 2020
View reviewed changes
Makefile
@@ -216,8 +216,19 @@ ${DIST}/linux/amd64/nodeup: ${BINDATA_TARGETS}
mkdir -p ${DIST}
GOOS=linux GOARCH=amd64 go build ${GCFLAGS} -a ${EXTRA_BUILDFLAGS} -o $@ ${LDFLAGS}"${EXTRA_LDFLAGS} -X k8s.io/kops.Version=${VERSION} -X k8s.io/kops.GitVersion=${GITSHA}" k8s.io/kops/cmd/nodeup

.PHONY: ${DIST}/linux/arm64/nodeup
Copy link
Member

Choose a reason for hiding this comment

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

Not actually phony.

Makefile
@@ -239,14 +258,18 @@ ${DIST}/linux/amd64/kops: ${BINDATA_TARGETS}
mkdir -p ${DIST}
GOOS=linux GOARCH=amd64 go build ${GCFLAGS} -a ${EXTRA_BUILDFLAGS} -o $@ ${LDFLAGS}"${EXTRA_LDFLAGS} -X k8s.io/kops.Version=${VERSION} -X k8s.io/kops.GitVersion=${GITSHA}" k8s.io/kops/cmd/kops

.PHONY: ${DIST}/linux/arm64/kops
Copy link
Member

Choose a reason for hiding this comment

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

Not phony

Copy link
Member Author

Choose a reason for hiding this comment

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

Some cleanup in the "phony" area would be nice also. Kept it similar to the similar targets for now.

Makefile
@@ -229,6 +240,14 @@ crossbuild-nodeup-in-docker:
docker kill nodeup-build-${UNIQUE}
docker rm nodeup-build-${UNIQUE}

.PHONY: nodeup-dist
nodeup-dist: crossbuild-nodeup-in-docker
mkdir -p ${DIST}
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't cross-build-nodeup-in-docker have created ${DIST}? Otherwise where does ${DIST}/linux/amd64/nodeup come from?

Copy link
Member Author

@hakman hakman Jun 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, this is pretty much not needed. I think it was copy/paste. In any case, seems that there are some more places where mkdirs would not be needed. Some cleanup will be nice.

Makefile
mkdir -p ${UPLOAD}/kops/${VERSION}/darwin/amd64/
mkdir -p ${UPLOAD}/kops/${VERSION}/images/
mkdir -p ${UPLOAD}/utils/${VERSION}/linux/amd64/
Copy link
Member

Choose a reason for hiding this comment

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

Looks like some of the utils.tar.gz-removal PR sneaked in here.

Copy link
Member Author

Choose a reason for hiding this comment

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

That this was not actually used:

kops/Makefile

Lines 321 to 323 in fecec5a

cp ${DIST}/linux/amd64/utils.tar.gz ${UPLOAD}/kops/${VERSION}/linux/amd64/utils.tar.gz
cp ${DIST}/linux/amd64/utils.tar.gz.sha1 ${UPLOAD}/kops/${VERSION}/linux/amd64/utils.tar.gz.sha1
cp ${DIST}/linux/amd64/utils.tar.gz.sha256 ${UPLOAD}/kops/${VERSION}/linux/amd64/utils.tar.gz.sha256

@k8s-ci-robot k8s-ci-robot merged commit 6d33b83 into kubernetes:master Jun 3, 2020
@k8s-ci-robot k8s-ci-robot modified the milestones: v1.19, v1.18 Jun 3, 2020
@hakman hakman deleted the build-arm64 branch June 3, 2020 05:18
@hakman
Copy link
Member Author

hakman commented Jun 3, 2020

@johngmyers any thoughts on trying to remove the .sha1 files from the build? Do we still need those?

@hakman hakman mentioned this pull request Jun 3, 2020
Merged
@johngmyers
Copy link
Member

I've been thinking of ripping out the .sha1 files from the build. I don't think we need them ourselves, but I don't know what might be depending on them downstream.

@hakman
Copy link
Member Author

hakman commented Jun 3, 2020

There shouldn't be anything downstream that cares about the upstream build files, but we can bring it up during office hours.

@hakman hakman mentioned this pull request Jun 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johngmyers johngmyers johngmyers left review comments

@rifelpet rifelpet rifelpet left review comments

@joshbranham joshbranham Awaiting requested review from joshbranham

@robinpercy robinpercy Awaiting requested review from robinpercy

@mikesplain mikesplain Awaiting requested review from mikesplain

Assignees

@justinsb justinsb

@rifelpet rifelpet

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/documentation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.18
Development

Successfully merging this pull request may close these issues.
5 participants
@hakman @justinsb @rifelpet @k8s-ci-robot @johngmyers