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

✨ Update Makefile for go/v3 to support darwin/arm64 #2706

Closed

Conversation

everettraven
Copy link
Contributor

Description of Change

  • Add Bash scripts for building kustomize and etcd locally
  • Add a Makefile target to build etcd locally with make etcd
  • Modify the kustomize Makefile target to build from source locally if running on darwin/arm64 (Darwin as result from uname -s and aarch64 as result from uname -m)

Motivation for Change

There is seemingly more and more issues occurring for users that are using the M1 Macs to build operators with both Kubebuilder and Operator SDK. This change should provide users an easier way to remediate the issues by providing a new Makefile target to build etcd from source and modifying the existing Makefile target for installing kustomize to build from source if running on darwin/arm64.

Additional Context

I do not have an M1 Mac and therefore cannot test this change myself to verify it resolves issues for users on the new M1 Macs. I was however able to test that the scripts added work as expected.

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@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. labels May 31, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @everettraven. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 31, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: everettraven
To complete the pull request process, please assign pwittrock after the PR has been reviewed.
You can assign the PR to them by writing /assign @pwittrock in a comment when ready.

The full list of commands accepted by this bot can be found 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

@everettraven
Copy link
Contributor Author

/cc @camilamacedo86 @ryantking

Do either of you have an M1 Mac and are able to test this and see if it works? If not do you know anyone who may be able to?

Thanks!

@k8s-ci-robot
Copy link
Contributor

@everettraven: GitHub didn't allow me to request PR reviews from the following users: ryantking.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @camilamacedo86 @ryantking

Do either of you have an M1 Mac and are able to test this and see if it works? If not do you know anyone who may be able to?

Thanks!

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.

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Only builds if etcd doesn't already exist on the system

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@@ -179,15 +179,35 @@ KUSTOMIZE_INSTALL_SCRIPT ?= "https://raw.githubusercontent.com/kubernetes-sigs/k
.PHONY: kustomize
kustomize: $(KUSTOMIZE) ## Download kustomize locally if necessary.
$(KUSTOMIZE): $(LOCALBIN)
ifeq "$(uname -s)" "Darwin"
Copy link
Member

@camilamacedo86 camilamacedo86 Jun 8, 2022

Choose a reason for hiding this comment

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

It is hard to keep maintained and not sustainable ( Spaghetti Code )
Also, we cannot do that. We cannot add a script from your repo in the default scaffold.

About etcd bin

The etcd bin is used for ENV TEST and is shipped via the kubebuilder-tools, see:

Also, we are close to having that bin from the project:

As soon we have the binary there we can trigger the job in the cloud to build the kubebuilder-tools for darwin/arm64. To better understand it check the issue: #2664

About kustomize bin

You can follow up: kubernetes-sigs/kustomize#4612
Users will be able to use it with kustomize/v2 and go/v4-alpha, see: #2552

I am not sure if we should assume the decomposability to build kustomize v2 for darwin/arm64 to make it work with go/v3 stable. That will add this kind of complexity to the Makefile and also we would need to properly build it in the cloud as we do with the kubebuilder-tools instead. It seems out of the scope from Kubebuilder itself.

However, we could doc the script for example to allow users to build it by themselves and doc as an option. But the script also needs to leave in the repo or some official place and not in our fork. We would like to have a FAQ section such as we have in SDK for the docs, so I think we could add the info there and link the script. WDYT?

To know more about what is required to support apple silicon and what was done or not so far see: #1932

Copy link
Member

Choose a reason for hiding this comment

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

/hold

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I think at least adding some docs for a workaround would be good so that users can begin using Kubebuilder/Operator SDK to develop operators on apple silicon.

Just as an aside, the goal of this PR is to add those scripts to Kubebuilder as well so that the scripts that are run in the makefile are from the official Kubebuilder repo, I just left it pointing to my fork so we could test that it actually works since Kubebuilder doesn't have the scripts in place yet and would modify it to point to Kubebuilder prior to merging.

If this isn't a direction that we want to explore that is fine and I can close this PR, just thought I would give it a shot at trying to provide a solution that would allow development with go/v3 on Apple Silicon. Thanks for the feedback!

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. I think at least adding some docs for a workaround would be good so that users can begin using Kubebuilder/Operator SDK to develop operators on apple silicon.

  • For the Kubebuilder side we can begin to support Apple Silicon from go/v4-alpha and with kustomize/v2-alpha
  • For SDK we need to discuss how we will do it there? Could we accept the breaking change for ansible/helm/hybrid instead of creating a new plugin or not for example?

Just as an aside, the goal of this PR is to add those scripts to Kubebuilder as well so that the scripts that are run in the makefile are from the official Kubebuilder repo, I just left it pointing to my fork so we could test that it actually works since Kubebuilder doesn't have the scripts in place yet and would modify it to point to Kubebuilder prior to merging.

I think we have some options to address these ones, follow some ideas/suggestion:

Regards the bins used in kubebuilder-tools/env test:

a) We could Improve the kubebuilder-tools scripts for if the bin does not exist we build them for the arch informed so that it can work in the cloud and can be used by the authors. Also, we can improve the doc: https://book.kubebuilder.io/reference/artifacts.html and let the users know about that and that they can use it to build for any platform

Regards kustomize

  • For the next release, we will need to clarify its support with go/v4-alpha in the quickstart probably, I am looking if we can merge ✨(go/v4-alpha): add new golang plugin which uses the default golang base v3 and the new alpha kustomze/v2-alpha #2552 so that we can work on this
  • Also, we could add a new section FAQ as we have in SDK docs ( see that we have many issues track to do that and add the info there ) then, we can have a directory under it called script and add the script that will let users know that they can use to build kustomize v2 for apple silicon or for any other platform that they need.

Regards SDK
After we add the changes here in SDK we can use/ link and redirect users to not need to duplicate the content and be required to keep maintained two places

WDYT? Is that make sense?
Could you shape your scripts so we provide them to the users via the FAQ? Would you like to contribute with the kubebuilder-tools scripts to make them be able to build the project when the bin is not found?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of that makes sense to me. I may need a bit more clarification as I start to do some of those things. I would be happy to help out in any way that I can.

As I have time, I can begin working on updating the kubebuilder-tools scripts to build the tool if necessary. Then I think we can potentially discuss more regarding kustomize and and SDK as work on those solutions progresses.

I will go ahead and close this PR. Thanks @camilamacedo86 !

Copy link
Member

Choose a reason for hiding this comment

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

Sure, if you wish we can track issues with that as well.
and by sure .. feel free to ping if you need any help

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2022
@everettraven
Copy link
Contributor Author

Closing as per discussion in this review comment thread: #2706 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants