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

feat: add OS X ARM download url #795

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

pjurczynski
Copy link

@pjurczynski pjurczynski commented Aug 7, 2024

Changes proposed by this PR

Addresses: https://pix4dbug.atlassian.net/browse/PCI-3893

Add OS X ARM option for download.

Notes to reviewer

chrome_MvNfSCVsfb

@pjurczynski pjurczynski self-assigned this Aug 7, 2024
@marco-m-pix4d
Copy link

marco-m-pix4d commented Aug 7, 2024

WOW THANKS Piotr !!!
Hello @pjurczynski , FYI I stumbled upon this PR by chance: since it is in Draft mode, we (PCI) did not get any notification.
What is its status?

UPDATE

Could you please set our pipeline? Currently it is a bit involved, see https://github.com/Pix4D/ci-for-concourse and just the two sections "Setup" and "Developing the fork". Thanks.

Copy link

@odormond odormond left a comment

Choose a reason for hiding this comment

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

Wow! Looks good. 😍

Copy link

@SpectreVert SpectreVert left a comment

Choose a reason for hiding this comment

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

Love the ARM Apple logo 💪

@marco-m-pix4d
Copy link

marco-m-pix4d commented Aug 7, 2024

What happens if the fly ARM binary is not there ?
Said in another way: upstream doesn't build ARM. If we propose this PR upstream and they merge it as-is, will it just ignore the missing binary, or does it assumes that the binary is there?

@pjurczynski
Copy link
Author

What happens if the fly ARM binary is not there ? Said in another way: upstream doesn't build ARM. If we propose this PR upstream and they merge it as-is, will it just ignore the missing binary, or does it assumes that the binary is there?

That's the part why it's still in the draft. So There's no check whatsoever in the code, it just displays the download link as it's all hardcoded in the elm. I'm trying to figure out what kind of logic we could add so that it could be merged with upstream as well.

In the ticket it was mentioned that we could write a logic around the embedded fly.. but I'm not sure that's the best. Another idea would be to maybe rely on some env configuration that can be passed (and if not, it just defaults to what was there currently).

There's also the part about the logo - I'm happy that @odormond likes it, but it cannot be merged like so to the upstream 🙃 .. even though it's fun, we probably can't afford customizing original logos in something that is public.

@odormond
Copy link

odormond commented Aug 7, 2024

What happens if the fly ARM binary is not there ? Said in another way: upstream doesn't build ARM. If we propose this PR upstream and they merge it as-is, will it just ignore the missing binary, or does it assumes that the binary is there?

The download link would be broken but I guess that's actually true of any of the fly executable.

@odormond
Copy link

odormond commented Aug 7, 2024

There's also the part about the logo - I'm happy that @odormond likes it, but it cannot be merged like so to the upstream 🙃 .. even though it's fun, we probably can't afford customizing original logos in something that is public.

Good point.

Also, upstream might not want to accept the change anyway. I'm actually thinking that the proper solution is not to add a mac arm download link but to make fly universal2. In my understanding this should be as simple as running lipo -create -output fly-universal2 fly-amd64 fly-arm64.

@marco-m-pix4d
Copy link

In my understanding this should be as simple as running lipo -create -output fly-universal2 fly-amd64 fly-arm64.

The thing is, in our CI, thanks to Go, we just cross-build fly from Linux AMD64 to macOS ARM64 (sure we could change it...)

@odormond
Copy link

odormond commented Aug 8, 2024

In my understanding this should be as simple as running lipo -create -output fly-universal2 fly-amd64 fly-arm64.

The thing is, in our CI, thanks to Go, we just cross-build fly from Linux AMD64 to macOS ARM64 (sure we could change it...)

Maybe we should try using: https://github.com/konoui/lipo

@marco-m-pix4d
Copy link

Maybe we should try using: https://github.com/konoui/lipo

AH! This is nice!

@@ -13,6 +13,7 @@ all =
List.map downloadUrl clis
|> Expect.equal
[ "/api/v1/cli?arch=amd64&platform=darwin"
, "/api/v1/cli?arch=arm&platform=darwin"

Choose a reason for hiding this comment

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

this should be arm64

fly-darwin-amd64.tgz fly-darwin-arm64.tgz fly-linux-amd64.tgz fly-windows-amd64.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants