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

chore: change binary release artefact names #3997

Merged
merged 3 commits into from
Jul 1, 2023
Merged

Conversation

EnzoPlayer0ne
Copy link
Contributor

@EnzoPlayer0ne EnzoPlayer0ne commented May 23, 2023

Here is a suggested PR to update the name of the release so they follow $(uname -s)-$(uname -m) convention. This would make it easier to fetch the releases from container build files like Dockerfile.

Hence:

  • x86 Linux becomes Linux-x86_64
  • x86 Mac becomes Darwin-x86_64

In the future this would allow for:

  • M1/M2 Mac to become Darwin-arm64
  • arm64 Linux to become Linux-arm64

Which would not have been possible under the current naming scheme which does not differentiate for the different macOS architecture (understandable as from my understanding there are no releases for M1/2 macOS yet).

Note: the legacy naming is still available. This will be discontinued when all downstream consumers have updated their procedures.

@github-actions
Copy link

github-actions bot commented May 23, 2023

Comparing from 14a3ad4 to 917e1f7:
The produced WebAssembly code seems to be completely unchanged.

@chenyan-dfinity
Copy link
Contributor

This would break other CIs which rely on fetching moc from the URL. Not sure if it's worth the breaking change.

@EnzoPlayer0ne
Copy link
Contributor Author

EnzoPlayer0ne commented May 23, 2023

This would break other CIs which rely on fetching moc from the URL. Not sure if it's worth the breaking change.

It would break other CIs for the next releases only, no? If that's the case people would have to update the version manually I'm guessing, so they could update the architectures package name at the same time.

@crusso
Copy link
Contributor

crusso commented May 24, 2023

Not opposed but will break sdk CI and possibly other CIs. We currently use GH runners to produces binaries... AFAIK, there's no M1/M2 runners available yet.

@ggreif
Copy link
Contributor

ggreif commented May 24, 2023

This is going in the right direction, but I doubt it is necessary. Why not have a fat binary for OS X?
OTOH Linux doesn't have those so this would be just a stopgap until ARM becomes significant/dominant there too.

Anyway before changing artefact names we should at least hunt down the main consumers and set up fallback locations for them.

@crusso
Copy link
Contributor

crusso commented May 24, 2023

FTR, SDK seems to adhere to this (see https://github.com/dfinity/sdk/releases) but uses lower-case linux/darwin. Not sure which is "correct".

@crusso
Copy link
Contributor

crusso commented May 24, 2023

I guess we could release under the old and new names to avoid breakage...

@EnzoPlayer0ne
Copy link
Contributor Author

@ggreif I like your idea of fat binaries - however from my understanding it still packs up both architectures inside the fat format. This would require us to compile to both x86-64 and arm64 architectures. Hence we could just skip packaging them together and rather release them separately. On top of fat binaries seem to increase the size of the binary.

@crusso I believe the correct upgrade plan would be to:

  1. now - for the current release nothing changes
  2. have a period where we have both naming conventions for a few releases
  3. switch to only one naming convention where we specify os-arch - this would allow for future arm64 releases for both Linux and Darwin.

@chenyan-dfinity I believe this would not break any CI - as if a CI fetches the latest release version rather than have a pinned version that they update when a new releases comes they already are susceptible to CI issues - they would have to fix their CI but it would be a minimal change.

@EnzoPlayer0ne
Copy link
Contributor Author

@crusso I added back the current format for release name, so next release there should be both the "old" way and "new" way.

@ggreif
Copy link
Contributor

ggreif commented Jun 28, 2023

Drive-by comment: Since GH doesn't offer ARM builders, this would be necessary to build those images: https://www.cachix.org/github-runners-for-nix-nixos

@ggreif ggreif changed the title Update release name chore: change binary release artefact names Jul 1, 2023
Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

Ok by me

@ggreif ggreif added the automerge-squash When ready, merge (using squash) label Jul 1, 2023
@mergify mergify bot merged commit 2cb72c3 into master Jul 1, 2023
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Jul 1, 2023
@mergify mergify bot deleted the update-release-names branch July 1, 2023 17:13
@ggreif
Copy link
Contributor

ggreif commented Jul 5, 2023

@EnzoPlayer0ne the build products are now available, please check: https://github.com/dfinity/motoko/releases/tag/0.9.5

@bitdivine
Copy link
Member

bitdivine commented Jul 5, 2023

Please note that rust crates can be installed by compiling, with cargo install XXX, or as a pre-built binary with cargo binstall XXX For cargo binstall to work, the builds are typically named according to one of these patterns: https://github.com/cargo-bins/cargo-binstall/blob/main/SUPPORT.md#defaults

If you don't like the standard patterns, it is possible to define another pattern, as described at the beginning of the doc: https://github.com/cargo-bins/cargo-binstall/blob/main/SUPPORT.md Given that our releases already fix a version, we aren't putting all builds into one big directory, I have been using the first pattern that does not specify a version: { name }-{ target }. Thus e.g. for idl2json there are two builds: https://github.com/dfinity/idl2json/releases/tag/v0.8.8 and I can install with:

max@sinkpad:/ (1:26)$ cargo binstall idl2json_cli@0.8.8
 INFO idl2json_cli v0.8.8 is already installed, use --force to override
 INFO Done in 1.560194ms
max@sinkpad:~/ (29)$ cargo binstall --strategies crate-meta-data idl2json_cli --force --no-confirm
 INFO resolve: Resolving package: 'idl2json_cli'
 WARN The package idl2json_cli v0.8.8 will be downloaded from github.com
 INFO This will install the following binaries:
 INFO   - idl2json (idl2json -> /home/max/.cargo/bin/idl2json)
 INFO Installing binaries...
 INFO Done in 1.882651139s

Just a thought. If you want a standard, there are already some to choose from. :-)

mergify bot pushed a commit that referenced this pull request Jul 18, 2023
This moves forward the name normalisation that was commenced in #3997.
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