Make the default behavior of pkg_files#strip_prefix more intuitive; rename it to srcs_strip_prefix #656
Conversation
- `strip_prefix#files_only()` is now `strip_prefix#flatten()` - `pkg_files#strip_prefix` is now `pkg_files#srcs_strip_prefix` - The default for (now) `pkg_files#srcs_strip_prefix` is now to strip prefix from the package root instead of flattening.
This script migrates BUILD files to the new behavior. It is written using libCST, which provides concrete syntax tree functionality for python -- in particular, it knows how to preserve whitespace.
The migration script, due to its relative simplicity, only knows how to operate on BUILD files. This change provides analogous changes within our macros, which are mostly used in our unit tests.
This is the most substantial part of the change -- we automatically update all of rules_pkg to account for this new behavior.
Examples should reflect how you should do things, and specifying `srcs_strip_prefix` everywhere in the intuitive case is no longer necessary.
|
This seems to be different from what I thought we agreed on, which was that strip prefix would strip from root or package relative, and flatten was a separate attribute to flatten paths after the stripping. Also, quick side question. does strip_prefix='.' still do the right thing, which is to strip the package and not flatten the rest. |
| @@ -337,19 +337,18 @@ pkg_files = rule( | |||
| """, | |||
| default = "", | |||
| ), | |||
| "strip_prefix": attr.string( | |||
| doc = """What prefix of a file's path to discard prior to installation. | |||
| "srcs_strip_prefix": attr.string( | |||
There was a problem hiding this comment.
Do we really have to do this rename? It will force incompatibility between versions.
That will basically mean I'll have to fork this for Google to keep the old naming for a very long time.
There was a problem hiding this comment.
Do we really have to do this rename?
It is not necessary. It came up in one of our older discussions that it may be nonobvious what strip_prefix refers to.
It will force incompatibility between versions.
That will basically mean I'll have to fork this for Google to keep the old naming for a very long time.
Even if we didn't make this change, there is still an incompatibility, as the default has changed. It may be easier without this attribute change, though.
Also, FWIW, theoretically a vast majority of the use cases can be taken care of using the migration script or something like it, but it may be trickier than it looks.
| @@ -82,6 +83,7 @@ pkg_files( | |||
| srcs = [ | |||
| "mappings_test.bzl", | |||
| ], | |||
| srcs_strip_prefix = strip_prefix.flatten(), | |||
There was a problem hiding this comment.
Hm. Need to fix indentation here.
I think we were thinking different things when we came to an agreement on this in December. Could you provide an example of what you intend the behavior to be with the separate
I believe so; that code hasn't changed. |
pkg_files#strip_prefixprovides the capability to remove path segments from files prior to them entering the output package. The default for this attribute is to flatten the outputs (e.g. remove all directory structure), which is potentially confusing to users and can cause odd problems. What is generally more expected (and error-prone) is to preserve the package-relative path, as specified when you passstrip_prefix.from_pkg()topkg_files,This change implements this update to the default, and implements the following additional changes:
pkg_files#strip_prefixis nowpkg_files#srcs_strip_prefix-- making it more clear whatstrip_prefixis actually applying tostrip_prefix#files_only(which flattens the directory structure) is nowstrip_prefix#flatten.Given that this is a breaking change, we have provided a migration script that users can run to implement this change in their trees. I have partially tested this in an internal tree, and the outputs are identical. This script uses libcst to process the BUILD files and implement transformations. Updates to .bzl files were not attempted due to complexity requirements; users will have to apply them manually.
All BUILD changes were implemented by applying the script like so:
bazel runthe above target for more details. It can be run as@rules_pkg//migration:strip_prefixif the appropriate WORKSPACE stuffs are added.Fixes #354. Supplants #492.
TODO: provide instructions for migrating without downloading rules_pkg -- copying and tweaking some of the stuff added to the WORKSPACE file should be enough.