-
Notifications
You must be signed in to change notification settings - Fork 14
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
#nojira need to distinguish between arm64 mac vs x86_64 mac #31
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
414546f
to
3e2c045
Compare
rojo = { source = "Roblox/rojo", version = "0.6.0-alpha.1" } | ||
selene = { source = "Kampfkarren/selene", version = "0.15.0" } |
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.
if I don't have this here, the test with working directory fails: https://github.com/Roblox/setup-foreman/runs/7569087575?check_suite_focus=true#step:8:21
@amatosov-rbx is that the expected behavior?
I have read the CLA Document and I hereby sign the CLA |
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.
Looks like it should be okay, but I think it needs a bit more backwards compatibility
platformMatcher = name => name.includes("macos-x86_64"); | ||
} else { | ||
platformMatcher = name => name.includes("macos-arm64"); | ||
} |
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.
We may still need some sort of fallback here since there are older projects with just a macos
asset. I presume that would likely be only x86_64 ones anyway, so maybe the x64 case needs to include a || name.includes("macos")
or something?
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.
Oh wait, this is only for foreman itself, huh. I wonder if we can retroactively fix up the asset names?
setup-foreman fails to run on legacy Mac as it was trying to download and run arm64 binary onto x64 machines