Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 windows msi for arm and amd #1796
build windows msi for arm and amd #1796
Changes from 4 commits
3b59fd8
0aff250
eca6558
9a49d63
0b736a3
2fbeac3
926987b
5f89750
5dfec02
80c3def
e840e20
5c90381
910cabb
de4544b
832ddf1
2f739af
10314cc
ed806ee
163ca6a
8293631
53ad5c3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I see this is what we're doing everywhere here, I wonder if we should consolidate and only open it once, instead of the rapid opening and closing. (If we think so, it's fine as a followup)
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.
Is this future-proofing in case we want to do more granular releases for launcher in the future (e.g. "bug in launcher version x.y.z on ARM only, so leave stable at x.y.z for all other arches and x.y.y for launcher ARM")? Or are we adding this flag for another reason?
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.
I added this just so that I could specify a local arm build to package. I can't imagine a situation where we would want the arm and amd binaries to have different code.
This has me thinking though that maybe we should add in some safe guards to ensure the arm and amd versions are the same. If we some how managed to get into a situation where TUF updated one and not the other, we could end up with a package where different arches are on different versions.
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.
I can't find a coherent way to verify the versions are the same. However, I set up the flag so that it's default is an empty string and it will just set that flag to what ever
launcher_version
is if nolauncher_arm_version
is provided.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.
I'm not sure there's a good way to validate a launcher version, without invoking it. Maaaybe we can string parse it, but I'm not sure it's worth it. (And given what Zack recently learned about file versions and MSIs, let's not go there...)
Maybe this is an argument for being less clever. Command line either takes
launcher_version=
and downloads from TUF (we should have enough metadata there to validate thatstable
is the same) Oruse_local_binary=...
and it ignores TUFThere 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.
I guess that's kinda what would happen. So maybe I'm over thinking it. Maybe we validate the versions from tuf, and skip that validation on local paths.
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.
invoking it could be troublesome since we'll have 2 binaries with to different arches, An arm64 machine will probably emulate the amd64 binary and run it, but the reverse doesn't work.
I think just having separate flags for paths is probably good with only a single option for
launcher_version
which is downloaded form tuf for both arches.