Feature: Enable variable expansion for remap_paths in pkg_tar (#947)

[dmarquez-splunk]

These changes allow for variable expansion in remap_paths and modes attributes 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/"
 },
modes = {
  "Redist/$(SDK_VERSION)/ucrt/DLLs/$(ARCH)/<some_file>": "0444"
}
 toolchains = ["@rules_cc//cc:current_cc_toolchain"]
)

[google-cla]

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]

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]

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]

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]

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

[cgrindel]

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.

[cgrindel]

Why did this need to be added to this test workspace? It is already included in rules_pkg.

[dmarquez-splunk]

Let me try and build the tests without this. I assumed it was required

[dmarquez-splunk]

Removed

[aiuto]

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.

[dmarquez-splunk]

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.

@aiuto how do you feel about this? I am interested to hear your thoughts before I proceed writing tests & fixing the build

[dmarquez-splunk]

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.

I am open to writing a new implementation similar to expand_variables in the aspect_bazel_lib. My first inclination was to just use a pre-existing implementation but can look into writing an alternative if thats what we think is best. Just looking for a little direction here