Hierarchical pkg_filegroup implementation mockup #212
Closed
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
This change is intended to start a more detailed discussion on how we might make a "nestable" `pkg_filegroup` rule. The mechanism for calling rules out in other rules was tested and is known to work, but I wonder about what to do with the `name` attribute. A thought that comes to mind is that `pkg_file` could emit a specification that a (currently nonexistent) `pkg_filegroup` rule could use to provide a name to a "real" `pkg_file` rule (or however it is implemented).
This was referenced Aug 4, 2020
|
Sorry for the slow response. I got busy with other responsibilities. I'll try to look in the next couple of days. |
|
The 5 minute impression is that this needs a glob() in one of the sample cases to show how that behaves. |
|
I'm still waiting to come out from under a deluge of work. It's annual
performance review season and that has put me off code for a couple of
weeks.
…On Tue, Aug 4, 2020 at 3:47 PM Andrew Psaltis ***@***.***> wrote:
@nacl <https://github.com/nacl> requested your review on: #212
<#212> Hierarchical
pkg_filegroup implementation mockup.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#212 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXHHHGWQMF2ALOJHK6ZQZDR7BQVHANCNFSM4PUXT6UQ>
.
|
- Hierarchical PFG example now moved to its own directory (pkg/experimental/hierarchical/example), now with (empty) example files. - Hierarchical PFG example has a (little) documentation - Hierarchical PFG actually compiles now. Basic tree data structure traversal function implemented and used; prints outputs to the terminal as it's run.
|
Just posted an update that makes the code runnable. It doesn't support much, but demonstrates how the file mapping mechanism might work. I'm imagining things like globs will work similar to the existing With this sort of scheme, I think we may be able to change the name of |
|
Collisions to the final destination path of a package should fail hard. It is the most straightforward thing we can do. You're right that pkg_filegroup is a strange name. bug pkg_dir is a misnomer too, because it can include things globed in from subdirs. It is occuring to me that we should brainstorm on this in a shard document rather than trying to make code work. A meeting room and a whiteboard would help too. The big questions.
I think the pro outweighs the con, but I am not sure. This might not even be legal syntax for Starlark. |
nacl
pushed a commit
to nacl/rules_pkg
that referenced
this pull request
Sep 25, 2020
--- In using `pkg_filegroup` in our codebases, we've found that it takes a significant amount of vertical space for complex packages. For simple mappings (e.g. copy a target/file to a destination), we've found that a simple tabular format provides a succinct way to specify contents without sacrificing much in the way of readability. At this time, I'm looking for ideas and discussions related to the patterns used here. The idea itself is essentially agnostic to the implementation of `pkg_filegroup`, but `pkg_filegroup` needs some changes in order to make the usage below more idiomatic: there's currently no means to consolidate multiple differing `pkg_filegroup`-like rules into a single rule. A potential idea for this is discussed in bazelbuild#212. --- This change provides a simple csv-like mechanism for specifying package contents. Instead of specifying multiple `pkg_filegroup`s, you can instead say something like: ```python manifest = [ ("/usr/bin/foo", "copy", "unix=0755", ":foo"), ("/etc/foo.d", "mkdir" "unix=0755", ""), ... ] manifest_rules = pkg_list_manifest( name = "manifest", manifest = manifest, default_attrs = "user=root;group=root", ) pkg_rpm( name = "my-rpm", data = manifest_rules + [ # Complex pkg_filegroup rules here ], ... ) ``` Which, for simple install-and-create operations, is enough and is much easier to read. See the in-file documentation in `pkg/experimental/manifest/manifest.bzl` for more details on how the manifest is intended to be structured. Tests were also provided, runnable as `//experimental/manifest:manifest_test`. They are currently incomplete, but are enough to prove basic correctness so far. The one thing that is not yet clear to us is how we should specify and order the manifests. Some potential examples are provided in `pkg/experimental/manifest/examples`, with further information described in `pkg/experimental/manifest/examples/README.md`.
nacl
pushed a commit
to nacl/rules_pkg
that referenced
this pull request
Sep 25, 2020
--- In using `pkg_filegroup` in our codebases, we've found that it takes a significant amount of vertical space for complex packages. For simple mappings (e.g. copy a target/file to a destination), we've found that a simple tabular format provides a succinct way to specify contents without sacrificing much in the way of readability. At this time, I'm looking to start a discussion on the idea presented here. The idea itself is essentially agnostic to the implementation of `pkg_filegroup`, but `pkg_filegroup` needs some changes in order to make the usage below more idiomatic: there's currently no means to consolidate multiple differing `pkg_filegroup`-like rules into a single rule. A potential idea for this is discussed in bazelbuild#212. --- This change provides a simple csv-like mechanism for specifying package contents. Instead of specifying multiple `pkg_filegroup`s, you can instead say something like: ```python manifest = [ ("/usr/bin/foo", "copy", "unix=0755", ":foo"), ("/etc/foo.d", "mkdir" "unix=0755", ""), ... ] manifest_rules = pkg_list_manifest( name = "manifest", manifest = manifest, default_attrs = "user=root;group=root", ) pkg_rpm( name = "my-rpm", data = manifest_rules + [ # Complex pkg_filegroup rules here ], ... ) ``` Which, for simple install-and-create operations, is enough and is much easier to read. See the in-file documentation in `pkg/experimental/manifest/manifest.bzl` for more details on how the manifest is intended to be structured. Tests were also provided, runnable as `//experimental/manifest:manifest_test`. They are currently incomplete, but are enough to prove basic correctness so far. The one thing that is not yet clear to us is how we should specify and order the manifests. Some potential examples are provided in `pkg/experimental/manifest/examples`, with further information described in `pkg/experimental/manifest/examples/README.md`.
|
Whoops.... I did not mean to close this. github is not letting me reopen. I don't understand why. |
|
Ah.... I see it. The PR is against the master branch instead of main. It may need a rebase. |
|
Hm. Apparently I can't change the destination branch either, like I can with an already open PR. Sigh. I'll open up a new PR for this. |
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.
This change is intended to start a discussion on how we might make a "nestable"
pkg_filegrouprule, as @aiuto mentioned in the review forpkg_mklinks.The mechanism for calling rules out in other rules was tested and is known to work, but I wonder about what to do with the
nameattribute.A thought that comes to mind is that
pkg_filecould emit a specification that a (currently nonexistent)pkg_filegrouprule could use to provide a name to a "real"pkg_filerule (or however it is implemented).