Skip to content

Commit 2fe0ebc

Browse files
authored
Fix make variable expansion (bazelbuild#7399)
We previously had a custom implementation form make variable expansion. However, this implementation was different from the one used by bazel and can lead to issues in some edge cases.
1 parent 840b622 commit 2fe0ebc

File tree

13 files changed

+276
-145
lines changed

13 files changed

+276
-145
lines changed

.bazelrc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@ build --tool_java_language_version=21 --tool_java_runtime_version=21
33

44
# Delete test data packages, needed for bazel integration tests. Update by running the following command:
55
# bazel run @rules_bazel_integration_test//tools:update_deleted_packages
6-
build --deleted_packages=aspect/testing/tests/src/com/google/idea/blaze/aspect/integration/testdata,clwb/tests/projects/external_includes/main,clwb/tests/projects/llvm_toolchain/.clwb/aspects,clwb/tests/projects/llvm_toolchain/main,clwb/tests/projects/llvm_toolchain/wasm,clwb/tests/projects/simple/main,clwb/tests/projects/virtual_includes/lib/impl_deps,clwb/tests/projects/virtual_includes/lib/strip_absolut,clwb/tests/projects/virtual_includes/lib/strip_relative,clwb/tests/projects/virtual_includes/main,ijwb/tests/projects/simple,testing/test_deps/projects,testing/test_deps/projects/simple_java,testing/test_deps/projects/simple_java/java/com/example/sample/nested
7-
query --deleted_packages=aspect/testing/tests/src/com/google/idea/blaze/aspect/integration/testdata,clwb/tests/projects/external_includes/main,clwb/tests/projects/llvm_toolchain/.clwb/aspects,clwb/tests/projects/llvm_toolchain/main,clwb/tests/projects/llvm_toolchain/wasm,clwb/tests/projects/simple/main,clwb/tests/projects/virtual_includes/lib/impl_deps,clwb/tests/projects/virtual_includes/lib/strip_absolut,clwb/tests/projects/virtual_includes/lib/strip_relative,clwb/tests/projects/virtual_includes/main,ijwb/tests/projects/simple,testing/test_deps/projects,testing/test_deps/projects/simple_java,testing/test_deps/projects/simple_java/java/com/example/sample/nested
6+
build --deleted_packages=aspect/testing/tests/src/com/google/idea/blaze/aspect/integration/testdata,clwb/tests/projects/external_includes/main,clwb/tests/projects/llvm_toolchain/.clwb/aspects,clwb/tests/projects/llvm_toolchain/main,clwb/tests/projects/llvm_toolchain/wasm,clwb/tests/projects/query_sync/main,clwb/tests/projects/simple/main,clwb/tests/projects/virtual_includes/lib/impl_deps,clwb/tests/projects/virtual_includes/lib/strip_absolut,clwb/tests/projects/virtual_includes/lib/strip_relative,clwb/tests/projects/virtual_includes/main,ijwb/tests/projects/simple,testing/test_deps/projects,testing/test_deps/projects/java_and_deps,testing/test_deps/projects/java_and_deps/deps/top_level_lib_1,testing/test_deps/projects/java_and_deps/deps/top_level_lib_2,testing/test_deps/projects/java_and_deps/deps/transitive_dep_lib,testing/test_deps/projects/java_and_deps/project,testing/test_deps/projects/java_and_deps/project/java/com/example/sample/nested,testing/test_deps/projects/simple_java,testing/test_deps/projects/simple_java/java/com/example/sample/nested
7+
query --deleted_packages=aspect/testing/tests/src/com/google/idea/blaze/aspect/integration/testdata,clwb/tests/projects/external_includes/main,clwb/tests/projects/llvm_toolchain/.clwb/aspects,clwb/tests/projects/llvm_toolchain/main,clwb/tests/projects/llvm_toolchain/wasm,clwb/tests/projects/query_sync/main,clwb/tests/projects/simple/main,clwb/tests/projects/virtual_includes/lib/impl_deps,clwb/tests/projects/virtual_includes/lib/strip_absolut,clwb/tests/projects/virtual_includes/lib/strip_relative,clwb/tests/projects/virtual_includes/main,ijwb/tests/projects/simple,testing/test_deps/projects,testing/test_deps/projects/java_and_deps,testing/test_deps/projects/java_and_deps/deps/top_level_lib_1,testing/test_deps/projects/java_and_deps/deps/top_level_lib_2,testing/test_deps/projects/java_and_deps/deps/transitive_dep_lib,testing/test_deps/projects/java_and_deps/project,testing/test_deps/projects/java_and_deps/project/java/com/example/sample/nested,testing/test_deps/projects/simple_java,testing/test_deps/projects/simple_java/java/com/example/sample/nested
88

99
common --enable_bzlmod
1010
common --noincompatible_disallow_empty_glob

aspect/intellij_info_impl.bzl

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -349,14 +349,13 @@ def _get_python_srcs_version(ctx):
349349
srcs_version = getattr(ctx.rule.attr, "srcs_version", "PY2AND3")
350350
return _SRCS_VERSION_MAPPING.get(srcs_version, default = SRC_PY2AND3)
351351

352-
def _do_starlark_string_expansion(ctx, name, strings, extra_targets = []):
352+
def _do_starlark_string_expansion(ctx, name, strings, extra_targets = [], tokenization = True):
353353
# first, expand all starlark predefined paths:
354354
# location, locations, rootpath, rootpaths, execpath, execpaths
355355
strings = [ctx.expand_location(value, targets = extra_targets) for value in strings]
356356

357357
# then expand any regular GNU make style variables
358-
strings = [expand_make_variables(name, value, ctx) for value in strings]
359-
return strings
358+
return expand_make_variables(ctx, tokenization, strings)
360359

361360
##### Builders for individual parts of the aspect output
362361

@@ -375,7 +374,7 @@ def collect_py_info(target, ctx, semantics, ide_info, ide_info_file, output_grou
375374
to_build = get_py_info(target).transitive_sources
376375
args = getattr(ctx.rule.attr, "args", [])
377376
data_deps = getattr(ctx.rule.attr, "data", [])
378-
args = _do_starlark_string_expansion(ctx, "args", args, data_deps)
377+
args = _do_starlark_string_expansion(ctx, "args", args, data_deps, tokenization = False)
379378
imports = getattr(ctx.rule.attr, "imports", [])
380379
is_code_generator = False
381380

aspect/make_variables.bzl

Lines changed: 205 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -1,98 +1,209 @@
1-
"""Utility functions to expand make variables."""
2-
3-
def _is_valid_make_var(varname):
4-
"""Check if the make variable name seems valid."""
5-
if len(varname) == 0:
6-
return False
7-
8-
# According to gnu make, any chars not whitespace, ':', '#', '=' are valid.
9-
invalid_chars = ":#= \t\n\r"
10-
for n in range(0, len(invalid_chars)):
11-
if invalid_chars[n] in varname:
12-
return False
13-
return True
14-
15-
def expand_make_variables(attr_name, expression, ctx, additional_subs = {}):
16-
"""Substitutes make variables defined in $() syntax.
17-
18-
Because ctx.expand_make_variables is deprecated, we need to be able to do the
19-
substitution without relying on it.
20-
Before the aspect is processed, the build system already detects most/all of
21-
the failure modes and the aspect does not get processed, but including them
22-
here helps with following the logic.
23-
24-
Args:
25-
attr_name: The attribute name. Used for error reporting.
26-
expression: The expression to expand. It can contain references to "Make
27-
variables".
28-
ctx: The context containing default make variables to subtitute.
29-
additional_subs: Additional substitutions to make beyond the default make
30-
variables.
31-
32-
Returns:
33-
Returns a string after expanding all references to "Make variables". The
34-
variables must have the following format: $(VAR_NAME). Also, $$VAR_NAME
35-
expands to $VAR_NAME.
36-
37-
38-
"""
39-
if "$" not in expression:
40-
return expression
41-
42-
current_offset = 0
43-
rv = ""
44-
substitutions = {}
45-
substitutions.update(ctx.var)
46-
47-
# make variables from ctx.var can be overridden
48-
substitutions.update(additional_subs)
49-
50-
# skylark does not support while. This is the maximum iteration count this
51-
# loop will need, but it will exit early if possible.
52-
for _n in range(0, len(expression)):
53-
if current_offset >= len(expression):
1+
# Copyright 2020 The Bazel Authors. All rights reserved.
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License");
4+
# you may not use this file except in compliance with the License.
5+
# You may obtain a copy of the License at
6+
#
7+
# http://www.apache.org/licenses/LICENSE-2.0
8+
#
9+
# Unless required by applicable law or agreed to in writing, software
10+
# distributed under the License is distributed on an "AS IS" BASIS,
11+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
# See the License for the specific language governing permissions and
13+
# limitations under the License.
14+
15+
# Copied from: https://github.com/bazelbuild/bazel/blob/6f7faa659e5eb3e56c8a6274ebcb86884703d603/src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl
16+
17+
"""Utility functions to expand make variables. Implementation taken from cc_helper. """
18+
19+
def expand_make_variables(ctx, tokenization, unexpanded_tokens, additional_make_variable_substitutions = {}):
20+
tokens = []
21+
targets = []
22+
for additional_compiler_input in getattr(ctx.attr, "additional_compiler_inputs", []):
23+
targets.append(additional_compiler_input)
24+
for token in unexpanded_tokens:
25+
if tokenization:
26+
expanded_token = _expand(ctx, token, additional_make_variable_substitutions, targets = targets)
27+
_tokenize(tokens, expanded_token)
28+
else:
29+
exp = _expand_single_make_variable(ctx, token, additional_make_variable_substitutions)
30+
if exp != None:
31+
_tokenize(tokens, exp)
32+
else:
33+
tokens.append(_expand(ctx, token, additional_make_variable_substitutions, targets = targets))
34+
return tokens
35+
36+
# Tries to expand a single make variable from token.
37+
# If token has additional characters other than ones
38+
# corresponding to make variable returns None.
39+
def _expand_single_make_variable(ctx, token, additional_make_variable_substitutions = {}):
40+
if len(token) < 3:
41+
return None
42+
if token[0] != "$" or token[1] != "(" or token[len(token) - 1] != ")":
43+
return None
44+
unexpanded_var = token[2:len(token) - 1]
45+
expanded_var = _expand_nested_variable(ctx, additional_make_variable_substitutions, unexpanded_var)
46+
return expanded_var
47+
48+
49+
def _expand_nested_variable(ctx, additional_vars, exp, execpath = True, targets = []):
50+
# If make variable is predefined path variable(like $(location ...))
51+
# we will expand it first.
52+
if exp.find(" ") != -1:
53+
if not execpath:
54+
if exp.startswith("location"):
55+
exp = exp.replace("location", "rootpath", 1)
56+
data_targets = []
57+
if ctx.attr.data != None:
58+
data_targets = ctx.attr.data
59+
60+
# Make sure we do not duplicate targets.
61+
unified_targets_set = {}
62+
for data_target in data_targets:
63+
unified_targets_set[data_target] = True
64+
for target in targets:
65+
unified_targets_set[target] = True
66+
return ctx.expand_location("$({})".format(exp), targets = unified_targets_set.keys())
67+
68+
# Recursively expand nested make variables, but since there is no recursion
69+
# in Starlark we will do it via for loop.
70+
unbounded_recursion = True
71+
72+
# The only way to check if the unbounded recursion is happening or not
73+
# is to have a look at the depth of the recursion.
74+
# 10 seems to be a reasonable number, since it is highly unexpected
75+
# to have nested make variables which are expanding more than 10 times.
76+
for _ in range(10):
77+
exp = _lookup_var(ctx, additional_vars, exp)
78+
if len(exp) >= 3 and exp[0] == "$" and exp[1] == "(" and exp[len(exp) - 1] == ")":
79+
# Try to expand once more.
80+
exp = exp[2:len(exp) - 1]
81+
continue
82+
unbounded_recursion = False
83+
break
84+
85+
if unbounded_recursion:
86+
fail("potentially unbounded recursion during expansion of {}".format(exp))
87+
return exp
88+
89+
def _lookup_var(ctx, additional_vars, var):
90+
expanded_make_var_ctx = ctx.var.get(var)
91+
expanded_make_var_additional = additional_vars.get(var)
92+
if expanded_make_var_additional != None:
93+
return expanded_make_var_additional
94+
if expanded_make_var_ctx != None:
95+
return expanded_make_var_ctx
96+
fail("{}: {} not defined".format(ctx.label, "$(" + var + ")"))
97+
98+
def _expand(ctx, expression, additional_make_variable_substitutions, execpath = True, targets = []):
99+
idx = 0
100+
last_make_var_end = 0
101+
result = []
102+
n = len(expression)
103+
for _ in range(n):
104+
if idx >= n:
54105
break
55-
begin_dollars = expression.find("$", current_offset)
56-
if begin_dollars == -1:
57-
# append whatever is left in expression
58-
rv = rv + expression[current_offset:]
59-
current_offset = len(expression)
106+
if expression[idx] != "$":
107+
idx += 1
60108
continue
61-
if begin_dollars != current_offset:
62-
rv = rv + expression[current_offset:begin_dollars]
63-
64-
# consume the entire run of $$$...
65-
end_dollars = begin_dollars + 1
66-
for _m in range(end_dollars, len(expression)):
67-
if expression[end_dollars] == "$":
68-
end_dollars = end_dollars + 1
109+
110+
idx += 1
111+
112+
# We've met $$ pattern, so $ is escaped.
113+
if idx < n and expression[idx] == "$":
114+
idx += 1
115+
result.append(expression[last_make_var_end:idx - 1])
116+
last_make_var_end = idx
117+
# We might have found a potential start for Make Variable.
118+
119+
elif idx < n and expression[idx] == "(":
120+
# Try to find the closing parentheses.
121+
make_var_start = idx
122+
make_var_end = make_var_start
123+
for j in range(idx + 1, n):
124+
if expression[j] == ")":
125+
make_var_end = j
126+
break
127+
128+
# Note we cannot go out of string's bounds here,
129+
# because of this check.
130+
# If start of the variable is different from the end,
131+
# we found a make variable.
132+
if make_var_start != make_var_end:
133+
# Some clarifications:
134+
# *****$(MAKE_VAR_1)*******$(MAKE_VAR_2)*****
135+
# ^ ^ ^
136+
# | | |
137+
# last_make_var_end make_var_start make_var_end
138+
result.append(expression[last_make_var_end:make_var_start - 1])
139+
make_var = expression[make_var_start + 1:make_var_end]
140+
exp = _expand_nested_variable(ctx, additional_make_variable_substitutions, make_var, execpath, targets)
141+
result.append(exp)
142+
143+
# Update indexes.
144+
idx = make_var_end + 1
145+
last_make_var_end = idx
146+
147+
# Add the last substring which would be skipped by for loop.
148+
if last_make_var_end < n:
149+
result.append(expression[last_make_var_end:n])
150+
151+
return "".join(result)
152+
153+
def _tokenize(options, options_string):
154+
token = []
155+
force_token = False
156+
quotation = "\0"
157+
length = len(options_string)
158+
159+
# Since it is impossible to modify loop variable inside loop
160+
# in Starlark, and also there is no while loop, I have to
161+
# use this ugly hack.
162+
i = -1
163+
for _ in range(length):
164+
i += 1
165+
if i >= length:
166+
break
167+
c = options_string[i]
168+
if quotation != "\0":
169+
# In quotation.
170+
if c == quotation:
171+
# End quotation.
172+
quotation = "\0"
173+
elif c == "\\" and quotation == "\"":
174+
i += 1
175+
if i == length:
176+
fail("backslash at the end of the string: {}".format(options_string))
177+
c = options_string[i]
178+
if c != "\\" and c != "\"":
179+
token.append("\\")
180+
token.append(c)
69181
else:
70-
break
71-
if (end_dollars - begin_dollars) % 2 == 0:
72-
# even number of '$'
73-
rv = rv + "$" * ((end_dollars - begin_dollars) // 2)
74-
current_offset = end_dollars
75-
continue
182+
# Regular char, in quotation.
183+
token.append(c)
184+
else:
185+
# Not in quotation.
186+
if c == "'" or c == "\"":
187+
# Begin single double quotation.
188+
quotation = c
189+
force_token = True
190+
elif c == " " or c == "\t":
191+
# Space not quoted.
192+
if force_token or len(token) > 0:
193+
options.append("".join(token))
194+
token = []
195+
force_token = False
196+
elif c == "\\":
197+
# Backslash not quoted.
198+
i += 1
199+
if i == length:
200+
fail("backslash at the end of the string: {}".format(options_string))
201+
token.append(options_string[i])
202+
else:
203+
# Regular char, not quoted.
204+
token.append(c)
205+
if quotation != "\0":
206+
fail("unterminated quotation at the end of the string: {}".format(options_string))
76207

77-
# odd number of '$'
78-
if end_dollars == len(expression) or expression[end_dollars] != "(":
79-
# odd number of '$' at the end of the string is invalid
80-
# odd number of '$' followed by non-( is invalid
81-
fail("expand_make_variables: unterminated $", attr_name)
82-
end_parens = expression.find(")", end_dollars)
83-
if end_parens == -1:
84-
# no end parens is invalid
85-
fail("expand_make_variables: unterminated variable reference", attr_name)
86-
87-
# odd number of '$', but integer division will provide correct count
88-
rv = rv + "$" * ((end_dollars - begin_dollars) // 2)
89-
varname = expression[end_dollars + 1:end_parens]
90-
if not _is_valid_make_var(varname):
91-
# invalid make variable name
92-
fail("expand_make_variables: $(%s) invalid name" % varname, attr_name)
93-
if not varname in substitutions:
94-
# undefined make variable
95-
fail("expand_make_variables: $(%s) not defined" % varname, attr_name)
96-
rv = rv + substitutions[varname]
97-
current_offset = end_parens + 1
98-
return rv
208+
if force_token or len(token) > 0:
209+
options.append("".join(token))

clwb/BUILD

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ clwb_headless_test(
183183
clwb_headless_test(
184184
name = "query_sync_headless_test",
185185
srcs = ["tests/headlesstests/com/google/idea/blaze/clwb/QuerySyncTest.java"],
186-
project = "simple",
186+
project = "query_sync",
187187
)
188188

189189
test_suite(

clwb/tests/headlesstests/com/google/idea/blaze/clwb/QuerySyncTest.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package com.google.idea.blaze.clwb;
22

33
import static com.google.common.truth.Truth.assertThat;
4+
import static com.google.idea.blaze.clwb.base.Assertions.assertContainsCompilerFlag;
45
import static com.google.idea.blaze.clwb.base.Assertions.assertContainsHeader;
56

67
import com.google.idea.blaze.base.bazel.BazelVersion;
@@ -62,6 +63,7 @@ private void checkCompiler() {
6263
// }
6364

6465
assertContainsHeader("iostream", compilerSettings);
66+
assertContainsCompilerFlag("-Wall", compilerSettings);
6567
}
6668

6769
private void checkTest() throws ExecutionException {

clwb/tests/headlesstests/com/google/idea/blaze/clwb/SimpleTest.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package com.google.idea.blaze.clwb;
22

33
import static com.google.common.truth.Truth.assertThat;
4+
import static com.google.idea.blaze.clwb.base.Assertions.assertContainsCompilerFlag;
45
import static com.google.idea.blaze.clwb.base.Assertions.assertContainsHeader;
56
import static com.google.idea.blaze.clwb.base.Assertions.assertContainsPattern;
67

@@ -43,6 +44,7 @@ private void checkCompiler() {
4344
}
4445

4546
assertContainsHeader("iostream", compilerSettings);
47+
assertContainsCompilerFlag("-Wall", compilerSettings);
4648
}
4749

4850
private void checkTest() {

0 commit comments

Comments
 (0)