Skip to content

Commit

Permalink
Emit labels in display form in Java rules
Browse files Browse the repository at this point in the history
Buildozer fixes for Java strict deps violations referring to external repositories now contain labels in display form, which avoid canonical repository names that should never be added to BUILD files and aren't understood by `buildozer`.

The Starlark actions can use `Label.to_display_form()`. Java compilation actions, which are still implemented in Java, can't access the main repository mapping via `BazelModuleContext` and instead retrieve it from a new field on `AnalysisEnvironment`.

Closes bazelbuild#21180.

PiperOrigin-RevId: 607803030
Change-Id: I0cdccabfde0217c9201cef9ca9d260b0c8ca27cd
  • Loading branch information
fmeum authored and copybara-github committed Feb 16, 2024
1 parent 31fae9e commit b11fa7a
Show file tree
Hide file tree
Showing 22 changed files with 254 additions and 32 deletions.
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 @@ -16,13 +16,17 @@
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.CommandLineItem.ExceptionlessMapFn;
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 +42,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 @@ -50,6 +55,7 @@
import com.google.devtools.build.lib.rules.java.JavaToolchainProvider.JspecifyInfo;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.Serializable;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
Expand All @@ -63,6 +69,33 @@
*/
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 ->
(ExceptionlessMapFn<Label> & Serializable)
(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 @@ -38,6 +38,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 @@ -439,14 +440,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,7 @@
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.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 +764,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
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import com.google.devtools.build.lib.causes.AnalysisFailedCause;
import com.google.devtools.build.lib.causes.Cause;
import com.google.devtools.build.lib.cmdline.Label;
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.collect.nestedset.Order;
Expand Down Expand Up @@ -373,11 +374,21 @@ private static ConfiguredTargetValue createConfiguredTarget(
if (starlarkBuiltinsValue == null) {
return null;
}
RepositoryMappingValue mainRepoMappingValue =
(RepositoryMappingValue) env.getValue(RepositoryMappingValue.key(RepositoryName.MAIN));
if (mainRepoMappingValue == null) {
return null;
}

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

Preconditions.checkNotNull(depValueMap);
ConfiguredTarget configuredTarget;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildMetrics.BuildGraphMetrics;
import com.google.devtools.build.lib.causes.AnalysisFailedCause;
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 Down Expand Up @@ -1267,7 +1268,8 @@ CachingAnalysisEnvironment createAnalysisEnvironment(
ExtendedEventHandler eventHandler,
Environment env,
BuildConfigurationValue config,
StarlarkBuiltinsValue starlarkBuiltinsValue) {
StarlarkBuiltinsValue starlarkBuiltinsValue,
RepositoryMapping mainRepoMapping) {
boolean extendedSanityChecks = config != null && config.extendedSanityChecks();
boolean allowAnalysisFailures = config != null && config.allowAnalysisFailures();
return new CachingAnalysisEnvironment(
Expand All @@ -1278,7 +1280,8 @@ CachingAnalysisEnvironment createAnalysisEnvironment(
allowAnalysisFailures,
eventHandler,
env,
starlarkBuiltinsValue);
starlarkBuiltinsValue,
mainRepoMapping);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ def _android_lint_action(ctx, source_files, source_jars, compilation_info):
args.add_all("--bootclasspath", bootclasspath)
args.add_all("--classpath", classpath)
args.add_all("--lint_rules", compilation_info.plugins.processor_jars)
args.add("--target_label", ctx.label)
args.add_all("--target_label", [ctx.label], map_each = helper.map_to_display_form)

javac_opts = compilation_info.javac_options
if javac_opts:
Expand Down
2 changes: 1 addition & 1 deletion src/main/starlark/builtins_bzl/common/java/java_common.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ def _stamp_jar(actions, jar, java_toolchain, target_label):
args.add(jar)
args.add(output)
args.add("--nostrip_jar")
args.add("--target_label", target_label)
args.add_all("--target_label", [target_label], map_each = helper.map_to_display_form)
actions.run(
mnemonic = "JavaIjar",
inputs = [jar],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ def run_ijar(
args.add(jar)
args.add(output)
if target_label != None:
args.add("--target_label", target_label)
args.add_all("--target_label", [target_label], map_each = helper.map_to_display_form)
if injecting_rule_kind != None:
args.add("--injecting_rule_kind", injecting_rule_kind)

Expand Down
12 changes: 12 additions & 0 deletions src/main/starlark/builtins_bzl/common/java/java_helper.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,17 @@ def _derive_output_file(ctx, base_file, *, name_suffix = "", extension = None, e
new_basename = paths.replace_extension(base_file.basename, name_suffix + "." + extension + extension_suffix)
return ctx.actions.declare_file(new_basename, sibling = base_file)

def _map_to_display_form(label):
"""`map_each` callback that formats a `Label` with `Label.to_display_form()`
Args:
label: (Label) the label of a target.
Returns:
(str) the display form representation of `label`
"""
return label.to_display_form()

helper = struct(
collect_all_targets_as_deps = _collect_all_targets_as_deps,
filter_launcher_for_target = _filter_launcher_for_target,
Expand All @@ -479,4 +490,5 @@ helper = struct(
tokenize_javacopts = _tokenize_javacopts,
detokenize_javacopts = _detokenize_javacopts,
derive_output_file = _derive_output_file,
map_to_display_form = _map_to_display_form,
)
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
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.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
Expand Down Expand Up @@ -225,6 +226,11 @@ public ImmutableSet<Artifact> getTreeArtifactsConflictingWithFiles() {
public ActionKeyContext getActionKeyContext() {
return original.getActionKeyContext();
}

@Override
public RepositoryMapping getMainRepoMapping() {
return original.getMainRepoMapping();
}
}

/** A dummy WorkspaceStatusAction. */
Expand Down Expand Up @@ -455,6 +461,11 @@ public ImmutableSet<Artifact> getTreeArtifactsConflictingWithFiles() {
public ActionKeyContext getActionKeyContext() {
return null;
}

@Override
public RepositoryMapping getMainRepoMapping() {
throw new UnsupportedOperationException();
}
}

/** Matches the output path prefix contributed by a C++ configuration fragment. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/skyframe:diff_awareness",
"//src/main/java/com/google/devtools/build/lib/skyframe:package_roots_no_symlink_creation",
"//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
"//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",
"//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_executor_repository_helpers_holder",
Expand Down
Loading

0 comments on commit b11fa7a

Please sign in to comment.