Skip to content

Commit

Permalink
Bzlmod: Refactor repo rule creation code
Browse files Browse the repository at this point in the history 
(#13316)

By recognizing that the only difference between creating a native and starlark repo rule is the code to acquire the RuleClass object, we can remove a lot of duplicate code around repo rule creation.

* BzlmodRepoRuleCreator the interface is simply replaced with the existing interface RuleFunction, which exposes the underlying RuleClass.
* Moved repo rule creation code into BzlmodRepoRuleCreator, which is now a utility class. This logic can then be shared by ModuleExtensionEvalStarlarkThreadContext and BzlmodRepoRuleFunction, instead of existing in 3 different places.

This refactor helps us eventually support rules like `native.local_repository` in module extensions.

PiperOrigin-RevId: 424627127
  • Loading branch information
@Wyverald @copybara-github
Wyverald authored and copybara-github committed Jan 27, 2022
1 parent 85821a2 commit 90abed5
Show file tree
Hide file tree
Showing 5 changed files with 128 additions and 173 deletions.
6 changes: 5 additions & 1 deletion src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
"//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/vfs",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//src/main/java/net/starlark/java/eval",
"//src/main/java/net/starlark/java/syntax",
Expand Down Expand Up @@ -206,8 +205,13 @@ java_library(
name = "repo_rule_creator",
srcs = ["BzlmodRepoRuleCreator.java"],
deps = [
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//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/vfs",
"//src/main/java/net/starlark/java/eval",
"//src/main/java/net/starlark/java/syntax",
"//third_party:guava",
],
)
64 changes: 55 additions & 9 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,23 +14,69 @@

package com.google.devtools.build.lib.bazel.bzlmod;

import com.google.devtools.build.lib.events.EventHandler;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.cmdline.LabelConstants;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.Package.NameConflictException;
import com.google.devtools.build.lib.packages.PackageFactory;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleFactory;
import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap;
import com.google.devtools.build.lib.packages.RuleFactory.InvalidRuleException;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
import java.util.Map;
import net.starlark.java.eval.StarlarkSemantics;
import net.starlark.java.eval.StarlarkThread.CallStackEntry;
import net.starlark.java.syntax.Location;

/**
* An interface for {@link RepositoryRuleFunction} to create a repository rule instance with given
* parameters.
* Creates a repo rule instance for Bzlmod. This class contrasts with the WORKSPACE repo rule
* creation mechanism in that it creates an "external" package that contains only 1 rule.
*/
public interface BzlmodRepoRuleCreator {
Rule createAndAddRule(
Package.Builder packageBuilder,
public final class BzlmodRepoRuleCreator {
private BzlmodRepoRuleCreator() {}

/** Creates a repo rule instance from the given parameters. */
public static Rule createRule(
PackageFactory packageFactory,
BlazeDirectories directories,
StarlarkSemantics semantics,
Map<String, Object> kwargs,
EventHandler handler)
throws InterruptedException, InvalidRuleException, NameConflictException;
ExtendedEventHandler eventHandler,
String callStackEntry,
RuleClass ruleClass,
Map<String, Object> attributes)
throws InterruptedException, InvalidRuleException, NoSuchPackageException {
// TODO(bazel-team): Don't use the {@link Rule} class for repository rule.
// Currently, the repository rule is represented with the {@link Rule} class that's designed
// for build rules. Therefore, we have to create a package instance for it, which doesn't make
// sense. We should migrate away from this implementation so that we don't refer to any build
// rule specific things in repository rule.
Package.Builder packageBuilder =
packageFactory.newExternalPackageBuilder(
RootedPath.toRootedPath(
Root.fromPath(directories.getWorkspace()),
LabelConstants.MODULE_DOT_BAZEL_FILE_NAME),
"dummy_name",
semantics);
BuildLangTypedAttributeValuesMap attributeValues =
new BuildLangTypedAttributeValuesMap(attributes);
ImmutableList<CallStackEntry> callStack =
ImmutableList.of(new CallStackEntry(callStackEntry, Location.BUILTIN));
Rule rule;
try {
rule =
RuleFactory.createAndAddRule(
packageBuilder, ruleClass, attributeValues, eventHandler, semantics, callStack);
} catch (NameConflictException e) {
// This literally cannot happen -- we just created the package!
throw new IllegalStateException(e);
}
packageBuilder.build();
return rule;
}
}
55 changes: 25 additions & 30 deletions .../com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,19 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.cmdline.LabelConstants;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.Package.NameConflictException;
import com.google.devtools.build.lib.packages.PackageFactory;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleFactory.InvalidRuleException;
import com.google.devtools.build.lib.packages.StarlarkNativeModule.ExistingRulesShouldBeNoOp;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
import java.util.HashMap;
import java.util.Map;
import net.starlark.java.eval.Dict;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Starlark;
import net.starlark.java.eval.StarlarkSemantics;
import net.starlark.java.eval.StarlarkThread;
import net.starlark.java.syntax.Location;

Expand Down Expand Up @@ -86,38 +84,35 @@ public String getRepoPrefix() {
return repoPrefix;
}

public void createRepo(
String internalRepoName,
BzlmodRepoRuleCreator repoRuleCreator,
Map<String, Object> kwargs,
StarlarkSemantics starlarkSemantics,
Location location)
public void createRepo(StarlarkThread thread, Dict<String, Object> kwargs, RuleClass ruleClass)
throws InterruptedException, EvalException {
PackageAndLocation conflict = generatedRepos.get(internalRepoName);
Object nameValue = kwargs.getOrDefault("name", Starlark.NONE);
if (!(nameValue instanceof String)) {
throw Starlark.errorf(
"expected string for attribute 'name', got '%s'", Starlark.type(nameValue));
}
String name = (String) nameValue;
PackageAndLocation conflict = generatedRepos.get(name);
if (conflict != null) {
throw Starlark.errorf(
"A repo named %s is already generated by this module extension at %s",
internalRepoName, conflict.getLocation());
}

Package.Builder packageBuilder =
packageFactory.newExternalPackageBuilder(
RootedPath.toRootedPath(
Root.fromPath(directories.getWorkspace()),
LabelConstants.MODULE_DOT_BAZEL_FILE_NAME),
"dummy_name",
starlarkSemantics);
try {
repoRuleCreator.createAndAddRule(packageBuilder, starlarkSemantics, kwargs, eventHandler);
} catch (InvalidRuleException | NameConflictException e) {
throw Starlark.errorf("%s", e.getMessage());
name, conflict.getLocation());
}
String prefixedName = repoPrefix + name;
try {
Rule rule =
BzlmodRepoRuleCreator.createRule(
packageFactory,
directories,
thread.getSemantics(),
eventHandler,
"RepositoryRuleFunction.createRule",
ruleClass,
Maps.transformEntries(kwargs, (k, v) -> k.equals("name") ? prefixedName : v));
generatedRepos.put(
internalRepoName, PackageAndLocation.create(packageBuilder.build(), location));
} catch (NoSuchPackageException e) {
// This will never happen!
throw new IllegalStateException(e);
name, PackageAndLocation.create(rule.getPackage(), thread.getCallerLocation()));
} catch (InvalidRuleException | NoSuchPackageException e) {
throw Starlark.errorf("%s", e.getMessage());
}
}

