Skip to content

WIP: Add java_current_repository rule #16281

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.

Sign up for GitHub

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

Closed
fmeum wants to merge 1 commit into from
Closed

WIP: Add java_current_repository rule #16281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion .../com/google/devtools/build/lib/analysis/starlark/StarlarkAttributeTransitionProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import java.util.Map;
import java.util.Objects;
import net.starlark.java.eval.Printer;
import net.starlark.java.eval.Starlark;

/**
* This class implements {@link TransitionFactory} to provide a starlark-defined transition that
Expand All @@ -53,7 +54,9 @@ public class StarlarkAttributeTransitionProvider
implements TransitionFactory<AttributeTransitionData>, SplitTransitionProviderApi {
private final StarlarkDefinedConfigTransition starlarkDefinedConfigTransition;

StarlarkAttributeTransitionProvider(
// Only public for use in BazelJava* rules while these rules have not yet been Starlarkified.
// Do not use for any other purpose.
public StarlarkAttributeTransitionProvider(
StarlarkDefinedConfigTransition starlarkDefinedConfigTransition) {
this.starlarkDefinedConfigTransition = starlarkDefinedConfigTransition;
}
Expand Down
4 changes: 4 additions & 0 deletions src/main/java/com/google/devtools/build/lib/bazel/rules/java/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,19 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/analysis:build_info",
"//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:config/starlark_defined_config_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
"//src/main/java/com/google/devtools/build/lib/bazel/rules/cpp",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/packages",
"//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/java:java-rules",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
"//src/main/java/com/google/devtools/build/lib/util:filetype",
"//src/main/java/net/starlark/java/eval",
"//src/main/java/net/starlark/java/syntax",
"//third_party:guava",
"//third_party:jsr305",
],
Expand Down
11 changes: 7 additions & 4 deletions src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaBinaryRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,11 @@

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

import static com.google.devtools.build.lib.packages.Attribute.attr;
import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
import static com.google.devtools.build.lib.packages.Type.BOOLEAN;

import com.google.devtools.build.lib.analysis.BaseRuleClasses;
import com.google.devtools.build.lib.analysis.RuleDefinition;
import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
import com.google.devtools.build.lib.bazel.rules.java.BazelJavaRuleClasses.BaseJavaBinaryRule;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.AttributeMap;
import com.google.devtools.build.lib.packages.RuleClass;
Expand All @@ -30,6 +27,10 @@
import com.google.devtools.build.lib.rules.java.JavaConfiguration;
import com.google.devtools.build.lib.util.FileTypeSet;

import static com.google.devtools.build.lib.packages.Attribute.attr;
import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
import static com.google.devtools.build.lib.packages.Type.BOOLEAN;

/**
* Rule definition for the java_binary rule.
*/
Expand Down Expand Up @@ -101,6 +102,8 @@ public Object getDefault(AttributeMap rule) {
return rule.get("create_executable", BOOLEAN);
}
}))
.addAttribute(BazelJavaCurrentRepositorySupport.CURRENT_REPOSITORY_ATTRIBUTE)
.addAttribute(BazelJavaCurrentRepositorySupport.RUNFILES_CONSTANTS_ATTRIBUTE)
.addToolchainTypes(CppRuleClasses.ccToolchainTypeRequirement(env))
.build();
}
Expand Down
61 changes: 61 additions & 0 deletions ...ava/com/google/devtools/build/lib/bazel/rules/java/BazelJavaCurrentRepositorySupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package com.google.devtools.build.lib.bazel.rules.java;

import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
import com.google.devtools.build.lib.analysis.starlark.StarlarkAttributeTransitionProvider;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.AttributeMap;
import com.google.devtools.build.lib.packages.StructImpl;
import net.starlark.java.eval.*;
import net.starlark.java.syntax.Location;

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.packages.Type.STRING;

