Skip to content

Commit

Permalink
Stringify Labels in display form in Args
Browse files Browse the repository at this point in the history 
`Label`s added to `Args` are now formatted in display form, which means that they will be repo-relative if referring to the main repo and use an apparent repository name if possible.

Previously, labels were formatted in repo-relative form if referring to the main repo and using canonical repository names exclusively for external repos. At the same time, `Args` docs explicitly stated that the exact stringification of any type other than `File` is unspecified. The previous behavior was problematic since it neither always used canonical labels (which could be useful for writing tests that check these labels against an allowlist) nor provided labels that could be included in BUILD files (canonical names are explicitly unstable). Furthermore, whether a label resulted in a string prefixed with a single or two `@`s already dependended on a user choice, namely the value of `--enable_bzlmod`.

Thus, this change is not considered to be breaking. It makes it so that existing rulesets relying on default stringification to return a BUILD-file compatible label in WORKSPACE continue to do so with Bzlmod with no code changes.

This change aims to be as memory efficient as possible:
* Single labels or sequences of labels that reference targets in the main repo incur no memory overhead.
* Labels referring to external repos as well as `NestedSet`s of labels result in an additional Skyframe dependency for each target using the command line as well as one additional reference (4 bytes) stored in the command line's `arguments`.

Work towards #20486

Closes #21702.

PiperOrigin-RevId: 620925978
Change-Id: I54aa807c41bf783aee223482d2309f5cee2726b5
  • Loading branch information
@fmeum @copybara-github
fmeum authored and copybara-github committed Apr 1, 2024
1 parent 852273f commit 9d3a8b0
Show file tree
Hide file tree
Showing 18 changed files with 450 additions and 75 deletions.
6 changes: 6 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/AnalysisEnvironment.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.actions.MiddlemanFactory;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyFunction;
Expand Down Expand Up @@ -166,4 +167,9 @@ Artifact.DerivedArtifact getDerivedArtifact(
ImmutableSet<Artifact> getTreeArtifactsConflictingWithFiles();

ActionKeyContext getActionKeyContext();

/**
* Returns and registers a Skyframe dependency on the {@link RepositoryMapping} of the main repo.
*/
RepositoryMapping getMainRepoMapping() throws InterruptedException;
}
4 changes: 4 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",
"//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key",
"//src/main/java/com/google/devtools/build/lib/skyframe:configured_value_creation_exception",
"//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:starlark_builtins_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:workspace_status_value",
"//src/main/java/com/google/devtools/build/lib/skyframe/config",
Expand All @@ -421,6 +422,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/core",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/platform",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test",
"//src/main/java/com/google/devtools/build/lib/supplier",
"//src/main/java/com/google/devtools/build/lib/unsafe:string",
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
Expand Down Expand Up @@ -2450,9 +2452,11 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",
"//src/main/java/com/google/devtools/build/lib/supplier",
"//src/main/java/net/starlark/java/eval",
"//src/main/java/net/starlark/java/syntax",
"//third_party:guava",
Expand Down
16 changes: 16 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,12 @@
import com.google.devtools.build.lib.actions.ArtifactFactory;
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.actions.MiddlemanFactory;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.skyframe.RepositoryMappingValue;
import com.google.devtools.build.lib.skyframe.StarlarkBuiltinsValue;
import com.google.devtools.build.lib.skyframe.WorkspaceStatusValue;
import com.google.devtools.build.lib.util.Pair;
Expand Down Expand Up @@ -368,6 +371,19 @@ private WorkspaceStatusValue getWorkspaceStatusValue() throws InterruptedExcepti
return workspaceStatusValue;
}

@Override
public RepositoryMapping getMainRepoMapping() throws InterruptedException {
var mainRepoMapping =
(RepositoryMappingValue)
skyframeEnv.getValue(RepositoryMappingValue.key(RepositoryName.MAIN));
if (mainRepoMapping == null) {
// This isn't expected to happen since the main repository mapping is computed before the
// analysis phase.
throw new MissingDepException("Restart due to missing main repository mapping");
}
return mainRepoMapping.getRepositoryMapping();
}

