Skip to content

Commit

Permalink
Show display form labels in java_* buildozer fixups
Browse files Browse the repository at this point in the history
The change in bazelbuild#21702 only fixed the buildozer fixup messages for `java_*` actions entirely defined in Starlark. This commit makes the analogous change to the remaining native actions.

Related to bazelbuild#20486 and bazelbuild#21702

Closes bazelbuild#21827.

PiperOrigin-RevId: 623253302
Change-Id: Iadc2b0d8ce64f359b921ad7a7c489111a3a29997
  • Loading branch information
fmeum committed Apr 10, 2024
1 parent 58a2922 commit 035ee82
Show file tree
Hide file tree
Showing 10 changed files with 208 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.google.devtools.build.lib.actions.PathMapper;
import com.google.devtools.build.lib.actions.SingleStringArgFormatter;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.concurrent.BlazeInterners;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
Expand Down Expand Up @@ -580,9 +581,16 @@ static class PrefixArg implements ArgvFragment {

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

private static void push(List<Object> arguments, String before, Object arg) {
private static void push(
List<Object> arguments,
String before,
Object arg,
@Nullable RepositoryMapping mainRepoMapping) {
arguments.add(INSTANCE);
arguments.add(before);
if (mainRepoMapping != null) {
arguments.add(mainRepoMapping);
}
arguments.add(arg);
}

Expand All @@ -594,6 +602,9 @@ public int eval(
PathMapper pathMapper) {
String before = (String) arguments.get(argi++);
Object arg = arguments.get(argi++);
if (arg instanceof RepositoryMapping mainRepoMapping) {
arg = ((Label) arguments.get(argi++)).getDisplayForm(mainRepoMapping);
}
builder.add(before + CommandLineItem.expandToCommandLine(arg));
return argi;
}
Expand All @@ -606,7 +617,11 @@ public int addToFingerprint(
Fingerprint fingerprint) {
fingerprint.addUUID(PREFIX_UUID);
fingerprint.addString((String) arguments.get(argi++));
fingerprint.addString(CommandLineItem.expandToCommandLine(arguments.get(argi++)));
Object arg = arguments.get(argi++);
if (arg instanceof RepositoryMapping mainRepoMapping) {
arg = ((Label) arguments.get(argi++)).getDisplayForm(mainRepoMapping);
}
fingerprint.addString(CommandLineItem.expandToCommandLine(arg));
return argi;
}
}
Expand Down Expand Up @@ -752,6 +767,7 @@ public static final class Builder {
*
* <p>Prefer this over its dynamic cousin, as using static strings saves memory.
*/
@CanIgnoreReturnValue
public Builder add(@CompileTimeConstant String value) {
return addObjectInternal(value);
}
Expand All @@ -761,6 +777,7 @@ public Builder add(@CompileTimeConstant String value) {
*
* <p>If the value is null, neither the arg nor the value is added.
*/
@CanIgnoreReturnValue
public Builder add(@CompileTimeConstant String arg, @Nullable String value) {
return addObjectInternal(arg, value);
}
Expand Down Expand Up @@ -797,6 +814,7 @@ public Builder addObject(@Nullable Object value) {
* <p>There are many other ways you can try to avoid calling this. In general, try to use
* constants or objects that are already on the heap elsewhere.
*/
@CanIgnoreReturnValue
public Builder addDynamicString(@Nullable String value) {
return addObjectInternal(value);
}
Expand All @@ -807,6 +825,7 @@ public Builder addDynamicString(@Nullable String value) {
* <p>Prefer this over manually calling {@link Label#getCanonicalForm}, as it avoids a copy of
* the label value.
*/
@CanIgnoreReturnValue
public Builder addLabel(@Nullable Label value) {
return addObjectInternal(value);
}
Expand All @@ -819,6 +838,7 @@ public Builder addLabel(@Nullable Label value) {
*
* <p>If the value is null, neither the arg nor the value is added.
*/
@CanIgnoreReturnValue
public Builder addLabel(@CompileTimeConstant String arg, @Nullable Label value) {
return addObjectInternal(arg, value);
}
Expand All @@ -829,6 +849,7 @@ public Builder addLabel(@CompileTimeConstant String arg, @Nullable Label value)
* <p>Prefer this over manually calling {@link PathFragment#getPathString}, as it avoids storing
* a copy of the path string.
*/
@CanIgnoreReturnValue
public Builder addPath(@Nullable PathFragment value) {
return addObjectInternal(value);
}
Expand All @@ -841,6 +862,7 @@ public Builder addPath(@Nullable PathFragment value) {
*
* <p>If the value is null, neither the arg nor the value is added.
*/
@CanIgnoreReturnValue
public Builder addPath(@CompileTimeConstant String arg, @Nullable PathFragment value) {
return addObjectInternal(arg, value);
}
Expand All @@ -851,6 +873,7 @@ public Builder addPath(@CompileTimeConstant String arg, @Nullable PathFragment v
* <p>Prefer this over manually calling {@link Artifact#getExecPath}, as it avoids storing a
* copy of the artifact path string.
*/
@CanIgnoreReturnValue
public Builder addExecPath(@Nullable Artifact value) {
return addObjectInternal(value);
}
Expand All @@ -863,16 +886,19 @@ public Builder addExecPath(@Nullable Artifact value) {
*
* <p>If the value is null, neither the arg nor the value is added.
*/
@CanIgnoreReturnValue
public Builder addExecPath(@CompileTimeConstant String arg, @Nullable Artifact value) {
return addObjectInternal(arg, value);
}

/** Adds a lazily expanded string. */
@CanIgnoreReturnValue
public Builder addLazyString(@Nullable OnDemandString value) {
return addObjectInternal(value);
}

/** Adds a lazily expanded string. */
@CanIgnoreReturnValue
public Builder addLazyString(@CompileTimeConstant String arg, @Nullable OnDemandString value) {
return addObjectInternal(arg, value);
}
Expand All @@ -887,23 +913,33 @@ public Builder addFormatted(@FormatString String formatStr, Object... args) {
}

/** Concatenates the passed prefix string and the string. */
@CanIgnoreReturnValue
public Builder addPrefixed(@CompileTimeConstant String prefix, @Nullable String arg) {
return addPrefixedInternal(prefix, arg);
return addPrefixedInternal(prefix, arg, /* mainRepoMapping= */ null);
}

/** Concatenates the passed prefix string and the label using {@link Label#getCanonicalForm}. */
public Builder addPrefixedLabel(@CompileTimeConstant String prefix, @Nullable Label arg) {
return addPrefixedInternal(prefix, arg);
/**
* Concatenates the passed prefix string and the label using {@link Label#getDisplayForm}, which
* is identical to {@link Label#getCanonicalForm()} for main repo labels.
*/
@CanIgnoreReturnValue
public Builder addPrefixedLabel(
@CompileTimeConstant String prefix,
@Nullable Label arg,
RepositoryMapping mainRepoMapping) {
return addPrefixedInternal(prefix, arg, mainRepoMapping);
}

/** Concatenates the passed prefix string and the path. */
@CanIgnoreReturnValue
public Builder addPrefixedPath(@CompileTimeConstant String prefix, @Nullable PathFragment arg) {
return addPrefixedInternal(prefix, arg);
return addPrefixedInternal(prefix, arg, /* mainRepoMapping= */ null);
}

/** Concatenates the passed prefix string and the artifact's exec path. */
@CanIgnoreReturnValue
public Builder addPrefixedExecPath(@CompileTimeConstant String prefix, @Nullable Artifact arg) {
return addPrefixedInternal(prefix, arg);
return addPrefixedInternal(prefix, arg, /* mainRepoMapping= */ null);
}

/**
Expand All @@ -912,6 +948,7 @@ public Builder addPrefixedExecPath(@CompileTimeConstant String prefix, @Nullable
* <p>If you are converting long lists or nested sets of a different type to string lists,
* please try to use a different method that supports what you are trying to do directly.
*/
@CanIgnoreReturnValue
public Builder addAll(@Nullable Collection<String> values) {
return addCollectionInternal(values);
}
Expand All @@ -922,6 +959,7 @@ public Builder addAll(@Nullable Collection<String> values) {
* <p>If you are converting long lists or nested sets of a different type to string lists,
* please try to use a different method that supports what you are trying to do directly.
*/
@CanIgnoreReturnValue
public Builder addAll(@Nullable NestedSet<String> values) {
return addNestedSetInternal(values);
}
Expand All @@ -934,6 +972,7 @@ public Builder addAll(@Nullable NestedSet<String> values) {
*
* <p>If values is empty, the arg isn't added.
*/
@CanIgnoreReturnValue
public Builder addAll(@CompileTimeConstant String arg, @Nullable Collection<String> values) {
return addCollectionInternal(arg, values);
}
Expand All @@ -943,11 +982,13 @@ public Builder addAll(@CompileTimeConstant String arg, @Nullable Collection<Stri
*
* <p>If values is empty, the arg isn't added.
*/
@CanIgnoreReturnValue
public Builder addAll(@CompileTimeConstant String arg, @Nullable NestedSet<String> values) {
return addNestedSetInternal(arg, values);
}

/** Adds the passed vector arg. See {@link VectorArg}. */
@CanIgnoreReturnValue
public Builder addAll(VectorArg<String> vectorArg) {
return addVectorArgInternal(vectorArg);
}
Expand All @@ -957,16 +998,19 @@ public Builder addAll(VectorArg<String> vectorArg) {
*
* <p>If values is empty, the arg isn't added.
*/
@CanIgnoreReturnValue
public Builder addAll(@CompileTimeConstant String arg, VectorArg<String> vectorArg) {
return addVectorArgInternal(arg, vectorArg);
}

/** Adds the passed paths to the command line. */
@CanIgnoreReturnValue
public Builder addPaths(@Nullable Collection<PathFragment> values) {
return addCollectionInternal(values);
}

/** Adds the passed paths to the command line. */
@CanIgnoreReturnValue
public Builder addPaths(@Nullable NestedSet<PathFragment> values) {
return addNestedSetInternal(values);
}
Expand All @@ -976,6 +1020,7 @@ public Builder addPaths(@Nullable NestedSet<PathFragment> values) {
*
* <p>If values is empty, the arg isn't added.
*/
@CanIgnoreReturnValue
public Builder addPaths(
@CompileTimeConstant String arg, @Nullable Collection<PathFragment> values) {
return addCollectionInternal(arg, values);
Expand All @@ -986,12 +1031,14 @@ public Builder addPaths(
*
* <p>If values is empty, the arg isn't added.
*/
@CanIgnoreReturnValue
public Builder addPaths(
@CompileTimeConstant String arg, @Nullable NestedSet<PathFragment> values) {
return addNestedSetInternal(arg, values);
}

/** Adds the passed vector arg. See {@link VectorArg}. */
@CanIgnoreReturnValue
public Builder addPaths(VectorArg<PathFragment> vectorArg) {
return addVectorArgInternal(vectorArg);
}
Expand All @@ -1001,6 +1048,7 @@ public Builder addPaths(VectorArg<PathFragment> vectorArg) {
*
* <p>If values is empty, the arg isn't added.
*/
@CanIgnoreReturnValue
public Builder addPaths(@CompileTimeConstant String arg, VectorArg<PathFragment> vectorArg) {
return addVectorArgInternal(arg, vectorArg);
}
Expand All @@ -1011,11 +1059,13 @@ public Builder addPaths(@CompileTimeConstant String arg, VectorArg<PathFragment>
* <p>Do not use this method if the list is derived from a flattened nested set. Instead, figure
* out how to avoid flattening the set and use {@link #addExecPaths(NestedSet)}.
*/
@CanIgnoreReturnValue
public Builder addExecPaths(@Nullable Collection<Artifact> values) {
return addCollectionInternal(values);
}

/** Adds the artifacts' exec paths to the command line. */
@CanIgnoreReturnValue
public Builder addExecPaths(@Nullable NestedSet<Artifact> values) {
return addNestedSetInternal(values);
}
Expand All @@ -1028,6 +1078,7 @@ public Builder addExecPaths(@Nullable NestedSet<Artifact> values) {
*
* <p>If values is empty, the arg isn't added.
*/
@CanIgnoreReturnValue
public Builder addExecPaths(
@CompileTimeConstant String arg, @Nullable Collection<Artifact> values) {
return addCollectionInternal(arg, values);
Expand All @@ -1038,12 +1089,14 @@ public Builder addExecPaths(
*
* <p>If values is empty, the arg isn't added.
*/
@CanIgnoreReturnValue
public Builder addExecPaths(
@CompileTimeConstant String arg, @Nullable NestedSet<Artifact> values) {
return addNestedSetInternal(arg, values);
}

/** Adds the passed vector arg. See {@link VectorArg}. */
@CanIgnoreReturnValue
public Builder addExecPaths(VectorArg<Artifact> vectorArg) {
return addVectorArgInternal(vectorArg);
}
Expand All @@ -1053,6 +1106,7 @@ public Builder addExecPaths(VectorArg<Artifact> vectorArg) {
*
* <p>If values is empty, the arg isn't added.
*/
@CanIgnoreReturnValue
public Builder addExecPaths(@CompileTimeConstant String arg, VectorArg<Artifact> vectorArg) {
return addVectorArgInternal(arg, vectorArg);
}
Expand Down Expand Up @@ -1129,10 +1183,11 @@ private Builder addObjectInternal(@CompileTimeConstant String arg, @Nullable Obj
}

@CanIgnoreReturnValue
private Builder addPrefixedInternal(String prefix, @Nullable Object arg) {
private Builder addPrefixedInternal(
String prefix, @Nullable Object arg, @Nullable RepositoryMapping mainRepoMapping) {
Preconditions.checkNotNull(prefix);
if (arg != null) {
PrefixArg.push(arguments, prefix, arg);
PrefixArg.push(arguments, prefix, arg, mainRepoMapping);
}
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -893,7 +893,7 @@ public void addToFingerprint(
throws CommandLineExpansionException, InterruptedException {
List<Object> arguments = rawArgsAsList();
int size;
if (arguments.getLast() instanceof RepositoryMapping mainRepoMapping) {
if (!arguments.isEmpty() && arguments.getLast() instanceof RepositoryMapping mainRepoMapping) {
fingerprint.addStringMap(
Maps.transformValues(mainRepoMapping.entries(), RepositoryName::getName));
size = arguments.size() - 1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,8 @@ public JavaCompileOutputs<Artifact> createOutputs(JavaOutput output) {
return builder.build();
}

public void createCompileAction(JavaCompileOutputs<Artifact> outputs) throws RuleErrorException {
public void createCompileAction(JavaCompileOutputs<Artifact> outputs)
throws RuleErrorException, InterruptedException {
if (outputs.genClass() != null) {
createGenJarAction(
outputs.output(),
Expand Down Expand Up @@ -544,7 +545,7 @@ private Artifact turbineOutput(Artifact classJar, String newExtension) {
* @param headerDeps the .jdeps output of this java compilation
*/
public void createHeaderCompilationAction(Artifact headerJar, Artifact headerDeps)
throws RuleErrorException {
throws RuleErrorException, InterruptedException {

JavaTargetAttributes attributes = getAttributes();

Expand Down Expand Up @@ -689,7 +690,8 @@ public void createSourceJarAction(Artifact outputJar, @Nullable Artifact gensrcJ
* @return the header jar (if requested), or ijar (if requested), or else the class jar
*/
public Artifact createCompileTimeJarAction(
Artifact runtimeJar, JavaCompilationArtifacts.Builder builder) throws RuleErrorException {
Artifact runtimeJar, JavaCompilationArtifacts.Builder builder)
throws RuleErrorException, InterruptedException {
Artifact jar;
boolean isFullJar = false;
if (shouldUseHeaderCompilation()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ public JavaCompileActionBuilder(
this.execGroup = execGroup;
}

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

Expand Down Expand Up @@ -271,7 +271,7 @@ private ImmutableSet<Artifact> allOutputs() {
}

private CustomCommandLine buildParamFileContents(ImmutableList<String> javacOpts)
throws RuleErrorException {
throws RuleErrorException, InterruptedException {

CustomCommandLine.Builder result = CustomCommandLine.builder();

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

0 comments on commit 035ee82

Please sign in to comment.