public class BazelJavaCurrentRepositorySupport {
private static final String CURRENT_REPOSITORY_SETTING = "@bazel_tools//tools/java/runfiles:current_repository";
private static final Label RUNFILES_CONSTANTS_RULE = Label.parseCanonicalUnchecked("@@bazel_tools//tools/java/runfiles:java_current_repository");

static final Attribute CURRENT_REPOSITORY_ATTRIBUTE = attr("_current_repository", STRING)
.value(new Attribute.ComputedDefault() {
@Override
public String getDefault(AttributeMap rule) {
return rule.getLabel().getRepository().getName();
}
}
)
.build();

static final Attribute RUNFILES_CONSTANTS_ATTRIBUTE = attr("$runfiles_constants", LABEL)
.cfg(new StarlarkAttributeTransitionProvider(makeRunfilesConstantsTransition()))
.value(RUNFILES_CONSTANTS_RULE)
.build();

private static StarlarkDefinedConfigTransition makeRunfilesConstantsTransition() {
try {
return StarlarkDefinedConfigTransition.newRegularTransition(
new StarlarkCallable() {
@Override
public Object fastcall(StarlarkThread thread, Object[] positional, Object[] named) throws EvalException {
String currentRepository = (String) ((StructImpl) positional[1]).getValue("_current_repository");
return Dict.builder().put(CURRENT_REPOSITORY_SETTING, currentRepository).buildImmutable();
}

@Override
public String getName() {
return "_current_repository_transition_impl";
}
},
StarlarkList.empty(),
StarlarkList.immutableOf(CURRENT_REPOSITORY_SETTING),
StarlarkSemantics.DEFAULT,
Label.parseCanonicalUnchecked("@@_builtins//:common/java/java_common.bzl"),
Location.BUILTIN,
RepositoryMapping.ALWAYS_FALLBACK);
} catch (EvalException e) {
throw new RuntimeException(e);
}
}
}
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaTestRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
</p>
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
.add(attr("test_class", STRING))
.addAttribute(BazelJavaCurrentRepositorySupport.CURRENT_REPOSITORY_ATTRIBUTE)
.addAttribute(BazelJavaCurrentRepositorySupport.RUNFILES_CONSTANTS_ATTRIBUTE)
.addToolchainTypes(CppRuleClasses.ccToolchainTypeRequirement(env))
.build();
}
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/com/google/devtools/build/lib/packages/License.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.Locale;
import java.util.Set;
import net.starlark.java.eval.Printer;
import net.starlark.java.eval.StarlarkValue;