@Override
public ActionLookupKey getOwner() {
return owner;
Expand Down
59 changes: 42 additions & 17 deletions src/main/java/com/google/devtools/build/lib/analysis/starlark/Args.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,13 @@
import com.google.devtools.build.lib.actions.ParamFileInfo;
import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
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.Depset;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.starlarkbuildapi.CommandLineArgsApi;
import com.google.devtools.build.lib.supplier.InterruptibleSupplier;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
Expand Down Expand Up @@ -70,7 +73,8 @@ public void repr(Printer printer) {
@Override
public void debugPrint(Printer printer, StarlarkSemantics semantics) {
try {
printer.append(Joiner.on(" ").join(build().arguments()));
printer.append(
Joiner.on(" ").join(build(/* mainRepoMappingSupplier= */ () -> null).arguments()));
} catch (CommandLineExpansionException e) {
printer.append("Cannot expand command line: " + e.getMessage());
} catch (InterruptedException e) {
Expand Down Expand Up @@ -102,7 +106,8 @@ public void debugPrint(Printer printer, StarlarkSemantics semantics) {
public abstract ImmutableSet<Artifact> getDirectoryArtifacts();

/** Returns the command line built by this {@link Args} object. */
public abstract CommandLine build();
public abstract CommandLine build(
InterruptibleSupplier<RepositoryMapping> mainRepoMappingSupplier) throws InterruptedException;

/**
* Returns a frozen {@link Args} representation corresponding to an already-registered action.
Expand Down Expand Up @@ -157,7 +162,7 @@ public ImmutableSet<Artifact> getDirectoryArtifacts() {
}

@Override
public CommandLine build() {
public CommandLine build(InterruptibleSupplier<RepositoryMapping> mainRepoMappingSupplier) {
return commandLine;
}

Expand Down Expand Up @@ -259,6 +264,12 @@ private static class MutableArgs extends Args implements StarlarkValue, Mutabili
*/
private boolean flagPerLine = false;

/**
* True if the command line needs to stringify any {@link Label}s without an explicit 'map_each'
* function.
*/
private boolean mayStringifyExternalLabel = false;

// May be set explicitly once -- if unset defaults to ParameterFileType.SHELL_QUOTED.
private ParameterFileType parameterFileType = null;
private String flagFormatString;
Expand Down Expand Up @@ -307,6 +318,9 @@ public CommandLineArgsApi addArgument(
"Args.add() doesn't accept vectorized arguments. Please use Args.add_all() or"
+ " Args.add_joined() instead.");
}
if (value instanceof Label label && !label.getRepository().isMain()) {
mayStringifyExternalLabel = true;
}
addSingleArg(value, format != Starlark.NONE ? (String) format : null);
return this;
}
Expand Down Expand Up @@ -436,8 +450,12 @@ private void addVectorArg(
validateFormatString("format_each", formatEach);
validateFormatString("format_joined", formatJoined);
StarlarkCustomCommandLine.VectorArg.Builder vectorArg;
if (value instanceof Depset) {
Depset starlarkNestedSet = (Depset) value;
if (value instanceof Depset starlarkNestedSet) {
if (mapEach == null && Label.class.equals(starlarkNestedSet.getElementClass())) {
// We don't want to eagerly check whether all labels reference targets in the main repo,
// so just assume they might not. Nested sets of labels should be rare.
mayStringifyExternalLabel = true;
}
NestedSet<?> nestedSet = starlarkNestedSet.getSet();
if (nestedSet.isEmpty() && omitIfEmpty) {
return;
Expand All @@ -451,8 +469,16 @@ private void addVectorArg(
if (starlarkList.isEmpty() && omitIfEmpty) {
return;
}
if (expandDirectories) {
scanForDirectories(starlarkList);
for (Object object : starlarkList) {
if (expandDirectories && isDirectory(object)) {
directoryArtifacts.add((Artifact) object);
}
// Labels referencing targets in the main repo are stringified as //pkg:name and thus
// don't require a RepositoryMapping. If a map_each function is provided, default
// stringification via Label#toString() is not used.
if (mapEach == null && object instanceof Label label && !label.getRepository().isMain()) {
mayStringifyExternalLabel = true;
}
}
vectorArg = new StarlarkCustomCommandLine.VectorArg.Builder(starlarkList);
}
Expand Down Expand Up @@ -573,8 +599,10 @@ private MutableArgs(@Nullable Mutability mutability, StarlarkSemantics starlarkS
}

@Override
public CommandLine build() {
return commandLine.build(flagPerLine);
public CommandLine build(InterruptibleSupplier<RepositoryMapping> mainRepoMappingSupplier)
throws InterruptedException {
return commandLine.build(
flagPerLine, mayStringifyExternalLabel ? mainRepoMappingSupplier.get() : null);
}

@Override
Expand All @@ -585,18 +613,15 @@ public Mutability mutability() {
@Override
public ImmutableSet<Artifact> getDirectoryArtifacts() {
for (NestedSet<?> collection : potentialDirectoryArtifacts) {
scanForDirectories(collection.toList());
for (Object object : collection.toList()) {
if (isDirectory(object)) {
directoryArtifacts.add((Artifact) object);
}
}
}
potentialDirectoryArtifacts.clear();
return ImmutableSet.copyOf(directoryArtifacts);
}

private void scanForDirectories(Iterable<?> objects) {
for (Object object : objects) {
if (isDirectory(object)) {
directoryArtifacts.add((Artifact) object);
}
}
}
}
}
28 changes: 19 additions & 9 deletions src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.collect.nestedset.Depset;
import com.google.devtools.build.lib.collect.nestedset.Depset.TypeException;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
Expand All @@ -68,6 +69,7 @@
import com.google.devtools.build.lib.starlarkbuildapi.FileApi;
import com.google.devtools.build.lib.starlarkbuildapi.StarlarkActionFactoryApi;
import com.google.devtools.build.lib.starlarkbuildapi.TemplateDictApi;
import com.google.devtools.build.lib.supplier.InterruptibleSupplier;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.protobuf.GeneratedMessage;
Expand Down Expand Up @@ -332,7 +334,8 @@ public void symlink(
}

@Override
public void write(FileApi output, Object content, Boolean isExecutable) throws EvalException {
public void write(FileApi output, Object content, Boolean isExecutable)
throws EvalException, InterruptedException {
context.checkMutable("actions.write");
RuleContext ruleContext = getRuleContext();

Expand All @@ -347,7 +350,7 @@ public void write(FileApi output, Object content, Boolean isExecutable) throws E
ruleContext.getActionOwner(),
NestedSetBuilder.wrap(Order.STABLE_ORDER, args.getDirectoryArtifacts()),
(Artifact) output,
args.build(),
args.build(getMainRepoMappingSupplier()),
args.getParameterFileType());
} else {
throw new AssertionError("Unexpected type: " + content.getClass().getSimpleName());
Expand All @@ -373,7 +376,7 @@ public void run(
Object shadowedActionUnchecked,
Object resourceSetUnchecked,
Object toolchainUnchecked)
throws EvalException {
throws EvalException, InterruptedException {
context.checkMutable("actions.run");
execGroupUnchecked = context.maybeOverrideExecGroup(execGroupUnchecked);
toolchainUnchecked = context.maybeOverrideToolchain(toolchainUnchecked);
Expand All @@ -382,7 +385,7 @@ public void run(
boolean useAutoExecGroups = ruleContext.useAutoExecGroups();

StarlarkAction.Builder builder = new StarlarkAction.Builder();
buildCommandLine(builder, arguments);
buildCommandLine(builder, arguments, getMainRepoMappingSupplier());
if (executableUnchecked instanceof Artifact) {
Artifact executable = (Artifact) executableUnchecked;
FilesToRunProvider provider = context.getExecutableRunfiles(executable, "executable");
Expand Down Expand Up @@ -560,15 +563,15 @@ public void runShell(
Object shadowedActionUnchecked,
Object resourceSetUnchecked,
Object toolchainUnchecked)
throws EvalException {
throws EvalException, InterruptedException {
context.checkMutable("actions.run_shell");
execGroupUnchecked = context.maybeOverrideExecGroup(execGroupUnchecked);
toolchainUnchecked = context.maybeOverrideToolchain(toolchainUnchecked);

RuleContext ruleContext = getRuleContext();

StarlarkAction.Builder builder = new StarlarkAction.Builder();
buildCommandLine(builder, arguments);
buildCommandLine(builder, arguments, getMainRepoMappingSupplier());

// When we use a shell command, add an empty argument before other arguments.
// e.g. bash -c "cmd" '' 'arg1' 'arg2'
Expand Down Expand Up @@ -631,8 +634,11 @@ public void runShell(
builder);
}

private static void buildCommandLine(SpawnAction.Builder builder, Sequence<?> argumentsList)
throws EvalException {
private static void buildCommandLine(
SpawnAction.Builder builder,
Sequence<?> argumentsList,
InterruptibleSupplier<RepositoryMapping> repoMappingSupplier)
throws EvalException, InterruptedException {
ImmutableList.Builder<String> stringArgs = null;
for (Object value : argumentsList) {
if (value instanceof String) {
Expand All @@ -647,7 +653,7 @@ private static void buildCommandLine(SpawnAction.Builder builder, Sequence<?> ar
}
Args args = (Args) value;
ParamFileInfo paramFileInfo = args.getParamFileInfo();
builder.addCommandLine(args.build(), paramFileInfo);
builder.addCommandLine(args.build(repoMappingSupplier), paramFileInfo);
} else {
throw Starlark.errorf(
"expected list of strings or ctx.actions.args() for arguments instead of %s",
Expand Down Expand Up @@ -1046,6 +1052,10 @@ public void repr(Printer printer) {
context.repr(printer);
}

private InterruptibleSupplier<RepositoryMapping> getMainRepoMappingSupplier() {
return context.getRuleContext().getAnalysisEnvironment()::getMainRepoMapping;
}

/** The analysis context for {@code Starlark} actions */
// For now, this contains methods necessary for SubruleContext to begin using
// StarlarkActionFactory without any invasive changes to the latter. It will be improved once the
Expand Down
Loading

0 comments on commit 9d3a8b0

Please sign in to comment.