Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Label.to_display_name() #21120

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -351,11 +351,22 @@ public String get(Iterable<JarOwner> missing, String recipient) {
if (missingTargets.isEmpty()) {
return "";
}
String buildozerCommand = "";
// buildozer can't modify BUILD files in external repositories.
if (!recipient.startsWith("@")) {
buildozerCommand =
String.format(
"%1$s ** You can use the following buildozer command:%2$s "
+ "\nbuildozer 'add deps %3$s' %4$s \n",
"\033[35m\033[1m", "\033[0m", Joiner.on(" ").join(missingTargets), recipient);
}
return String.format(
"%1$s ** Please add the following dependencies:%2$s \n %3$s to %4$s \n"
+ "%1$s ** You can use the following buildozer command:%2$s "
+ "\nbuildozer 'add deps %3$s' %4$s \n\n",
"\033[35m\033[1m", "\033[0m", Joiner.on(" ").join(missingTargets), recipient);
"%1$s ** Please add the following dependencies:%2$s \n %3$s to %4$s \n%5$s\n",
"\033[35m\033[1m",
"\033[0m",
Joiner.on(" ").join(missingTargets),
recipient,
buildozerCommand);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -373,13 +373,9 @@ private String getProgressMessageChecked(@Nullable RepositoryMapping mainReposit
private String replaceProgressMessagePlaceholders(
String progressMessage, @Nullable RepositoryMapping mainRepositoryMapping) {
if (progressMessage.contains("%{label}") && owner.getLabel() != null) {
String labelString;
if (mainRepositoryMapping != null) {
labelString = owner.getLabel().getDisplayForm(mainRepositoryMapping);
} else {
labelString = owner.getLabel().toString();
}
progressMessage = progressMessage.replace("%{label}", labelString);
progressMessage =
progressMessage.replace(
"%{label}", owner.getLabel().getDisplayForm(mainRepositoryMapping));
}
if (progressMessage.contains("%{output}") && getPrimaryOutput() != null) {
progressMessage =
Expand Down
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,10 @@ Artifact.DerivedArtifact getDerivedArtifact(
ImmutableSet<Artifact> getTreeArtifactsConflictingWithFiles();

ActionKeyContext getActionKeyContext();

/**
* The repository mapping applicable to the main repository. This is purely meant to support
* {@link com.google.devtools.build.lib.cmdline.Label#getDisplayForm)}.
*/
RepositoryMapping getMainRepoMapping();
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
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.events.ExtendedEventHandler;
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.Target;
Expand Down Expand Up @@ -84,6 +85,8 @@ public final class CachingAnalysisEnvironment implements AnalysisEnvironment {
*/
private final List<ActionAnalysisMetadata> actions = new ArrayList<>();

private final RepositoryMapping mainRepoMapping;

public CachingAnalysisEnvironment(
ArtifactFactory artifactFactory,
ActionKeyContext actionKeyContext,
Expand All @@ -92,7 +95,8 @@ public CachingAnalysisEnvironment(
boolean allowAnalysisFailures,
ExtendedEventHandler errorEventListener,
SkyFunction.Environment env,
StarlarkBuiltinsValue starlarkBuiltinsValue) {
StarlarkBuiltinsValue starlarkBuiltinsValue,
RepositoryMapping mainRepoMapping) {
this.artifactFactory = artifactFactory;
this.actionKeyContext = actionKeyContext;
this.owner = Preconditions.checkNotNull(owner);
Expand All @@ -101,6 +105,7 @@ public CachingAnalysisEnvironment(
this.errorEventListener = errorEventListener;
this.skyframeEnv = env;
this.starlarkBuiltinsValue = starlarkBuiltinsValue;
this.mainRepoMapping = mainRepoMapping;
middlemanFactory = new MiddlemanFactory(artifactFactory, this);
}

Expand Down Expand Up @@ -227,6 +232,11 @@ public ActionKeyContext getActionKeyContext() {
return actionKeyContext;
}

@Override
public RepositoryMapping getMainRepoMapping() {
return mainRepoMapping;
}

@Override
public boolean hasErrors() {
Preconditions.checkState(enabled);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleContext;
import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.collect.ImmutableSortedKeyListMultimap;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
Expand Down Expand Up @@ -375,6 +376,11 @@ public String getWorkspaceName() {
return rule.getPackage().getWorkspaceName();
}

/** Returns the repository mapping of the main repository. */
public RepositoryMapping getMainRepoMapping() {
return getAnalysisEnvironment().getMainRepoMapping();
}

/** The configuration conditions that trigger this rule's configurable attributes. */
public ImmutableMap<Label, ConfigMatchingProvider> getConfigConditions() {
return configConditions;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,13 @@ public abstract class BazelModuleContext {
/** The repository mapping applicable to the repo where the .bzl file is located in. */
public abstract RepositoryMapping repoMapping();

/**
* The repository mapping applicable to the main repository, possibly without WORKSPACE repos or
* null. This is purely meant to support {@link Label#getDisplayFormForStarlark(StarlarkThread)}.
*/
@Nullable
public abstract RepositoryMapping bestEffortMainRepoMapping();

/** Returns the name of the module's .bzl file, as provided to the parser. */
public abstract String filename();

Expand Down Expand Up @@ -160,11 +167,12 @@ public static BazelModuleContext ofInnermostBzlOrFail(StarlarkThread thread, Str
public static BazelModuleContext create(
Label label,
RepositoryMapping repoMapping,
@Nullable RepositoryMapping bestEffortMainRepoMapping,
String filename,
ImmutableList<Module> loads,
byte[] bzlTransitiveDigest) {
return new AutoValue_BazelModuleContext(
label, repoMapping, filename, loads, bzlTransitiveDigest);
label, repoMapping, bestEffortMainRepoMapping, filename, loads, bzlTransitiveDigest);
}

public final Label.PackageContext packageContext() {
Expand Down
20 changes: 19 additions & 1 deletion src/main/java/com/google/devtools/build/lib/cmdline/Label.java
Original file line number Diff line number Diff line change
Expand Up @@ -438,10 +438,28 @@ public String getUnambiguousCanonicalForm() {
* @param mainRepositoryMapping the {@link RepositoryMapping} of the main repository
* @return analogous to {@link PackageIdentifier#getDisplayForm(RepositoryMapping)}
*/
public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
public String getDisplayForm(@Nullable RepositoryMapping mainRepositoryMapping) {
return packageIdentifier.getDisplayForm(mainRepositoryMapping) + ":" + name;
}

@StarlarkMethod(
name = "to_display_form",
useStarlarkThread = true,
doc =
"Returns a string representation of this label that is optimized for human readability."
+ " The exact form of the return value is explicitly unspecified and subject to"
+ " change."
+ " The following properties are guaranteed for a <code>Label</code> <code>l</code>:"
+ "<ul>"
+ " <li><code>l.to_display_form()</code> has no repository part if and only if <code>l</code> references the main repository;</li>"
+ " <li><code>Label(l.to_display_form()) == l</code> if the call to <code>Label</code> occurs in the main repository.</li>"
+ "</ul>")
public String getDisplayFormForStarlark(StarlarkThread starlarkThread) throws EvalException {
checkRepoVisibilityForStarlark("to_display_form");
return getDisplayForm(
BazelModuleContext.ofInnermostBzlOrThrow(starlarkThread).bestEffortMainRepoMapping());
}

/**
* Returns a shorthand label string that is suitable for display, i.e. in addition to simplifying
* the repository part, labels of the form {@code [@repo]//foo/bar:bar} are simplified to the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyKey.SkyKeyInterner;

import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;

/**
Expand Down Expand Up @@ -222,7 +224,7 @@ public String getUnambiguousCanonicalForm() {
* <dd>only with Bzlmod if the current package belongs to a repository that is not visible
* from the main module
*/
public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
public String getDisplayForm(@Nullable RepositoryMapping mainRepositoryMapping) {
return repository.getDisplayForm(mainRepositoryMapping) + "//" + pkgName;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,20 +247,25 @@ public String getCanonicalForm() {
* <dt><code>@protobuf</code>
* <dd>if this repository is a WORKSPACE dependency and its <code>name</code> is "protobuf",
* or if this repository is a Bzlmod dependency of the main module and its apparent name
* is "protobuf"
* is "protobuf" (in both cases only if mainRepositoryMapping is not null)
* <dt><code>@@protobuf~3.19.2</code>
* <dd>only with Bzlmod, if this a repository that is not visible from the main module
*/
public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
public String getDisplayForm(@Nullable RepositoryMapping mainRepositoryMapping) {
Preconditions.checkArgument(
mainRepositoryMapping.ownerRepo() == null || mainRepositoryMapping.ownerRepo().isMain());
mainRepositoryMapping == null
|| mainRepositoryMapping.ownerRepo() == null
|| mainRepositoryMapping.ownerRepo().isMain());
if (!isVisible()) {
return getNameWithAt();
}
if (isMain()) {
// Packages in the main repository can always use repo-relative form.
return "";
}
if (mainRepositoryMapping == null) {
return getNameWithAt();
}
if (!mainRepositoryMapping.usesStrictDeps()) {
// If the main repository mapping is not using strict visibility, then Bzlmod is certainly
// disabled, which means that canonical and apparent names can be used interchangeably from
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,16 @@
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.devtools.build.lib.packages.ExecGroup.DEFAULT_EXEC_GROUP_NAME;

import com.github.benmanes.caffeine.cache.Caffeine;
import com.github.benmanes.caffeine.cache.LoadingCache;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Interner;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.CommandLineItem;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.actions.ParamFileInfo;
import com.google.devtools.build.lib.actions.ParameterFile;
Expand All @@ -38,6 +41,7 @@
import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.StrictDepsMode;
import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector;
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.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
Expand All @@ -63,6 +67,31 @@
*/
public final class JavaCompilationHelper {

/**
* Cache for the {@link com.google.devtools.build.lib.actions.CommandLineItem.MapFn} that turns a
* {@link Label} into its possibly @-prefixed display form, which requires the repository mapping
* of the main repo.
*
* <p>The capacity of this cache after weakly reachable object have been cleaned will always be 1
* as there is only a single main repo mapping in a given build.
*/
static final LoadingCache<RepositoryMapping, CommandLineItem.ExceptionlessMapFn<Label>>
TARGET_LABEL_MAP_FN_CACHE =
Caffeine.newBuilder()
.initialCapacity(1)
.weakKeys()
.build(
mainRepoMapping ->
(label, args) -> {
String displayForm = label.getDisplayForm(mainRepoMapping);
if (displayForm.startsWith("@")) {
// @-prefixed strings will be assumed to be filenames and expanded by
// {@link JavaLibraryBuildRequest}, so add an extra &at; to escape it.
args.accept("@" + displayForm);
} else {
args.accept(displayForm);
}
});
private static final Interner<ImmutableList<String>> javacOptsInterner =
BlazeInterners.newWeakInterner();
private static final Interner<ImmutableMap<String, String>> executionInfoInterner =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.google.devtools.build.lib.actions.extra.JavaCompileInfo;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.VectorArg;
import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.StrictDepsMode;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
Expand Down Expand Up @@ -313,14 +314,12 @@ private CustomCommandLine buildParamFileContents(ImmutableList<String> javacOpts
result.add("--");
}
if (targetLabel != null) {
result.add("--target_label");
if (targetLabel.getRepository().isMain()) {
result.addLabel(targetLabel);
} 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.addAll(
"--target_label",
VectorArg.of(ImmutableList.of(targetLabel))
.mapped(
JavaCompilationHelper.TARGET_LABEL_MAP_FN_CACHE.get(
ruleContext.getMainRepoMapping())));
}
result.add("--injecting_rule_kind", injectingRuleKind);
// strict_java_deps controls whether the mapping from jars to targets is
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.VectorArg;
import com.google.devtools.build.lib.analysis.actions.PathMappers;
import com.google.devtools.build.lib.analysis.actions.SpawnAction;
import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.StrictDepsMode;
Expand Down Expand Up @@ -443,14 +444,12 @@ public void build(JavaToolchainProvider javaToolchain) throws RuleErrorException
}

if (targetLabel != null) {
commandLine.add("--target_label");
if (targetLabel.getRepository().isMain()) {
commandLine.addLabel(targetLabel);
} else {
// @-prefixed strings will be assumed to be params filenames and expanded,
// so add an extra @ to escape it.
commandLine.addPrefixedLabel("@", targetLabel);
}
commandLine.addAll(
"--target_label",
VectorArg.of(ImmutableList.of(targetLabel))
.mapped(
JavaCompilationHelper.TARGET_LABEL_MAP_FN_CACHE.get(
ruleContext.getMainRepoMapping())));
}

ImmutableMap<String, String> executionInfo =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@
import com.google.devtools.build.lib.causes.LabelCause;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.events.Event;
Expand Down Expand Up @@ -763,12 +765,23 @@ private AspectValue createAspect(
if (env.valuesMissing()) {
return null;
}
RepositoryMappingValue mainRepoMappingValue =
(RepositoryMappingValue) env.getValue(RepositoryMappingValue.key(RepositoryName.MAIN));
if (mainRepoMappingValue == null) {
return null;
}

SkyframeBuildView view = buildViewProvider.getSkyframeBuildView();

StoredEventHandler events = new StoredEventHandler();
CachingAnalysisEnvironment analysisEnvironment =
view.createAnalysisEnvironment(key, events, env, configuration, starlarkBuiltinsValue);
view.createAnalysisEnvironment(
key,
events,
env,
configuration,
starlarkBuiltinsValue,
mainRepoMappingValue.getRepositoryMapping());

ConfiguredAspect configuredAspect;
if (aspect.getDefinition().applyToGeneratingRules() && associatedTarget instanceof OutputFile) {
Expand Down
Loading
Loading