-
Notifications
You must be signed in to change notification settings - Fork 201
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
build: add support to build linux/mac arm64 #1658
Conversation
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.
Thanks for the PR!
Re: arm64 Azure Pipelines agent -- it doesn't appear there are official agents available yet, so I believe a self-hosted solution would be needed (looking at https://docs.microsoft.com/en-us/azure/devops/release-notes/2020/sprint-171-update#additional-agent-platform-arm64). I know @carolynvs has previously set up a self-hosted Windows agent, so she may have a good idea around level of effort for an arm64 agent.
Signed-off-by: Carlos Panato <ctadeu@gmail.com>
@vdice thanks for your review! |
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.
Let's please hold off on merging until we can further think through how to treat the runtime binary.
|
||
curl -fsSLo $PORTER_HOME/porter $PORTER_MIRROR/$PORTER_PERMALINK/porter-linux-arm64 | ||
chmod +x $PORTER_HOME/porter | ||
cp $PORTER_HOME/porter $PORTER_HOME/runtimes/porter-runtime |
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.
Have you tested out using porter-linux-arm64 installed to ~/.porter/runtimes/porter-runtime? Like built a bundle and run it?
I'm not sure that will work without further changes because we build bundles with a linux amd64 invocation image.
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.
sorry for the delay to reply. No, but I will test, just need to setup a linux-arm64 machine, thanks for the review and feedback!
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.
Just checking if you tried this out yet and how it went?
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.
@carolynvs sorry :( i was busy with company work and did not have time, I will take some days off and when I comeback i will get this back on track. I will be out starting today till next Tuesday and on Wednesday I will run all the tests on this.
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.
No need to apologize! I just ping issues after it's been a while to see how things are going and make sure that people aren't stuck. Take your time and I appreciate you looking into this. 👍
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.
published the exec
binary but looks like it tried to get the linux/amd64
ubuntu@ip-172-31-10-226:~/porter/porter$ porter mixin install exec --url https://github.com/cpanato/testing-ci-providers/releases/download/ --version v0.0.51
Error: bad status returned when downloading https://github.com/cpanato/testing-ci-providers/releases/download/v0.0.51/exec-linux-amd64 (404) 404 Not Found
which I did not publish to check if will download the right one
but my question is (I'm a newbie here, so apolagises)
the mixin exec needs to be in the same arch as we are running? if so we might need to change the code around https://github.com/getporter/porter/blob/main/pkg/pkgmgmt/client/install.go#L91
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.
made the change and then when I run it again apparently it works
ubuntu@ip-172-31-10-226:~/porter/porter$ porter mixin install exec --url https://github.com/cpanato/testing-ci-providers/releases/download/ --version v0.0.51
installed exec mixin v0.38.3-11-g6f9ae84e (6f9ae84e)
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.
ahh! we will need to build the other mixins for the others like kubernetes/helm ... I get it now
sorry doing some rubber duck here :)
if this is something we want, I'm willing to open the changes in the other repos
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.
Yes that would be great! 💯
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.
ok, will work on those things 😃
@@ -0,0 +1,38 @@ | |||
#!/usr/bin/env bash |
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 duplicating scripts for each os+arch, since I think the only difference is the arch name, I'd like for us to look at using go env GOARCH
to determine the arch value and then use that as a variable. Then we just have to maintain 3 install scripts.
goos := goos | ||
arch := arch | ||
// skip windows arm64 | ||
if arch == "arm64" && goos == "windows" { |
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 Go 1.17 we should be able to remove this skip here. Can you add a comment about that and link to the issue for Go windows/arm64 support? That way we are more likely to remember. 😀
This is a great PR to reference in #2870 - however due to age I'm going to close it until we can readdress it in that issue. |
What does this change
This PR adds build support to build the binaries for Linux/ARM64 and Mac/ARM64
Did not touch on the
azure-pipelines.install.yml
because I don't know if there is a pool for ARM VMs running.Please let me know your thoughts on this and if this is not aligned with the roadmap feel free to close.
Also if someone is running Linux and/or Mac on ARM64 and what to give a try will be cool
What issue does it fix
Closes #1497
If there is not an existing issue, please make sure we have context on why this change is needed. See our Contributing Guide for examples of when an existing issue isn't necessary.
Notes for the reviewer
Put any questions or notes for the reviewer here.
Checklist
If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇♀️