Skip to content

Commit

Permalink
Remove uses of java_toolchain_alias
Browse files Browse the repository at this point in the history
This seems necessary so that we can start using C++ toolchain resolution.

Without it, rules that are using both C++ and Java toolchain can resolve to a second platform (when there is no crosscompiler available on the first platform) and then JDK binaries for the first platform get executed on the second one.

It's only necessary to use the toolchain directly on rules that also have other toolchains, like java_binary.

However on rules, where only JDK toolchain is needed like java_library, the whole mechanism seems to work because a Java toolchain is always available on the first platform. If that wasn't the case - toolchain resolution in java_library would still select first platform due to the use of alias (and the alias would select second execution platform).

(1 execution platform is matched to each target, based on the toolchains required by the rule)

PiperOrigin-RevId: 472313976
Change-Id: I1905c25e5caa6fb5269f1a9b9ee12bf1b9c58ba9
  • Loading branch information
comius authored and copybara-github committed Sep 5, 2022
1 parent 73ed74e commit c2ccb28
Show file tree
Hide file tree
Showing 15 changed files with 97 additions and 63 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception",
"//src/main/java/com/google/devtools/build/lib/analysis:config/toolchain_type_requirement",
"//src/main/java/com/google/devtools/build/lib/analysis:file_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:package_specification_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:platform_options",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@
import static com.google.devtools.build.lib.packages.Attribute.attr;
import static com.google.devtools.build.lib.packages.BuildType.LABEL;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.analysis.RuleDefinition;
import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
import com.google.devtools.build.lib.analysis.config.ToolchainTypeRequirement;
import com.google.devtools.build.lib.analysis.platform.ToolchainInfo;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
Expand All @@ -28,8 +30,28 @@
/** Common rule class definitions for Java rules. */
public class JavaRuleClasses {

public static final String JAVA_TOOLCHAIN_ATTRIBUTE_NAME = "$java_toolchain";
public static final String JAVA_RUNTIME_ATTRIBUTE_NAME = "$jvm";
private static final String JAVA_TOOLCHAIN_ATTRIBUTE_NAME = "$java_toolchain";
@VisibleForTesting public static final String JAVA_RUNTIME_ATTRIBUTE_NAME = "$jvm";

public static final String JAVA_TOOLCHAIN_TYPE_ATTRIBUTE_NAME = "$java_toolchain_type";
public static final String JAVA_RUNTIME_TOOLCHAIN_TYPE_ATTRIBUTE_NAME =
"$java_runtime_toolchain_type";
private static final String TOOLCHAIN_TYPE_LABEL = "//tools/jdk:toolchain_type";
private static final String RUNTIME_TOOLCHAIN_TYPE_LABEL = "//tools/jdk:runtime_toolchain_type";

public static ToolchainTypeRequirement javaToolchainTypeRequirement(
RuleDefinitionEnvironment env) {
return ToolchainTypeRequirement.builder(env.getToolsLabel(TOOLCHAIN_TYPE_LABEL))
.mandatory(true)
.build();
}

public static ToolchainTypeRequirement javaRuntimeToolchainTypeRequirement(
RuleDefinitionEnvironment env) {
return ToolchainTypeRequirement.builder(env.getToolsLabel(RUNTIME_TOOLCHAIN_TYPE_LABEL))
.mandatory(true)
.build();
}

/** Common attributes for rules that depend on ijar. */
public static final class IjarBaseRule implements RuleDefinition {
Expand All @@ -53,7 +75,11 @@ public static final class JavaToolchainBaseRule implements RuleDefinition {
@Override
public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) {
return builder
.addToolchainTypes(javaToolchainTypeRequirement(env))
.add(
attr(JAVA_TOOLCHAIN_TYPE_ATTRIBUTE_NAME, LABEL)
.value(javaToolchainTypeRequirement(env).toolchainType()))
.add( // TODO(b/245144242): Used by IDE integration, remove when toolchains are used
attr(JAVA_TOOLCHAIN_ATTRIBUTE_NAME, LABEL)
.useOutputLicenses()
.mandatoryProviders(ToolchainInfo.PROVIDER.id())
Expand All @@ -75,11 +101,10 @@ public static final class JavaRuntimeBaseRule implements RuleDefinition {
@Override
public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) {
return builder
.addToolchainTypes(javaRuntimeToolchainTypeRequirement(env))
.add(
attr(JAVA_RUNTIME_ATTRIBUTE_NAME, LABEL)
.value(JavaSemantics.jvmAttribute(env))
.mandatoryProviders(ToolchainInfo.PROVIDER.id())
.useOutputLicenses())
attr(JAVA_RUNTIME_TOOLCHAIN_TYPE_ATTRIBUTE_NAME, LABEL)
.value(javaRuntimeToolchainTypeRequirement(env).toolchainType()))
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

package com.google.devtools.build.lib.rules.java;

import static com.google.devtools.build.lib.rules.java.JavaRuleClasses.JAVA_RUNTIME_ATTRIBUTE_NAME;
import static com.google.devtools.build.lib.packages.BuildType.LABEL;

import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.Artifact;
Expand All @@ -25,7 +25,6 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.BuiltinProvider;
import com.google.devtools.build.lib.packages.NativeInfo;
import com.google.devtools.build.lib.rules.cpp.CcInfo;
Expand Down Expand Up @@ -76,12 +75,19 @@ public static JavaRuntimeInfo forHost(RuleContext ruleContext) {
}

public static JavaRuntimeInfo from(RuleContext ruleContext) {
return from(ruleContext, JAVA_RUNTIME_ATTRIBUTE_NAME);
ToolchainInfo toolchainInfo =
ruleContext
.getToolchainContext()
.forToolchainType(
ruleContext
.getPrerequisite(JavaRuleClasses.JAVA_RUNTIME_TOOLCHAIN_TYPE_ATTRIBUTE_NAME)
.getLabel());
return from(ruleContext, toolchainInfo);
}

@Nullable
public static JavaRuntimeInfo from(RuleContext ruleContext, String attributeName) {
if (!ruleContext.attributes().has(attributeName, BuildType.LABEL)) {
if (!ruleContext.attributes().has(attributeName, LABEL)) {
return null;
}
TransitiveInfoCollection prerequisite = ruleContext.getPrerequisite(attributeName);
Expand All @@ -90,6 +96,11 @@ public static JavaRuntimeInfo from(RuleContext ruleContext, String attributeName
}

ToolchainInfo toolchainInfo = prerequisite.get(ToolchainInfo.PROVIDER);
return from(ruleContext, toolchainInfo);
}

@Nullable
private static JavaRuntimeInfo from(RuleContext ruleContext, ToolchainInfo toolchainInfo) {
if (toolchainInfo != null) {
try {
JavaRuntimeInfo result = (JavaRuntimeInfo) toolchainInfo.getValue("java_runtime");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,16 +109,6 @@ static Label javaToolchainAttribute(RuleDefinitionEnvironment environment) {
String DIRECT_SOURCE_JARS_OUTPUT_GROUP =
OutputGroupInfo.HIDDEN_OUTPUT_GROUP_PREFIX + "direct_source_jars";

/** Implementation for the :jvm attribute. */
static Label jvmAttribute(RuleDefinitionEnvironment env) {
return env.getToolsLabel("//tools/jdk:current_java_runtime");
}

/** Implementation for the :host_jdk attribute. */
static Label hostJdkAttribute(RuleDefinitionEnvironment env) {
return env.getToolsLabel("//tools/jdk:current_host_java_runtime");
}

/**
* Implementation for the :java_launcher attribute. Note that the Java launcher is disabled by
* default, so it returns null for the configuration-independent default value.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static com.google.devtools.build.lib.rules.java.JavaStarlarkCommon.checkPrivateAccess;

import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSet;
Expand All @@ -26,7 +27,6 @@
import com.google.devtools.build.lib.analysis.ProviderCollection;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.RuleErrorConsumer;
import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
import com.google.devtools.build.lib.analysis.platform.ToolchainInfo;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.Depset;
Expand Down Expand Up @@ -55,19 +55,25 @@ public final class JavaToolchainProvider extends NativeInfo

/** Returns the Java Toolchain associated with the rule being analyzed or {@code null}. */
public static JavaToolchainProvider from(RuleContext ruleContext) {
TransitiveInfoCollection prerequisite =
ruleContext.getPrerequisite(JavaRuleClasses.JAVA_TOOLCHAIN_ATTRIBUTE_NAME);
return from(prerequisite, ruleContext);
ToolchainInfo toolchainInfo =
ruleContext
.getToolchainContext()
.forToolchainType(
ruleContext
.getPrerequisite(JavaRuleClasses.JAVA_TOOLCHAIN_TYPE_ATTRIBUTE_NAME)
.getLabel());
return from(toolchainInfo, ruleContext);
}

@VisibleForTesting
public static JavaToolchainProvider from(ProviderCollection collection) {
return from(collection, null);
ToolchainInfo toolchainInfo = collection.get(ToolchainInfo.PROVIDER);
return from(toolchainInfo, null);
}

@Nullable
private static JavaToolchainProvider from(
ProviderCollection collection, @Nullable RuleErrorConsumer errorConsumer) {
ToolchainInfo toolchainInfo = collection.get(ToolchainInfo.PROVIDER);
ToolchainInfo toolchainInfo, @Nullable RuleErrorConsumer errorConsumer) {
if (toolchainInfo != null) {
try {
JavaToolchainProvider provider = (JavaToolchainProvider) toolchainInfo.getValue("java");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/toolchain_type_requirement",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/patch_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/split_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/starlark_exposed_rule_transition_factory",
Expand All @@ -48,7 +49,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/rules/apple",
"//src/main/java/com/google/devtools/build/lib/rules/config",
"//src/main/java/com/google/devtools/build/lib/rules/cpp",
"//src/main/java/com/google/devtools/build/lib/rules/java:java-compilation",
"//src/main/java/com/google/devtools/build/lib/rules/proto",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.devtools.build.lib.packages.Attribute.attr;
import static com.google.devtools.build.lib.packages.BuildType.LABEL;
import static com.google.devtools.build.lib.rules.java.JavaRuleClasses.JAVA_TOOLCHAIN_ATTRIBUTE_NAME;
import static java.nio.charset.StandardCharsets.ISO_8859_1;
import static java.util.stream.Collectors.toList;

Expand All @@ -39,7 +38,7 @@
import com.google.devtools.build.lib.analysis.actions.SpawnAction;
import com.google.devtools.build.lib.analysis.config.ConfigAwareAspectBuilder;
import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
import com.google.devtools.build.lib.analysis.platform.ToolchainInfo;
import com.google.devtools.build.lib.analysis.config.ToolchainTypeRequirement;
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;
Expand Down Expand Up @@ -68,6 +67,7 @@
import com.google.devtools.build.lib.rules.java.JavaConfiguration;
import com.google.devtools.build.lib.rules.java.JavaGenJarsProvider;
import com.google.devtools.build.lib.rules.java.JavaInfo;
import com.google.devtools.build.lib.rules.java.JavaRuleClasses;
import com.google.devtools.build.lib.rules.java.JavaRuntimeInfo;
import com.google.devtools.build.lib.rules.java.JavaSemantics;
import com.google.devtools.build.lib.rules.objc.J2ObjcSource.SourceType;
Expand Down Expand Up @@ -118,13 +118,13 @@ private static LabelLateBoundDefault<ProtoConfiguration> getProtoToolchainLabel(
private final RepositoryName toolsRepository;
private final Label ccToolchainType;
private final LabelLateBoundDefault<CppConfiguration> ccToolchain;
private final Label javaToolchain;
private final ToolchainTypeRequirement javaToolchainTypeRequirement;

public J2ObjcAspect(RuleDefinitionEnvironment env, CppSemantics cppSemantics) {
this.toolsRepository = checkNotNull(env.getToolsRepository());
this.ccToolchainType = CppRuleClasses.ccToolchainTypeAttribute(env);
this.ccToolchain = CppRuleClasses.ccToolchainAttribute(env);
this.javaToolchain = JavaSemantics.javaToolchainAttribute(env);
this.javaToolchainTypeRequirement = JavaRuleClasses.javaToolchainTypeRequirement(env);
}

/** Returns whether this aspect allows proto services to be generated from this proto rule */
Expand All @@ -149,7 +149,9 @@ public AspectDefinition getDefinition(AspectParameters aspectParameters) {
J2ObjcConfiguration.class,
ObjcConfiguration.class,
ProtoConfiguration.class)
.addToolchainTypes(CppRuleClasses.ccToolchainTypeRequirement(ccToolchainType))
.addToolchainTypes(
CppRuleClasses.ccToolchainTypeRequirement(ccToolchainType),
javaToolchainTypeRequirement)
.add(
attr("$grep_includes", LABEL)
.cfg(ExecutionTransitionFactory.create())
Expand Down Expand Up @@ -222,10 +224,8 @@ public AspectDefinition getDefinition(AspectParameters aspectParameters) {
toolsRepository + "//tools/j2objc:j2objc_proto_toolchain")))
.add(attr(":j2objc_cc_toolchain", LABEL).value(ccToolchain))
.add(
attr(JAVA_TOOLCHAIN_ATTRIBUTE_NAME, LABEL)
.useOutputLicenses()
.mandatoryProviders(ToolchainInfo.PROVIDER.id())
.value(javaToolchain))
attr(JavaRuleClasses.JAVA_TOOLCHAIN_TYPE_ATTRIBUTE_NAME, LABEL)
.value(javaToolchainTypeRequirement.toolchainType()))
.build();
}

Expand Down
9 changes: 1 addition & 8 deletions src/main/starlark/builtins_bzl/common/java/android_lint.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def android_lint_action(ctx, source_files, source_jars, compilation_info):
if not (source_files or source_jars):
return None

toolchain = ctx.attr._java_toolchain[java_common.JavaToolchainInfo]
toolchain = semantics.find_java_toolchain(ctx)
java_runtime = toolchain.java_runtime
linter = toolchain.android_linter()
if not linter:
Expand Down Expand Up @@ -129,10 +129,3 @@ def android_lint_action(ctx, source_files, source_jars, compilation_info):
execution_requirements = {"supports-workers": "1"},
)
return android_lint_out

ANDROID_LINT_IMPLICIT_ATTRS = {
"_java_toolchain": attr.label(
default = semantics.JAVA_TOOLCHAIN_LABEL,
providers = [java_common.JavaToolchainInfo],
),
}
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ def compile_action(
exported_plugins = exported_plugins,
javac_opts = [ctx.expand_location(opt) for opt in javacopts],
neverlink = neverlink,
java_toolchain = ctx.attr._java_toolchain[java_common.JavaToolchainInfo],
java_toolchain = semantics.find_java_toolchain(ctx),
output = output_class_jar,
output_source_jar = output_source_jar,
strict_deps = _filter_strict_deps(strict_deps),
Expand All @@ -169,10 +169,3 @@ def compile_action(
)

return java_info, compilation_info

COMPILE_ACTION_IMPLICIT_ATTRS = {
"_java_toolchain": attr.label(
default = semantics.JAVA_TOOLCHAIN_LABEL,
providers = [java_common.JavaToolchainInfo],
),
}
8 changes: 6 additions & 2 deletions src/main/starlark/builtins_bzl/common/java/java_common.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Common code for reuse across java_* rules

load(":common/rule_util.bzl", "merge_attrs")
load(":common/java/android_lint.bzl", "android_lint_action")
load(":common/java/compile_action.bzl", "COMPILE_ACTION_IMPLICIT_ATTRS", "compile_action")
load(":common/java/compile_action.bzl", "compile_action")
load(":common/java/java_semantics.bzl", "semantics")
load(":common/java/proguard_validation.bzl", "VALIDATE_PROGUARD_SPECS_IMPLICIT_ATTRS", "validate_proguard_specs")

Expand Down Expand Up @@ -261,12 +261,16 @@ def construct_defaultinfo(ctx, files_to_build, files, neverlink, *extra_attrs):
return default_info

BASIC_JAVA_LIBRARY_IMPLICIT_ATTRS = merge_attrs(
COMPILE_ACTION_IMPLICIT_ATTRS,
{
"_java_plugins": attr.label(
default = semantics.JAVA_PLUGINS_FLAG_ALIAS_LABEL,
providers = [JavaPluginInfo],
),
# TODO(b/245144242): Used by IDE integration, remove when toolchains are used
"_java_toolchain": attr.label(
default = semantics.JAVA_TOOLCHAIN_LABEL,
providers = [java_common.JavaToolchainInfo],
),
},
)

Expand Down
2 changes: 2 additions & 0 deletions src/main/starlark/builtins_bzl/common/java/java_library.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ JAVA_LIBRARY_ATTRS = merge_attrs(
"add_exports": attr.string_list(),
"add_opens": attr.string_list(),
"licenses": attr.license() if hasattr(attr, "license") else attr.string_list(),
"_java_toolchain_type": attr.label(default = semantics.JAVA_TOOLCHAIN_TYPE),
},
)

Expand All @@ -177,4 +178,5 @@ java_library = rule(
},
fragments = ["java", "cpp"],
compile_one_filetype = [".java"],
toolchains = [semantics.JAVA_TOOLCHAIN],
)
2 changes: 2 additions & 0 deletions src/main/starlark/builtins_bzl/common/java/java_plugin.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ Definition of java_plugin rule.
load(":common/java/java_common.bzl", "basic_java_library", "construct_defaultinfo")
load(":common/java/java_library.bzl", "JAVA_LIBRARY_ATTRS", "JAVA_LIBRARY_IMPLICIT_ATTRS")
load(":common/rule_util.bzl", "merge_attrs")
load(":common/java/java_semantics.bzl", "semantics")

JavaPluginInfo = _builtins.toplevel.JavaPluginInfo

Expand Down Expand Up @@ -137,4 +138,5 @@ java_plugin = rule(
"sourcejar": "lib%{name}-src.jar",
},
fragments = ["java", "cpp"],
toolchains = [semantics.JAVA_TOOLCHAIN],
)
12 changes: 12 additions & 0 deletions src/main/starlark/builtins_bzl/common/java/java_semantics.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,20 @@ def _get_coverage_runner(ctx):
def _add_constraints(java_info, constraints):
return java_info

def _find_java_toolchain(ctx):
return ctx.toolchains["@bazel_tools//tools/jdk:toolchain_type"].java

def _find_java_runtime_toolchain(ctx):
return ctx.toolchains["@bazel_tools//tools/jdk:runtime_toolchain_type"].java_runtime

semantics = struct(
JAVA_TOOLCHAIN_LABEL = "@bazel_tools//tools/jdk:current_java_toolchain",
JAVA_TOOLCHAIN_TYPE = "@bazel_tools//tools/jdk:toolchain_type",
JAVA_TOOLCHAIN = _builtins.toplevel.config_common.toolchain_type("@bazel_tools//tools/jdk:toolchain_type", mandatory = True),
find_java_toolchain = _find_java_toolchain,
JAVA_RUNTIME_TOOLCHAIN_TYPE = "@bazel_tools//tools/jdk:runtime_toolchain_type",
JAVA_RUNTIME_TOOLCHAIN = _builtins.toplevel.config_common.toolchain_type("@bazel_tools//tools/jdk:runtime_toolchain_type", mandatory = True),
find_java_runtime_toolchain = _find_java_runtime_toolchain,
JAVA_PLUGINS_FLAG_ALIAS_LABEL = "@bazel_tools//tools/jdk:java_plugins_flag_alias",
PROGUARD_ALLOWLISTER_LABEL = "@bazel_tools//tools/jdk:proguard_whitelister",
EXTRA_SRCS_TYPES = [],
Expand Down
Loading

0 comments on commit c2ccb28

Please sign in to comment.