/** Support for license and distribution checking. */
@Immutable
Expand All @@ -53,7 +54,7 @@ public static class LicenseParsingException extends Exception {
* <p>Note that the order is important for the purposes of finding the least
* restrictive license.
*/
public enum LicenseType {
public enum LicenseType implements StarlarkValue {
Copy link
Copy Markdown
Contributor

@comius comius Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this and other StarlarkValue needed?

Copy link
Copy Markdown
Collaborator Author

@fmeum fmeum Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This as well as the conversion of singleton sets are necessary to allow attaching a Starlark transition to java_binary. That triggers logic that converts all attributes to Starlark values and these legacy types as well as the Java standard library class for singleton sets failed during conversion. Do you see a better way to handle this?

BY_EXCEPTION_ONLY,
RESTRICTED,
RESTRICTED_IF_STATICALLY_LINKED,
Expand Down Expand Up @@ -95,7 +96,7 @@ public static LicenseType leastRestrictive(Collection<LicenseType> types) {
/**
* The types of distribution that are supported.
*/
public enum DistributionType {
public enum DistributionType implements StarlarkValue {
INTERNAL,
WEB,
CLIENT,
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/com/google/devtools/build/lib/packages/TriState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@

package com.google.devtools.build.lib.packages;

import net.starlark.java.eval.StarlarkValue;

/**
* Enum used to represent tri-state parameters in rule attributes (yes/no/auto).
*/
public enum TriState {
public enum TriState implements StarlarkValue {
YES,
NO,
AUTO;
Expand Down
10 changes: 10 additions & 0 deletions src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,15 @@ public static List<String> getJvmFlags(RuleContext ruleContext) {
return jvmFlags;
}

private static List<TransitiveInfoCollection> getRunfilesConstants(RuleContext ruleContext) {
// This attribute is only available on Bazel Java rules.
if (ruleContext.attributes().has("$runfiles_constants", BuildType.LABEL)) {
return ImmutableList.copyOf(ruleContext.getPrerequisites("$runfiles_constants"));
} else {
return ImmutableList.of();
}
}

private static List<TransitiveInfoCollection> getRuntimeDeps(RuleContext ruleContext) {
// We need to check here because there are classes inheriting from this class that implement
// rules that don't have this attribute.
Expand Down Expand Up @@ -616,6 +625,7 @@ private static ImmutableList<TransitiveInfoCollection> collectTargetsTreatedAsDe
builder.addAll(getRuntimeDeps(ruleContext));
builder.addAll(getExports(ruleContext));
}
builder.addAll(getRunfilesConstants(ruleContext));
builder.addAll(ruleContext.getPrerequisites("deps"));

semantics.collectTargetsTreatedAsDeps(ruleContext, builder, type);
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/net/starlark/java/eval/Starlark.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,8 @@ public static Object fromJava(Object x, @Nullable Mutability mutability) {
return StarlarkList.copyOf(mutability, (List<?>) x);
} else if (x instanceof Map) {
return Dict.copyOf(mutability, (Map<?, ?>) x);
} else if (x instanceof Set && ((Set<?>) x).size() == 1) {
Copy link
Copy Markdown
Contributor

@comius comius Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? We are usually more thorough on reviews of Starlark changes. If you need it, perhaps it's better to submit it in a separate PR.

return StarlarkList.copyOf(mutability, (Set<?>) x);
}
throw new InvalidStarlarkValueException(x.getClass());
}
Expand Down
38 changes: 35 additions & 3 deletions src/main/starlark/builtins_bzl/common/java/java_library.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,17 @@ load(":common/java/java_semantics.bzl", "semantics")
JavaInfo = _builtins.toplevel.JavaInfo
JavaPluginInfo = _builtins.toplevel.JavaPluginInfo
CcInfo = _builtins.toplevel.CcInfo
transition = _builtins.toplevel.transition
repository_name = _builtins.native.repository_name

def _current_repository_transition_impl(settings, attr):
return {"@bazel_tools//tools/java/runfiles:current_repository": attr.current_repository}

_current_repository_transition = transition(
implementation = _current_repository_transition_impl,
inputs = [],
outputs = ["@bazel_tools//tools/java/runfiles:current_repository"],
)

def bazel_java_library_rule(
ctx,
Expand All @@ -37,7 +48,8 @@ def bazel_java_library_rule(
neverlink = False,
proguard_specs = [],
add_exports = [],
add_opens = []):
add_opens = [],
runfiles_constants = None):
"""Implements java_library.

Use this call when you need to produce a fully fledged java_library from
Expand All @@ -58,6 +70,7 @@ def bazel_java_library_rule(
proguard_specs: (list[File]) Files to be used as Proguard specification.
add_exports: (list[str]) Allow this library to access the given <module>/<package>.
add_opens: (list[str]) Allow this library to reflectively access the given <module>/<package>.
runfiles_constants: (Target) Target providing RunfilesConstants.CURRENT_REPOSITORY.
Returns:
(list[provider]) A list containing DefaultInfo, JavaInfo,
InstrumentedFilesInfo, OutputGroupsInfo, ProguardSpecProvider providers.
Expand All @@ -68,7 +81,7 @@ def bazel_java_library_rule(
target, base_info = basic_java_library(
ctx,
srcs,
deps,
deps + [runfiles_constants],
runtime_deps,
plugins,
exports,
Expand Down Expand Up @@ -110,6 +123,8 @@ def _proxy(ctx):
ctx.files.proguard_specs,
ctx.attr.add_exports,
ctx.attr.add_opens,
# This attribute is subject to an outgoing transition.
ctx.attr._runfiles_constants[0],
).values()

JAVA_LIBRARY_IMPLICIT_ATTRS = BASIC_JAVA_LIBRARY_WITH_PROGUARD_IMPLICIT_ATTRS
Expand Down Expand Up @@ -164,11 +179,21 @@ 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(),
# Consumed by _current_repository_transition.
"current_repository": attr.string(),
"_runfiles_constants": attr.label(
default = "@bazel_tools//tools/java/runfiles:java_current_repository",
providers = [JavaInfo],
cfg = _current_repository_transition,
),
"_java_toolchain_type": attr.label(default = semantics.JAVA_TOOLCHAIN_TYPE),
"_allowlist_function_transition": attr.label(
default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
),
Comment on lines +182 to +192
Copy link
Copy Markdown
Contributor

@comius comius Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would also be pulled into Google java_library. Monorepo probably doesn't need it and there might be an observable memory regression involved (1 new public attribute will use more memory for each target).

Keep JAVA_LIBRARY_ATTRS intact and do a BAZEL_JAVA_LIBRARY_ATTRS which merges former and the new attributes into _java_library rule.

},
)

java_library = rule(
_java_library = rule(
Copy link
Copy Markdown
Contributor

@comius comius Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't change the name here, tooling uses it. Solution is to put java_library macro into a new file.

_proxy,
attrs = JAVA_LIBRARY_ATTRS,
provides = [JavaInfo],
Expand All @@ -180,3 +205,10 @@ java_library = rule(
compile_one_filetype = [".java"],
toolchains = [semantics.JAVA_TOOLCHAIN],
)

def java_library(**kwargs):
_java_library(
# Skip over the leading '@'.
current_repository = repository_name()[1:],
Copy link
Copy Markdown
Contributor

@comius comius Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

repository_name()[1:] creates a new string and will probably cause a memory regression, repository_name() is probably ok. If possible process it, that is remove @ in an action.

Copy link
Copy Markdown
Collaborator Author

@fmeum fmeum Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can definitely change that, but don't we get the regression either way? repository_name is backed by getNameWithAt, so the call will return a new string each time. The returned value isn't retained if we strip off the @.

**kwargs
)
20 changes: 20 additions & 0 deletions src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,26 @@ public void setupMockClient(MockToolsConfig config, List<String> workspaceConten
")",
"java_plugins_flag_alias(name = 'java_plugins_flag_alias')");

config.create("embedded_tools/tools/allowlists/function_transition_allowlist/BUILD",
"package_group(name='function_transition_allowlist',packages=['//...'])");
config.create("embedded_tools/tools/java/runfiles/java_current_repository.bzl",
"_CurrentRepositoryInfo = provider()",
"def _current_repository_impl(ctx):",
" return _CurrentRepositoryInfo(value = ctx.build_setting_value)",
"current_repository_setting = rule(",
" implementation = _current_repository_impl,",
" build_setting = config.string(),",
")",
"def _java_current_repository_impl(ctx):",
" jar_file = ctx.actions.declare_file('empty.jar')",
" ctx.actions.write(jar_file, '')",
" return [JavaInfo(output_jar = jar_file, compile_jar = jar_file)]",
"java_current_repository = rule(implementation = _java_current_repository_impl)");
config.create("embedded_tools/tools/java/runfiles/BUILD",
"load(':java_current_repository.bzl','current_repository_setting','java_current_repository')",
"java_current_repository(name='java_current_repository')",
"current_repository_setting(name='current_repository',build_setting_default='')");

config.create(
TestConstants.CONSTRAINTS_PATH + "/android/BUILD",
"package(default_visibility=['//visibility:public'])",
Expand Down
Loading