feat(wheel): Add support for add_path_prefix#3679
feat(wheel): Add support for add_path_prefix#3679antoniojkim wants to merge 2 commits intobazel-contrib:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new add_path_prefix argument to the py_wheel rule, enabling users to prepend a specific path to files within the generated wheel after any path stripping has occurred. The changes span the Bazel rule definition, the underlying wheelmaker.py utility, and include updated tests and documentation. Feedback highlights that the current implementation relies on raw string concatenation for the prefix, which is fragile and can result in malformed paths (e.g., missing or double slashes) depending on the specific strings provided for stripping and prepending.
| # An example of how to change the wheel package root directory using 'add_path_prefix'. | ||
| py_wheel( | ||
| name = "custom_prefix_package_root", | ||
| add_path_prefix = "custom_prefix", |
There was a problem hiding this comment.
The add_path_prefix value here is set to "custom_prefix" (without a trailing slash), which differs from the examples in the py_wheel documentation (e.g., "foo/").
This works in this specific case because strip_path_prefixes = ["examples"] leaves a leading slash in the path (e.g., /wheel/main.py), resulting in custom_prefix/wheel/main.py. However, if no stripping occurred or if the prefix in strip_path_prefixes included a trailing slash, the result would be an invalid path like custom_prefixwheel/main.py.
To ensure robustness and clarity, it is recommended to make the examples consistent or explicitly handle the path separator in the implementation.
There was a problem hiding this comment.
This is intentional so that you can actually prepend without a slash, e.g.
src/module -> my_src/module
| return add_path_prefix + normalized_arcname[len(prefix) :] | ||
|
|
||
| return normalized_arcname | ||
| return add_path_prefix + normalized_arcname |
There was a problem hiding this comment.
The implementation uses raw string concatenation for add_path_prefix. This is fragile because it depends on the exact string returned after stripping prefixes.
As seen in the tests and examples, stripping a directory name without a trailing slash (e.g., "examples") leaves a leading slash (e.g., /foo.py), while stripping with a trailing slash (e.g., "examples/") does not. This forces the user to know the internal state of the path to decide whether add_path_prefix should include a trailing slash.
Consider using a more robust path joining method or normalizing slashes to ensure a single / separator is always present when add_path_prefix is intended as a directory.
There was a problem hiding this comment.
This is intentional so that you can actually prepend without a slash, e.g.
src/module -> my_src/module
f4d710a to
b1bdf0d
Compare
aignas
left a comment
There was a problem hiding this comment.
Thank you for the contribution!
| For example: | ||
| + `"foo/" will prepend to `"bar/baz/file.py"` as `"foo/bar/baz/file.py"` | ||
| + `"foo_" will prepend to `"bar/baz/file.py"` as `"foo_bar/baz/file.py"` | ||
| + `stripping ["bar/"] and adding "foo/" will change `"bar/baz/file.py"` to `"foo/baz/file.py"` |
There was a problem hiding this comment.
| + `stripping ["bar/"] and adding "foo/" will change `"bar/baz/file.py"` to `"foo/baz/file.py"` | |
| + `stripping ["bar/"] and adding "foo/" will change `"bar/baz/file.py"` to `"foo/baz/file.py"` | |
| :::{versionadded} VERSION_NEXT_FEATURE | |
| The {attr}`add_path_prefix` attribute was added. | |
| ::: |
| "a/b/c/file.py", | ||
| "", | ||
| ["a/b/"], | ||
| "namespace_", |
There was a problem hiding this comment.
I wonder if supporting this is actually error prone. Maybe it would be best to tell users to use:
strip = ["a/b/c"]
add = "namespace_c"
This way we can keep the logic simpler.
| ("mylib/a/b/c/WHEEL", "mylib", ["mylib"], "", "mylib/a/b/c/WHEEL"), | ||
| # Check that prefixes are added | ||
| ("a/b/c/file.py", "", [], "namespace/", "namespace/a/b/c/file.py"), | ||
| ("a/b/c/file.py", "", ["a"], "namespace", "namespace/b/c/file.py"), |
There was a problem hiding this comment.
I know this may be a big ask, but it would be nice to change this to a list of dicts or namedtuples so that it is easier to understand the parameters in the test.
This change is being made to support prepending a prefix to the file paths in the wheel. This is useful for customizing the import path for the package. For example, if your code is implemented in
src/moduleyou may want to package this code for distribution asnamespace/moduleso that the import isI think ideally, this should have been implemented as
remap_path_prefixwhich is a map which specifies which prefixes should be changed and what to change them to. However, seeing asstrip_path_prefixesalready exists, I thought the simpler thing to do here was to just add theadd_path_prefixargument.Implements #515
Tests
Tested by building and manually inspecting the wheels generated by
More specifically