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

Emit labels in display form in Java rules #21180

Closed
wants to merge 1 commit 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 @@ -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,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 @@ -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,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
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
Loading