-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Ripunzip: use releases from github #20861
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
Conversation
This uses the ripunzip releases from github instead of building them ourselves.
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.
Pull Request Overview
This PR transitions from building ripunzip binaries in-house and storing them in Git LFS to downloading pre-built releases directly from the GoogleChrome/ripunzip GitHub repository. This simplifies the build infrastructure by eliminating the need for a custom build workflow and Git LFS storage.
- Replaces the custom build workflow with direct downloads from GitHub releases
- Implements a new Bazel repository rule that handles platform-specific downloads and extraction
- Updates to ripunzip version 2.0.3 with SHA256 verification for all platforms
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| misc/ripunzip/ripunzip.bzl | New Bazel repository rule implementing platform-specific download and extraction logic for ripunzip releases |
| MODULE.bazel | Replaces LFS-based repositories with the new ripunzip_archive rule, specifying version 2.0.3 and checksums |
| misc/ripunzip/BUILD.bazel | Simplifies alias to reference the unified @ripunzip repository instead of platform-specific repositories |
| misc/ripunzip/BUILD.ripunzip.bazel | Updates glob pattern to match the new bin/ directory structure from GitHub releases |
| .gitattributes | Removes Git LFS configuration for ripunzip binaries that are no longer stored locally |
| .github/workflows/build-ripunzip.yml | Removes the workflow that previously built ripunzip binaries in-house |
| misc/ripunzip/ripunzip-*.tar.zst | Removes locally stored binary archives that are now downloaded on-demand |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
esbena
left a comment
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.
LGTM modulo comments, but CI disagrees.
| output = "deb", | ||
| ) | ||
| repository_ctx.extract( | ||
| "deb/data.tar.xz", | ||
| strip_prefix = "usr", | ||
| ) |
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.
Please add a comment here explaining why output is not bin like for the other os cases. In isolation it is fine, but the inconsistency is confusing.
Related:
It did need a bit of bazel tinkering as they only publish debian packages.
Perhap this should be added as a docstrinb on _impl itself
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, went for stripping usr/bin and then outputting to bin like the rest
|
The CI failure is unrelated to these changes, and fixed in another internal PR |
This uses the ripunzip releases from github instead of building them ourselves.
It did need a bit of bazel tinkering as they only publish debian packages.
This can be tested locally with