added the ability to specify a custom wheel package root #200
Conversation
experimental/python/wheel.bzl
Outdated
| @@ -242,6 +243,10 @@ to refer to the package in other packages' dependencies. | ||
| "license": attr.string(default = ""), | ||
| "classifiers": attr.string_list(), | ||
| "description_file": attr.label(allow_single_file = True), | ||
| "package_root_path": attr.string( |
There was a problem hiding this comment.
I'd suggest renaming this to something like "strip_path_prefix" to make it more explicit what happens here. In particular, I'd read package_root_path as path to prepend (there's a request to do that as well).
In addition, perhaps it would make sense to make this a list so that multiple prefixes to strip could be provided?
There was a problem hiding this comment.
Pay attention to the fact that the order of prefixes is important. I created separate tests to show that and documented it in the wheelmaker argument and hinted that in the rule attribute description.
| else: | ||
| return path | ||
|
|
||
| resolved_package_filename = package_filename \ |
There was a problem hiding this comment.
Using backslashes for line continuaiton is frowned upon.
Can you change strip_prefix to be no-op when prefix is None and call it unconditionally?
| @@ -254,7 +268,9 @@ def main(): | ||
| python_tag=arguments.python_tag, | ||
| abi=arguments.abi, | ||
| platform=arguments.platform, | ||
| outfile=arguments.out) as maker: | ||
| outfile=arguments.out, | ||
| package_root_path=arguments.package_root_path, |
There was a problem hiding this comment.
If I recall correctly having trailing comma on argument list is not supported on all python versions. Please remove it.
| # Always use unix path separators. | ||
| arcname = package_filename.replace(os.path.sep, '/') | ||
| arcname = resolved_package_filename.replace(os.path.sep, '/') |
There was a problem hiding this comment.
Shouldn't this happen before strip_prefix? I'd suppose BUILD file would specify the prefix in unix style regardless of build platform, so stripping would need to use '/' separator to work reliably on windows.
There was a problem hiding this comment.
Correct! Fixed.
I incorporated all the logic into one function that returns the final arcname in order to avoid another variable that can be accidentally used instead of the correct one.
…ordered list of prefixes to strip from package file paths
|
Looks good to me. |
|
You should be able to "Squash and merge" the change now. |
@pstradomski I don't have any of the merge options, only close and comment. |
|
@sha1n Weird. Merged it for you. |
|
Merging PRs requires write permission, which Pawel has in this repository (for ownership of the rules under the experimental dir). |
This PR suggests a possible way to address the issues described in #199. I added an optional attribute
package_root_pathto thepy_wheelrule, that specifies the root of the package, relative to the workspace path.This patch only fixes the problem for cases in which the entire package is under the same path. If the package bundles dependencies from other paths, those will not be fixed. It would be nice if we could define the prefix on the sources and strip the prefix when we create the final package. Not sure how to do that and will be very happy to hear suggestions, or alternative approaches.
Thanks.