Skip to content

Commit 098bacf

Browse files
fmeumfmeum
committed
Stringify labels in display form in buildifier fixes
1 parent 4a7b1d3 commit 098bacf

File tree

9 files changed

+174
-25
lines changed

9 files changed

+174
-25
lines changed

‎src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java

+32-10
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import com.google.devtools.build.lib.actions.PathMapper;
3535
import com.google.devtools.build.lib.actions.SingleStringArgFormatter;
3636
import com.google.devtools.build.lib.cmdline.Label;
37+
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
3738
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
3839
import com.google.devtools.build.lib.concurrent.BlazeInterners;
3940
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
@@ -580,9 +581,16 @@ static class PrefixArg implements ArgvFragment {
580581

581582
private static final UUID PREFIX_UUID = UUID.fromString("a95eccdf-4f54-46fc-b925-c8c7e1f50c95");
582583

583-
private static void push(List<Object> arguments, String before, Object arg) {
584+
private static void push(
585+
List<Object> arguments,
586+
String before,
587+
Object arg,
588+
@Nullable RepositoryMapping mainRepoMapping) {
584589
arguments.add(INSTANCE);
585590
arguments.add(before);
591+
if (mainRepoMapping != null) {
592+
arguments.add(mainRepoMapping);
593+
}
586594
arguments.add(arg);
587595
}
588596

@@ -594,6 +602,9 @@ public int eval(
594602
PathMapper pathMapper) {
595603
String before = (String) arguments.get(argi++);
596604
Object arg = arguments.get(argi++);
605+
if (arg instanceof RepositoryMapping mainRepoMapping) {
606+
arg = ((Label) arguments.get(argi++)).getDisplayForm(mainRepoMapping);
607+
}
597608
builder.add(before + CommandLineItem.expandToCommandLine(arg));
598609
return argi;
599610
}
@@ -606,7 +617,11 @@ public int addToFingerprint(
606617
Fingerprint fingerprint) {
607618
fingerprint.addUUID(PREFIX_UUID);
608619
fingerprint.addString((String) arguments.get(argi++));
609-
fingerprint.addString(CommandLineItem.expandToCommandLine(arguments.get(argi++)));
620+
Object arg = arguments.get(argi++);
621+
if (arg instanceof RepositoryMapping mainRepoMapping) {
622+
arg = ((Label) arguments.get(argi++)).getDisplayForm(mainRepoMapping);
623+
}
624+
fingerprint.addString(CommandLineItem.expandToCommandLine(arg));
610625
return argi;
611626
}
612627
}
@@ -888,22 +903,28 @@ public Builder addFormatted(@FormatString String formatStr, Object... args) {
888903

889904
/** Concatenates the passed prefix string and the string. */
890905
public Builder addPrefixed(@CompileTimeConstant String prefix, @Nullable String arg) {
891-
return addPrefixedInternal(prefix, arg);
906+
return addPrefixedInternal(prefix, arg, /* mainRepoMapping= */ null);
892907
}
893908

894-
/** Concatenates the passed prefix string and the label using {@link Label#getCanonicalForm}. */
895-
public Builder addPrefixedLabel(@CompileTimeConstant String prefix, @Nullable Label arg) {
896-
return addPrefixedInternal(prefix, arg);
909+
/**
910+
* Concatenates the passed prefix string and the label using {@link Label#getDisplayForm}, which
911+
* is identical to {@link Label#getCanonicalForm()} for main repo labels.
912+
*/
913+
public Builder addPrefixedLabel(
914+
@CompileTimeConstant String prefix,
915+
@Nullable Label arg,
916+
RepositoryMapping mainRepoMapping) {
917+
return addPrefixedInternal(prefix, arg, mainRepoMapping);
897918
}
898919

899920
/** Concatenates the passed prefix string and the path. */
900921
public Builder addPrefixedPath(@CompileTimeConstant String prefix, @Nullable PathFragment arg) {
901-
return addPrefixedInternal(prefix, arg);
922+
return addPrefixedInternal(prefix, arg, /* mainRepoMapping= */ null);
902923
}
903924

904925
/** Concatenates the passed prefix string and the artifact's exec path. */
905926
public Builder addPrefixedExecPath(@CompileTimeConstant String prefix, @Nullable Artifact arg) {
906-
return addPrefixedInternal(prefix, arg);
927+
return addPrefixedInternal(prefix, arg, /* mainRepoMapping= */ null);
907928
}
908929

909930
/**
@@ -1129,10 +1150,11 @@ private Builder addObjectInternal(@CompileTimeConstant String arg, @Nullable Obj
11291150
}
11301151

11311152
@CanIgnoreReturnValue
1132-
private Builder addPrefixedInternal(String prefix, @Nullable Object arg) {
1153+
private Builder addPrefixedInternal(
1154+
String prefix, @Nullable Object arg, @Nullable RepositoryMapping mainRepoMapping) {
11331155
Preconditions.checkNotNull(prefix);
11341156
if (arg != null) {
1135-
PrefixArg.push(arguments, prefix, arg);
1157+
PrefixArg.push(arguments, prefix, arg, mainRepoMapping);
11361158
}
11371159
return this;
11381160
}

‎src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java

+5-3
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,8 @@ public JavaCompileOutputs<Artifact> createOutputs(JavaOutput output) {
208208
return builder.build();
209209
}
210210

211-
public void createCompileAction(JavaCompileOutputs<Artifact> outputs) throws RuleErrorException {
211+
public void createCompileAction(JavaCompileOutputs<Artifact> outputs)
212+
throws RuleErrorException, InterruptedException {
212213
if (outputs.genClass() != null) {
213214
createGenJarAction(
214215
outputs.output(),
@@ -542,7 +543,7 @@ private Artifact turbineOutput(Artifact classJar, String newExtension) {
542543
* @param headerDeps the .jdeps output of this java compilation
543544
*/
544545
public void createHeaderCompilationAction(Artifact headerJar, Artifact headerDeps)
545-
throws RuleErrorException {
546+
throws RuleErrorException, InterruptedException {
546547

547548
JavaTargetAttributes attributes = getAttributes();
548549

@@ -687,7 +688,8 @@ public void createSourceJarAction(Artifact outputJar, @Nullable Artifact gensrcJ
687688
* @return the header jar (if requested), or ijar (if requested), or else the class jar
688689
*/
689690
public Artifact createCompileTimeJarAction(
690-
Artifact runtimeJar, JavaCompilationArtifacts.Builder builder) throws RuleErrorException {
691+
Artifact runtimeJar, JavaCompilationArtifacts.Builder builder)
692+
throws RuleErrorException, InterruptedException {
691693
Artifact jar;
692694
boolean isFullJar = false;
693695
if (shouldUseHeaderCompilation()) {

‎src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java

+4-3
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ public JavaCompileActionBuilder(
171171
this.execGroup = execGroup;
172172
}
173173

174-
public JavaCompileAction build() throws RuleErrorException {
174+
public JavaCompileAction build() throws RuleErrorException, InterruptedException {
175175
// TODO(bazel-team): all the params should be calculated before getting here, and the various
176176
// aggregation code below should go away.
177177

@@ -271,7 +271,7 @@ private ImmutableSet<Artifact> allOutputs() {
271271
}
272272

273273
private CustomCommandLine buildParamFileContents(ImmutableList<String> javacOpts)
274-
throws RuleErrorException {
274+
throws RuleErrorException, InterruptedException {
275275

276276
CustomCommandLine.Builder result = CustomCommandLine.builder();
277277

@@ -304,7 +304,8 @@ private CustomCommandLine buildParamFileContents(ImmutableList<String> javacOpts
304304
} else {
305305
// @-prefixed strings will be assumed to be filenames and expanded by
306306
// {@link JavaLibraryBuildRequest}, so add an extra &at; to escape it.
307-
result.addPrefixedLabel("@", targetLabel);
307+
result.addPrefixedLabel(
308+
"@", targetLabel, ruleContext.getAnalysisEnvironment().getMainRepoMapping());
308309
}
309310
}
310311
result.add("--injecting_rule_kind", injectingRuleKind);

‎src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java

+4-2
Original file line numberDiff line numberDiff line change
@@ -368,7 +368,8 @@ public Builder enableDirectClasspath(boolean enableDirectClasspath) {
368368
}
369369

370370
/** Builds and registers the action for a header compilation. */
371-
public void build(JavaToolchainProvider javaToolchain) throws RuleErrorException {
371+
public void build(JavaToolchainProvider javaToolchain)
372+
throws RuleErrorException, InterruptedException {
372373
checkNotNull(outputDepsProto, "outputDepsProto must not be null");
373374
checkNotNull(sourceFiles, "sourceFiles must not be null");
374375
checkNotNull(sourceJars, "sourceJars must not be null");
@@ -478,7 +479,8 @@ public void build(JavaToolchainProvider javaToolchain) throws RuleErrorException
478479
} else {
479480
// @-prefixed strings will be assumed to be params filenames and expanded,
480481
// so add an extra @ to escape it.
481-
commandLine.addPrefixedLabel("@", targetLabel);
482+
commandLine.addPrefixedLabel(
483+
"@", targetLabel, ruleContext.getAnalysisEnvironment().getMainRepoMapping());
482484
}
483485
}
484486

‎src/main/java/com/google/devtools/build/lib/rules/java/JavaStarlarkCommon.java

+10-2
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,11 @@ public void createHeaderCompilationAction(
134134
Object injectingRuleKind,
135135
boolean enableDirectClasspath,
136136
Sequence<?> additionalInputs)
137-
throws EvalException, TypeException, RuleErrorException, LabelSyntaxException {
137+
throws EvalException,
138+
TypeException,
139+
RuleErrorException,
140+
LabelSyntaxException,
141+
InterruptedException {
138142
checkJavaToolchainIsDeclaredOnRule(ctx.getRuleContext());
139143
JavaTargetAttributes.Builder attributesBuilder =
140144
new JavaTargetAttributes.Builder(javaSemantics)
@@ -199,7 +203,11 @@ public void createCompilationAction(
199203
boolean enableDirectClasspath,
200204
Sequence<?> additionalInputs,
201205
Sequence<?> additionalOutputs)
202-
throws EvalException, TypeException, RuleErrorException, LabelSyntaxException {
206+
throws EvalException,
207+
TypeException,
208+
RuleErrorException,
209+
LabelSyntaxException,
210+
InterruptedException {
203211
checkJavaToolchainIsDeclaredOnRule(ctx.getRuleContext());
204212
JavaCompileOutputs<Artifact> outputs =
205213
JavaCompileOutputs.builder()

‎src/main/java/com/google/devtools/build/lib/starlarkbuildapi/java/JavaCommonApi.java

+10-2
Original file line numberDiff line numberDiff line change
@@ -523,7 +523,11 @@ void createHeaderCompilationAction(
523523
Object injectingRuleKind,
524524
boolean enableDirectClasspath,
525525
Sequence<?> additionalInputs)
526-
throws EvalException, TypeException, RuleErrorException, LabelSyntaxException;
526+
throws EvalException,
527+
TypeException,
528+
RuleErrorException,
529+
LabelSyntaxException,
530+
InterruptedException;
527531

528532
@StarlarkMethod(
529533
name = "create_compilation_action",
@@ -587,7 +591,11 @@ void createCompilationAction(
587591
boolean enableDirectClasspath,
588592
Sequence<?> additionalInputs,
589593
Sequence<?> additionalOutputs)
590-
throws EvalException, TypeException, RuleErrorException, LabelSyntaxException;
594+
throws EvalException,
595+
TypeException,
596+
RuleErrorException,
597+
LabelSyntaxException,
598+
InterruptedException;
591599

592600
@StarlarkMethod(
593601
name = "default_javac_opts",

‎src/test/java/com/google/devtools/build/lib/actions/CustomCommandLineTest.java

+22-2
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import com.google.common.base.Joiner;
2222
import com.google.common.collect.ImmutableList;
23+
import com.google.common.collect.ImmutableMap;
2324
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
2425
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
2526
import com.google.devtools.build.lib.actions.ArtifactRoot.RootType;
@@ -28,6 +29,8 @@
2829
import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.VectorArg;
2930
import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.VectorArg.SimpleVectorArg;
3031
import com.google.devtools.build.lib.cmdline.Label;
32+
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
33+
import com.google.devtools.build.lib.cmdline.RepositoryName;
3134
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
3235
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
3336
import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -126,7 +129,8 @@ public void addPrefixed_addsPrefixForArguments() throws Exception {
126129
.containsExactly("prefix-foo");
127130
assertThat(
128131
builder()
129-
.addPrefixedLabel("prefix-", Label.parseCanonical("//a:b"))
132+
.addPrefixedLabel(
133+
"prefix-", Label.parseCanonical("//a:b"), /* mainRepoMapping= */ null)
130134
.build()
131135
.arguments())
132136
.containsExactly("prefix-//a:b");
@@ -137,6 +141,22 @@ public void addPrefixed_addsPrefixForArguments() throws Exception {
137141
.containsExactly("prefix-dir/file1.txt");
138142
}
139143

144+
@Test
145+
public void addPrefixedLabel_emitsExternalLabelInDisplayForm() throws Exception {
146+
assertThat(
147+
builder()
148+
.addPrefixedLabel(
149+
"prefix-",
150+
Label.parseCanonical("@@canonical_name//a:b"),
151+
RepositoryMapping.create(
152+
ImmutableMap.of(
153+
"apparent_name", RepositoryName.createUnvalidated("canonical_name")),
154+
RepositoryName.MAIN))
155+
.build()
156+
.arguments())
157+
.containsExactly("prefix-@apparent_name//a:b");
158+
}
159+
140160
@Test
141161
public void addAll_addsAllArguments() throws Exception {
142162
assertThat(builder().addAll(list("val1", "val2")).build().arguments())
@@ -527,7 +547,7 @@ public void addNulls_addsNothing() throws Exception {
527547
.addExecPath("foo", null)
528548
.addLazyString("foo", null)
529549
.addPrefixed("prefix", null)
530-
.addPrefixedLabel("prefix", null)
550+
.addPrefixedLabel("prefix", null, /* mainRepoMapping= */ null)
531551
.addPrefixedPath("prefix", null)
532552
.addPrefixedExecPath("prefix", null)
533553
.addAll((ImmutableList<String>) null)

‎src/test/shell/bazel/bazel_java_test.sh

+86
Original file line numberDiff line numberDiff line change
@@ -1998,4 +1998,90 @@ EOF
19981998
expect_log "buildozer 'add deps @c//:c' //pkg:a"
19991999
}
20002000

2001+
function test_strict_deps_error_external_repo_header_compile_action() {
2002+
cat << 'EOF' > MODULE.bazel
2003+
bazel_dep(
2004+
name = "lib_c",
2005+
repo_name = "c",
2006+
)
2007+
local_path_override(
2008+
module_name = "lib_c",
2009+
path = "lib_c",
2010+
)
2011+
EOF
2012+
2013+
mkdir -p pkg
2014+
cat << 'EOF' > pkg/BUILD
2015+
java_binary(name = "Main", srcs = ["Main.java"], deps = [":a"])
2016+
java_library(name = "a", srcs = ["A.java"], deps = [":b"])
2017+
java_library(name = "b", srcs = ["B.java"], deps = ["@c"])
2018+
EOF
2019+
cat << 'EOF' > pkg/Main.java
2020+
public class Main extends A {}
2021+
EOF
2022+
cat << 'EOF' > pkg/A.java
2023+
public class A extends B implements C {}
2024+
EOF
2025+
cat << 'EOF' > pkg/B.java
2026+
public class B implements C {}
2027+
EOF
2028+
2029+
mkdir -p lib_c
2030+
cat << 'EOF' > lib_c/MODULE.bazel
2031+
module(name = "lib_c")
2032+
EOF
2033+
cat << 'EOF' > lib_c/BUILD
2034+
java_library(name = "c", srcs = ["C.java"], visibility = ["//visibility:public"])
2035+
EOF
2036+
cat << 'EOF' > lib_c/C.java
2037+
public interface C {}
2038+
EOF
2039+
2040+
bazel build //pkg:a >& $TEST_log && fail "build should fail"
2041+
expect_log "buildozer 'add deps @c//:c' //pkg:a"
2042+
}
2043+
2044+
function test_strict_deps_error_external_repo_compile_action() {
2045+
cat << 'EOF' > MODULE.bazel
2046+
bazel_dep(
2047+
name = "lib_c",
2048+
repo_name = "c",
2049+
)
2050+
local_path_override(
2051+
module_name = "lib_c",
2052+
path = "lib_c",
2053+
)
2054+
EOF
2055+
2056+
mkdir -p pkg
2057+
cat << 'EOF' > pkg/BUILD
2058+
java_library(name = "a", srcs = ["A.java"], deps = [":b"])
2059+
java_library(name = "b", srcs = ["B.java"], deps = ["@c"])
2060+
EOF
2061+
cat << 'EOF' > pkg/A.java
2062+
public class A extends B {
2063+
boolean foo() {
2064+
return this instanceof C;
2065+
}
2066+
}
2067+
EOF
2068+
cat << 'EOF' > pkg/B.java
2069+
public class B implements C {}
2070+
EOF
2071+
2072+
mkdir -p lib_c
2073+
cat << 'EOF' > lib_c/MODULE.bazel
2074+
module(name = "lib_c")
2075+
EOF
2076+
cat << 'EOF' > lib_c/BUILD
2077+
java_library(name = "c", srcs = ["C.java"], visibility = ["//visibility:public"])
2078+
EOF
2079+
cat << 'EOF' > lib_c/C.java
2080+
public interface C {}
2081+
EOF
2082+
2083+
bazel build //pkg:a >& $TEST_log && fail "build should fail"
2084+
expect_log "buildozer 'add deps @c//:c' //pkg:a"
2085+
}
2086+
20012087
run_suite "Java integration tests"

‎src/test/shell/bazel/local_repository_test.sh

+1-1
Original file line numberDiff line numberDiff line change
@@ -1411,7 +1411,7 @@ EOF
14111411

14121412
bazel build @x_repo//a >& $TEST_log && fail "Building @x_repo//a should error out"
14131413
expect_log "** Please add the following dependencies:"
1414-
expect_log "@@x_repo//x to @@x_repo//a"
1414+
expect_log " @x_repo//x to @x_repo//a"
14151415
}
14161416

14171417
# This test verifies that the `public` pattern includes external dependencies.

0 commit comments

Comments
 (0)