Expand Down
59 changes: 15 additions & 44 deletions ...ava/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,10 @@

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
import com.google.devtools.build.docgen.annot.DocCategory;
import com.google.devtools.build.docgen.annot.DocumentMethods;
import com.google.devtools.build.lib.analysis.BaseRuleClasses;
import com.google.devtools.build.lib.analysis.starlark.StarlarkAttrModule.Descriptor;
import com.google.devtools.build.lib.bazel.bzlmod.BzlmodRepoRuleCreator;
import com.google.devtools.build.lib.bazel.bzlmod.ModuleExtension;
import com.google.devtools.build.lib.bazel.bzlmod.ModuleExtensionEvalStarlarkThreadContext;
import com.google.devtools.build.lib.bazel.bzlmod.TagClass;
Expand All @@ -44,9 +42,8 @@
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
import com.google.devtools.build.lib.packages.RuleFactory;
import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap;
import com.google.devtools.build.lib.packages.RuleFactory.InvalidRuleException;
import com.google.devtools.build.lib.packages.RuleFunction;
import com.google.devtools.build.lib.packages.StarlarkExportable;
import com.google.devtools.build.lib.packages.WorkspaceFactoryHelper;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
Expand All @@ -62,11 +59,8 @@
import net.starlark.java.eval.Sequence;
import net.starlark.java.eval.Starlark;
import net.starlark.java.eval.StarlarkCallable;
import net.starlark.java.eval.StarlarkSemantics;
import net.starlark.java.eval.StarlarkThread;
import net.starlark.java.eval.StarlarkThread.CallStackEntry;
import net.starlark.java.eval.Tuple;
import net.starlark.java.syntax.Location;

/**
* The Starlark module containing the definition of {@code repository_rule} function to define a
Expand Down Expand Up @@ -136,7 +130,7 @@ public StarlarkCallable repositoryRule(
+ " the implementation function of a module extension to instantiate and return a"
+ " repository rule.")
private static final class RepositoryRuleFunction
implements StarlarkCallable, StarlarkExportable, BzlmodRepoRuleCreator {
implements StarlarkCallable, StarlarkExportable, RuleFunction {
private final RuleClass.Builder builder;
private final StarlarkCallable implementation;
private Label extensionLabel;
Expand Down Expand Up @@ -193,49 +187,37 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
if (!isExported()) {
throw new EvalException("attempting to instantiate a non-exported repository rule");
}
Object nameValue = kwargs.getOrDefault("name", Starlark.NONE);
if (!(nameValue instanceof String)) {
throw Starlark.errorf(
"expected string for attribute 'name', got '%s'", Starlark.type(nameValue));
}
String name = (String) nameValue;
String prefixedName = extensionEvalContext.getRepoPrefix() + name;
extensionEvalContext.createRepo(
name,
this,
Maps.transformEntries(kwargs, (k, v) -> k.equals("name") ? prefixedName : v),
thread.getSemantics(),
thread.getCallerLocation());
extensionEvalContext.createRepo(thread, kwargs, getRuleClass());
return Starlark.NONE;
}

private Object createRuleLegacy(StarlarkThread thread, Dict<String, Object> kwargs)
throws EvalException, InterruptedException {
BazelStarlarkContext.from(thread).checkWorkspacePhase("repository rule " + exportedName);
String ruleClassName;
private String getRuleClassName() {
// If the function ever got exported (the common case), we take the name
// it was exported to. Only in the not intended case of calling an unexported
// repository function through an exported macro, we fall back, for lack of
// alternatives, to the name in the local context.
// TODO(b/111199163): we probably should disallow the use of non-exported
// repository rules anyway.
if (isExported()) {
ruleClassName = exportedName;
return exportedName;
} else {
// repository_rules should be subject to the same "exported" requirement
// as package rules, but sadly we forgot to add the necessary check and
// now many projects create and instantiate repository_rules without an
// intervening export; see b/111199163. An incompatible flag is required.
if (false) {
throw new EvalException("attempt to instantiate a non-exported repository rule");
}

// The historical workaround was a fragile hack to introspect on the call
// expression syntax, f() or x.f(), to find the name f, but we no longer
// have access to the call expression, so now we just create an ugly
// name from the function. See github.com/bazelbuild/bazel/issues/10441
ruleClassName = "unexported_" + implementation.getName();
return "unexported_" + implementation.getName();
}
}

private Object createRuleLegacy(StarlarkThread thread, Dict<String, Object> kwargs)
throws EvalException, InterruptedException {
BazelStarlarkContext.from(thread).checkWorkspacePhase("repository rule " + exportedName);
String ruleClassName = getRuleClassName();
try {
RuleClass ruleClass = builder.build(ruleClassName, ruleClassName);
PackageContext context = PackageFactory.getContext(thread);
Expand All @@ -260,20 +242,9 @@ private Object createRuleLegacy(StarlarkThread thread, Dict<String, Object> kwar
}

@Override
public Rule createAndAddRule(
Package.Builder packageBuilder,
StarlarkSemantics semantics,
Map<String, Object> kwargs,
EventHandler handler)
throws InterruptedException, InvalidRuleException, NameConflictException {
RuleClass ruleClass = builder.build(exportedName, exportedName);
BuildLangTypedAttributeValuesMap attributeValues =
new BuildLangTypedAttributeValuesMap(kwargs);
ImmutableList.Builder<CallStackEntry> callStack = ImmutableList.builder();
// TODO(pcloudy): Optimize the callstack
callStack.add(new CallStackEntry("RepositoryRuleFunction.createRule", Location.BUILTIN));
return RuleFactory.createAndAddRule(
packageBuilder, ruleClass, attributeValues, handler, semantics, callStack.build());
public RuleClass getRuleClass() {
String name = getRuleClassName();
return builder.build(name, name);
}
}

Expand Down
Loading

0 comments on commit 90abed5

Please sign in to comment.