Genericize package manifest system and interface #660
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The current way that rules_pkg communicates with must packagers is using a manifest file, which is currently a JSON data structure based on a an array of arrays. While generally readable, it looks strange, as it was to reduce Bazel resource usage (JSON strings in memory). Further, our Python code is directly bound to this data structure format. However, if we want to add more or change this, it becomes cumbersome on both the Starlark and Python sides. This change alleviates concerns generally by: - Converting all manifests to a JSON "object" style, improving readability. Numerous "golden" tests were updated to support this. - Replace the `collections.namedtuple`-based `ManifestEntry` object with one that is a little more flexible and type-safe. - Providing a function (`read_entries_from`) that converts a file-like object into a list of `ManifestEntry`s, and replacing all JSON reading in packagers (`tar`, `zip`, `install`) and their tests with this function. Other convenience factors or things addressed: - `ManifestEntry.entry_type` is now just `ManifestEntry.type` - Bazel 6 now stringifies repository-local labels with a preceding `@`, unlike prior versions. Adapt to this in the manifest writer. Future changes will extend this interface to allow for custom attributes to be passed from `pkg_files` and friends, which, among other things, will be necessary to more generically support `pkg_rpm`. Provides groundwork for (but doesn't resolve) bazelbuild#385.
added 5 commits
January 17, 2023 10:11
aiuto
approved these changes
Feb 8, 2023
No worries, thanks for taking a look! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The current way that rules_pkg communicates with must packagers is using a manifest file, which is currently a JSON data structure based on a an array of arrays. While generally readable, it looks strange, as it was to reduce Bazel resource usage (JSON strings in memory). Further, our Python code is directly bound to this data structure format.
However, if we want to add more or change this, it becomes cumbersome on both the Starlark and Python sides. This change alleviates concerns generally by:
collections.namedtuple-basedManifestEntryobject with one that is a little more flexible and type-safe.read_entries_from) that converts a file-like object into a list ofManifestEntrys, and replacing all JSON reading in packagers (tar,zip,install) and their tests with this function. While this is theoretically usable by consumers of rules_pkg, it is not intended to be supported as such.Other convenience factors or things addressed:
ManifestEntry.entry_typeis now justManifestEntry.type@, unlike prior versions. Adapt to this in the manifest writer.Future changes will extend this interface to allow for custom attributes to be passed from
pkg_filesand friends, which, among other things, will be necessary to more generically supportpkg_rpm.Provides some groundwork for (but doesn't resolve) #385.