Genericize package manifest system and interface#660
Merged
nacl merged 6 commits intobazelbuild:mainfrom Feb 8, 2023
Merged
Conversation
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.
aiuto
approved these changes
Feb 8, 2023
Collaborator
aiuto
left a comment
aiuto
left a comment
There was a problem hiding this comment.
Sorry this took so long. It landed just before a crunch time.
I love PRs like this. All sorts of plumbling is different, but the tests remain the same.
Collaborator
Author
No worries, thanks for taking a look! |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.