-
Notifications
You must be signed in to change notification settings - Fork 195
Feature: Enable variable expansion for remap_paths in pkg_tar (#947) #950
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Running into a doc build issue that I'm not certain how to resolve:
Any guidance would be greatly appreciated! cc: @cgrindel |
You have to pull the dependency on the expand vars stuff into doc_build/BUILD. But, I really don't want to take dependencies on other packages from here. |
I haven't experimented with the substitute_pkg_variables functions yet. But expand_make_variables in aspect bazel supports expanding pre-defined make variables, pre-defined gen rule variables, and custom make variables. In our use case we have MSVC and other windows SDK variables defined within our windows toolchain that need to be interpolated in order for our builds to package proper versions of windows SDK libraries (like UCRT and MSVCRT). expand_variables seems to support it out of the box which at first glance seemed like the best path forward. Open to hearing alternative solutions if you think substitute package variables is a viable alternative! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am ambivalent about pulling in aspect_bazel_lib. I will leave that decision to @aiuto. If we do keep it, please see my PR comment. Something does not seem right.
@@ -26,6 +26,7 @@ local_path_override( | |||
bazel_dep(name = "rules_license", version = "1.0.0") | |||
bazel_dep(name = "rules_python", version = "1.0.0") | |||
bazel_dep(name = "bazel_skylib", version = "1.7.1") | |||
bazel_dep(name = "aspect_bazel_lib", version = "2.14.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this need to be added to this test workspace? It is already included in rules_pkg
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me try and build the tests without this. I assumed it was required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
Sorry for slow responses. I am in the middle of my last few weeks at Google. Not leaving Bazel community though. Will have time to look at this starting May 5. |
These changes allow for variable expansion in remap_paths in the pkg_tar rule. This will allow the use of bazel pre-defined variables, subset of genrule predefined variables, and toolchain custom make variables in key-value pairs of remap_paths attribute in rule pkg_tar.
Example usage with custom windows toolchain windows sdk version and target platform architecture make variable: