Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dmarquez-splunk
Copy link

@dmarquez-splunk dmarquez-splunk commented Apr 11, 2025

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:

pkg_tar (
 name = "ucrt-dll",
 srcs = [":ucrt-dll-files"],
 remap_paths = {
   "Redist/$(SDK_VERSION)/ucrt/DLLs/$(ARCH)" : "bin/"
 },
 toolchains = ["@rules_cc//cc:current_cc_toolchain"]
)

Copy link

google-cla bot commented Apr 11, 2025

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.

@dmarquez-splunk
Copy link
Author

Running into a doc build issue that I'm not certain how to resolve:

/workdir/doc_build/BUILD:65:12: in deps attribute of starlark_doc_extract rule //doc_build:pkg_tar_gen.extract: missing
bzl_library targets for Starlark module(s) @aspect_bazel_lib//lib:expand_make_vars.bzl. Since this rule was created by the 
macro 'stardoc', the error might have been caused by the macro implementation

Any guidance would be greatly appreciated! cc: @cgrindel

@aiuto
Copy link
Collaborator

aiuto commented Apr 14, 2025

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.
What particular value is the other expand_variables doing over pkg/private/tar/util.bzl%substitute_pkg_variables?

@dmarquez-splunk
Copy link
Author

dmarquez-splunk commented Apr 14, 2025

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. What particular value is the other expand_variables doing over pkg/private/tar/util.bzl%substitute_pkg_variables?

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!

@dmarquez-splunk
Copy link
Author

Can I get another review on this MR please? @aiuto @cgrindel

Copy link
Collaborator

@cgrindel cgrindel left a 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")
Copy link
Collaborator

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.

Copy link
Author

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@aiuto
Copy link
Collaborator

aiuto commented Apr 30, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants