Fix make substitution in starlark cc_test env

[dflems]

#18882 introduced an issue where nested make variable substitution would fail under cc_test. The change to the starlark tests fails before the change in cc_helper.bzl and passes after.

cc_test(
    name = "test",
    srcs = ["test.cc"],
    data = ["data.txt"],
    env = {
        "DATA_LOCATION": "$(location :data.txt)",
    }
)

ERROR: /playground/BUILD.bazel:1:8: in cc_test rule //:test:
Traceback (most recent call last):
	File "/virtual_builtins_bzl/common/cc/cc_test.bzl", line 70, column 29, in _impl
	File "/virtual_builtins_bzl/common/cc/cc_test.bzl", line 30, column 47, in _cc_test_impl
	File "/virtual_builtins_bzl/common/cc/cc_helper.bzl", line 900, column 34, in _get_expanded_env
	File "/virtual_builtins_bzl/common/cc/cc_helper.bzl", line 783, column 46, in _expand
	File "/virtual_builtins_bzl/common/cc/cc_helper.bzl", line 717, column 27, in _expand_nested_variable
Error in extend: trying to mutate a frozen list value
ERROR: /playground/BUILD.bazel:1:8: Analysis of target '//:test' failed
ERROR: Analysis of target '//:test' failed; build aborted:

@brentleyjones @oquenchil

[brentleyjones]

@iancha1992 I think this deserves going into a 6.3.1 release.

[brentleyjones]

@dflems thank you 🙏. Do we need a similar change on master? There it is also set to a default. Regardless it would be nice to have this test there as well.

[dflems]

@brentleyjones I think master differs in a way where this doesn't happen there

bazel/src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl

Lines 742 to 759 in bef4f7f

	def _expand_nested_variable(ctx, additional_vars, exp, execpath = True, targets = []):
	# If make variable is predefined path variable(like $(location ...))
	# we will expand it first.
	if exp.find(" ") != -1:
	if not execpath:
	if exp.startswith("location"):
	exp = exp.replace("location", "rootpath", 1)
	data_targets = []
	if ctx.attr.data != None:
	data_targets = ctx.attr.data
	# Make sure we do not duplicate targets.
	unified_targets_set = {}
	for data_target in data_targets:
	unified_targets_set[data_target] = True
	for target in targets:
	unified_targets_set[target] = True
	return ctx.expand_location("$({})".format(exp), targets = unified_targets_set.keys())

Alternatively, we could cherry-pick the changes from master if they apply ok

[iancha1992]

cc: @bazelbuild/triage

[keertk]

Hi @dflems could you submit a PR against master?
We can cherry-pick it into the release branch once approved and merged.

(Since 6.3.0 has been released, the release-6.3.0 branch is no longer active. We'll have to create a new one if we do a patch release.)

[dflems]

@keertk Master differs to the point where this patch cannot be applied, unfortunately

[dflems]

@brentleyjones @keertk @iancha1992

Looks like a fix for this exists on master at a63d8eb

Landed ~6mo ago but never landed on 6.x?

[brentleyjones]

Ahh. I just missed it when performing my other cherry-pick. We should take that instead.

[brentleyjones]

I can cherry-pick that against the new branch (if you haven't by the time I get to my computer). Thanks for the find!

[brentleyjones]

@iancha1992 I'm pretty sure we will need a 6.3.1 release. Would you be able to create that branch?

[buildbreaker2021]

Hey, AFAIU we will cherry-pick a63d8eb instead of this PR right?
Because I remember I solved the same issue with that commit.

[keertk]

@brentleyjones the 6.3.1 branch has been created. Could you submit a cherry-pick PR? (We'll also need the same against 6.4.0)

[brentleyjones]

• [6.3.1] Fix a bug where frozen targets list was mutated while expanding env attribute for cc_test #19052
• [6.4.0] Fix a bug where frozen targets list was mutated while expanding env attribute for cc_test #19053

[keertk]

@dflems @brentleyjones Thank you for reporting / looking into this issue.
We're looking to improve our release process so that we can catch more issues early on in the cycle (before the final release) and would love your input. What can we do to make it easier for you to test release candidates in the future?

[meteorcloudy]

@dflems Can you please confirm if this issue has been fixed in 6.3.1rc1?

[BalestraPatrick]

@meteorcloudy Sorry for the delay, but yes this issue is fixed in 6.3.1.

@keertk Yes, we're very interested in this topic. We currently have an internal fork of Bazel with about 15 patches (used to be over 20 before 6.3.0 was released) on top of it (either open PRs that haven't been merged or other changes required to get our Android builds in a good/stable place). Every time we want to bump our version of Bazel we need to rebase our fork and fix any kind of conflict arising from new upstream changes. This would be much easier if we were able to reduce the number of patches we have and have them merged upstream so that we could directly test releases produced with every release candidate.

[meteorcloudy]

This would be much easier if we were able to reduce the number of patches we have and have them merged upstream

Have you tried to upstream those patches? Any blocker from the Bazel side?

[BalestraPatrick]

@meteorcloudy Here is our list with related PR/issue tracking it:

Patch	Upstream issue / PR / commit
Add-link_library_resources-to-skip-linking-in-Androi.patch	PR
Add-output_library_linked_resources-to-remove-ap_-fi.patch	PR
Implement-non-transitive-R-class-via-android_non_tra.patch	PR
Patch-manifest-merger-25.0.0-with-queries-support.patch	Issue, PR
Add-debug_key_password-attribute-for-android_binary.patch	Issue
Expose-compatibility-arguments-on-java-toolchains.patch	Commit
Allow-passing-target-and-exec-constrains-to-java-rem.patch	PR
Add-patch-to-enable-R8-support-on-6.3.1.patch	PR
Fix-race-in-WorkerMetricsCollector.patch	Issue
Pass-host_jvmopt-to-host-options-attribute.patch	PR

On top of these that are available publicly, we have a few more that I need to check if possible to upstream.

[meteorcloudy]

@BalestraPatrick Thanks for sharing!

Those PRs are in many different statues.

For PRs that are closed due to inactivity or waiting for user response, please try to reopen the PR, rebase to HEAD and address the reviewer comments.

For PRs waiting for review, I can try to draw some attention from:

@ahumesky @ted-xie for Android rules
@hvadehra for Java rules

[fmeum]

@BalestraPatrick The two PRs for making Java runtime constraints accessible may become unnecessary with #18262 (if you use them to work around Bazel bugs rather than to encode custom requirements). I asked for clarification on the configurability PR.