Skip to content

Commit

Permalink
Add new flag --enable_native_repo_rules
Browse files Browse the repository at this point in the history
This new flag defaults to `true`. When set to `false`, native repo rules cannot be used in WORKSPACE; their Starlark counterparts must be used. Native repo rules already can't be used in Bzlmod.

Work towards #18285.
  • Loading branch information
Wyverald committed Apr 9, 2024
1 parent 89e602a commit af1766a
Show file tree
Hide file tree
Showing 35 changed files with 193 additions and 78 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,13 @@ public Builder addWorkspaceFilePrefix(String contents) {
return this;
}

@CanIgnoreReturnValue
@VisibleForTesting
public Builder clearWorkspaceFilePrefixForTesting() {
defaultWorkspaceFilePrefix.delete(0, defaultWorkspaceFilePrefix.length());
return this;
}

@CanIgnoreReturnValue
public Builder addWorkspaceFileSuffix(String contents) {
defaultWorkspaceFileSuffix.append(contents);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ public class BazelRepositoryModule extends BlazeModule {

private Optional<Path> vendorDirectory;
private List<String> allowedYankedVersions = ImmutableList.of();
private boolean enableNativeRepoRules;
private SingleExtensionEvalFunction singleExtensionEvalFunction;
private final ExecutorService repoFetchingWorkerThreadPool =
Executors.newFixedThreadPool(
Expand Down Expand Up @@ -343,8 +344,9 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
if (repoOptions.repositoryDownloaderRetries >= 0) {
downloadManager.setRetries(repoOptions.repositoryDownloaderRetries);
}
enableNativeRepoRules = repoOptions.enableNativeRepoRules;

repositoryCache.setHardlink(repoOptions.useHardlinks);
repositoryCache.setHardlink(repoOptions.useHardlinks);
if (repoOptions.experimentalScaleTimeouts > 0.0) {
starlarkRepositoryFunction.setTimeoutScaling(repoOptions.experimentalScaleTimeouts);
singleExtensionEvalFunction.setTimeoutScaling(repoOptions.experimentalScaleTimeouts);
Expand Down Expand Up @@ -594,7 +596,9 @@ public ImmutableList<Injected> getPrecomputedValues() {
PrecomputedValue.injected(RepositoryDelegatorFunction.IS_VENDOR_COMMAND, false),
PrecomputedValue.injected(RepositoryDelegatorFunction.VENDOR_DIRECTORY, vendorDirectory),
PrecomputedValue.injected(
YankedVersionsUtil.ALLOWED_YANKED_VERSIONS, allowedYankedVersions));
YankedVersionsUtil.ALLOWED_YANKED_VERSIONS, allowedYankedVersions),
PrecomputedValue.injected(
RepositoryDelegatorFunction.ENABLE_NATIVE_REPO_RULES, enableNativeRepoRules));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ public RepositoryDirectoryValue.Builder fetch(
Environment env,
Map<RepoRecordedInput, String> recordedInputValues,
SkyKey key)
throws RepositoryFunctionException {
throws RepositoryFunctionException, InterruptedException {
ensureNativeRepoRuleEnabled(rule, env, "the platform defined at @platforms//host");
String name = rule.getName();
try {
outputDirectory.createDirectoryAndParents();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,18 @@ public class RepositoryOptions extends OptionsBase {
+ " still run an arbitrary executable that accesses the Internet.")
public boolean disableDownload;

@Option(
name = "incompatible_enable_native_repo_rules",
defaultValue = "true",
documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS,
effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
help =
"If true, native repo rules can be used in WORKSPACE; otherwise, Starlark repo rules "
+ "must be used instead. Native repo rules include local_repository, "
+ "new_local_repository, local_config_platform, android_sdk_repository, and "
+ "android_ndk_repository.")
public boolean enableNativeRepoRules;

@Option(
name = "experimental_repository_downloader_retries",
defaultValue = "0",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
load("@bazel_tools//tools/build_defs/repo:local.bzl", "local_repository")
load("@bazel_tools//tools/build_defs/repo:utils.bzl", "maybe")

maybe(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ public RepositoryDirectoryValue.Builder fetch(
Map<RepoRecordedInput, String> recordedInputValues,
SkyKey key)
throws InterruptedException, RepositoryFunctionException {
ensureNativeRepoRuleEnabled(rule, env, "https://github.com/bazelbuild/rules_android_ndk");
Map<String, String> environ =
declareEnvironmentDependencies(recordedInputValues, env, PATH_ENV_VAR_AS_SET);
if (environ == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ public RepositoryDirectoryValue.Builder fetch(
Map<RepoRecordedInput, String> recordedInputValues,
SkyKey key)
throws RepositoryFunctionException, InterruptedException {
ensureNativeRepoRuleEnabled(rule, env, "https://github.com/bazelbuild/rules_android");
Map<String, String> environ =
declareEnvironmentDependencies(recordedInputValues, env, PATH_ENV_VAR_AS_SET);
if (environ == null) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
local_repository(
load("@bazel_tools//tools/build_defs/repo:local.bzl", rules_java_builtin_local_repository = "local_repository")
rules_java_builtin_local_repository(
name = "rules_java_builtin",
path = __embedded_dir__ + "/rules_java",
repo_mapping = {"@rules_java" : "@rules_java_builtin"}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ public RepositoryDirectoryValue.Builder fetch(
Map<RepoRecordedInput, String> recordedInputValues,
SkyKey key)
throws InterruptedException, RepositoryFunctionException {
ensureNativeRepoRuleEnabled(
rule, env, "load(\"@bazel_tools//tools/build_defs/repo:local.bzl\", \"local_repository\")");
// DO NOT MODIFY THIS! It's being deprecated in favor of Starlark counterparts.
// See https://github.com/bazelbuild/bazel/issues/18285
String userDefinedPath = RepositoryFunction.getPathAttr(rule);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ public RepositoryDirectoryValue.Builder fetch(
Map<RepoRecordedInput, String> recordedInputValues,
SkyKey key)
throws InterruptedException, RepositoryFunctionException {
ensureNativeRepoRuleEnabled(
rule,
env,
"load(\"@bazel_tools//tools/build_defs/repo:local.bzl\", \"new_local_repository\")");
// DO NOT MODIFY THIS! It's being deprecated in favor of Starlark counterparts.
// See https://github.com/bazelbuild/bazel/issues/18285

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ public final class RepositoryDelegatorFunction implements SkyFunction {
public static final Precomputed<Optional<Path>> VENDOR_DIRECTORY =
new Precomputed<>("vendor_directory");

public static final Precomputed<Boolean> ENABLE_NATIVE_REPO_RULES =
new Precomputed<>("enable_native_repo_rules");

// The marker file version is inject in the rule key digest so the rule key is always different
// when we decide to update the format.
private static final int MARKER_FILE_VERSION = 7;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,27 @@ public abstract RepositoryDirectoryValue.Builder fetch(
SkyKey key)
throws InterruptedException, RepositoryFunctionException;

protected static void ensureNativeRepoRuleEnabled(Rule rule, Environment env, String replacement)
throws RepositoryFunctionException, InterruptedException {
if (!isWorkspaceRepo(rule)) {
// If this native repo rule is used in a Bzlmod context, always allow it. This is because
// we're still using the native `local_repository` for `local_path_override`, and it's
// nontrivial to migrate that one to the Starlark version.
return;
}
if (RepositoryDelegatorFunction.ENABLE_NATIVE_REPO_RULES.get(env)) {
return;
}
throw new RepositoryFunctionException(
Starlark.errorf(
"Native repo rule %s is disabled since the flag "
+ "--noincompatible_enable_native_repo_rules is set. Native repo rules are "
+ "deprecated; please migrate to their Starlark counterparts. For %s, please use "
+ "%s.",
rule.getRuleClass(), rule.getRuleClass(), replacement),
Transience.PERSISTENT);
}

/**
* Verify the data provided by the marker file to check if a refetch is needed. Returns true if
* the data is up to date and no refetch is needed and false if the data is obsolete and a refetch
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileFunction;
import com.google.devtools.build.lib.bazel.bzlmod.RegistryFactory;
import com.google.devtools.build.lib.bazel.bzlmod.RegistryFactoryImpl;
import com.google.devtools.build.lib.bazel.bzlmod.RepoSpecFunction;
import com.google.devtools.build.lib.bazel.bzlmod.YankedVersionsUtil;
import com.google.devtools.build.lib.bazel.repository.RepositoryOptions;
import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache;
Expand Down Expand Up @@ -51,6 +52,7 @@
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;
Expand Down Expand Up @@ -112,8 +114,10 @@ private Builder(Root workspaceDir, Path installBase, Path outputBase, AtomicBool
RepositoryDelegatorFunction.FORCE_FETCH_CONFIGURE,
RepositoryDelegatorFunction.FORCE_FETCH_DISABLED),
PrecomputedValue.injected(RepositoryDelegatorFunction.VENDOR_DIRECTORY, Optional.empty()),
PrecomputedValue.injected(ModuleFileFunction.REGISTRIES, ImmutableList.of()),
PrecomputedValue.injected(
ModuleFileFunction.REGISTRIES, BazelRepositoryModule.DEFAULT_REGISTRIES),
PrecomputedValue.injected(ModuleFileFunction.IGNORE_DEV_DEPS, false),
PrecomputedValue.injected(RepositoryDelegatorFunction.ENABLE_NATIVE_REPO_RULES, true),
PrecomputedValue.injected(
BazelModuleResolutionFunction.CHECK_DIRECT_DEPENDENCIES,
RepositoryOptions.CheckDirectDepsMode.OFF),
Expand All @@ -136,7 +140,7 @@ public BazelPackageLoader buildImpl() {
new RegistryFactoryImpl(
directories.getWorkspace(), downloadManager, Suppliers.ofInstance(ImmutableMap.of()));

// Allow tests to override SkyFunctions.MODULE_FILE to use fake registry
// Allow tests to override the following functions to use fake registry
if (!this.extraSkyFunctions.containsKey(SkyFunctions.MODULE_FILE)) {
addExtraSkyFunctions(
ImmutableMap.of(
Expand All @@ -145,7 +149,16 @@ public BazelPackageLoader buildImpl() {
ruleClassProvider.getBazelStarlarkEnvironment(),
registryFactory,
directories.getWorkspace(),
ImmutableMap.of())));
ModuleFileFunction.getBuiltinModules(directories.getEmbeddedBinariesRoot())
.entrySet()
.stream()
.filter(e -> e.getKey().equals("bazel_tools"))
.collect(
ImmutableMap.toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)))));
}
if (!this.extraSkyFunctions.containsKey(SkyFunctions.REPO_SPEC)) {
addExtraSkyFunctions(
ImmutableMap.of(SkyFunctions.REPO_SPEC, new RepoSpecFunction(registryFactory)));
}

addExtraSkyFunctions(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -886,6 +886,15 @@ def http_file(**kwargs):
def http_jar(**kwargs):
pass
""");
config.create(
"embedded_tools/tools/build_defs/repo/local.bzl",
"""
def local_repository(**kwargs):
pass
def new_local_repository(**kwargs):
pass
""");
config.create("embedded_tools/tools/jdk/jdk_build_file.bzl", "JDK_BUILD_TEMPLATE = ''");
config.create(
"embedded_tools/tools/jdk/local_java_repository.bzl",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ public ImmutableList<PrecomputedValue.Injected> getPrecomputedValues() {
RepositoryDelegatorFunction.FORCE_FETCH,
RepositoryDelegatorFunction.FORCE_FETCH_DISABLED),
PrecomputedValue.injected(RepositoryDelegatorFunction.VENDOR_DIRECTORY, Optional.empty()),
PrecomputedValue.injected(RepositoryDelegatorFunction.ENABLE_NATIVE_REPO_RULES, true),
PrecomputedValue.injected(ModuleFileFunction.REGISTRIES, ImmutableList.of()),
PrecomputedValue.injected(ModuleFileFunction.IGNORE_DEV_DEPS, false),
PrecomputedValue.injected(ModuleFileFunction.MODULE_OVERRIDES, ImmutableMap.of()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,8 @@ protected void useRuleClassProvider(ConfiguredRuleClassProvider ruleClassProvide
PrecomputedValue.injected(
ModuleFileFunction.REGISTRIES, ImmutableList.of(registry.getUrl())),
PrecomputedValue.injected(ModuleFileFunction.IGNORE_DEV_DEPS, false),
PrecomputedValue.injected(
RepositoryDelegatorFunction.ENABLE_NATIVE_REPO_RULES, true),
PrecomputedValue.injected(
ModuleFileFunction.MODULE_OVERRIDES, ImmutableMap.of()),
PrecomputedValue.injected(
Expand Down Expand Up @@ -289,6 +291,8 @@ private void reinitializeSkyframeExecutor() {
PrecomputedValue.injected(
ModuleFileFunction.REGISTRIES, ImmutableList.of(registry.getUrl())),
PrecomputedValue.injected(ModuleFileFunction.IGNORE_DEV_DEPS, false),
PrecomputedValue.injected(
RepositoryDelegatorFunction.ENABLE_NATIVE_REPO_RULES, true),
PrecomputedValue.injected(
BazelModuleResolutionFunction.CHECK_DIRECT_DEPENDENCIES,
CheckDirectDepsMode.WARNING),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ public void setup() throws Exception {
ConfiguredRuleClassProvider.Builder builder = new ConfiguredRuleClassProvider.Builder();
TestRuleClassProvider.addStandardRules(builder);
builder
.clearWorkspaceFilePrefixForTesting()
.clearWorkspaceFileSuffixForTesting()
.addStarlarkBootstrap(new RepositoryBootstrap(new StarlarkRepositoryModule()));
ConfiguredRuleClassProvider ruleClassProvider = builder.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,6 @@ public void testBadRepoName() throws Exception {
context().write(WORKSPACE, "local_repository(name = '@a', path = 'abc')");
context().write("BUILD");
ProcessResult result = context().bazel().shouldFail().build("//...");
assertThat(result.errString())
.contains("Error in local_repository: invalid repository name '@a'");
assertThat(result.errString()).contains("invalid repository name '@a'");
}
}
3 changes: 3 additions & 0 deletions src/test/java/com/google/devtools/build/lib/packages/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
"//src/main/java/com/google/devtools/build/lib/analysis:server_directories",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution_impl",
"//src/main/java/com/google/devtools/build/lib/bazel/rules/python",
"//src/main/java/com/google/devtools/build/lib/clock",
"//src/main/java/com/google/devtools/build/lib/cmdline",
Expand All @@ -167,6 +168,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/rules/proto",
"//src/main/java/com/google/devtools/build/lib/rules/python",
"//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_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/packages:PackageFactoryBuilderWithSkyframeForTesting",
"//src/main/java/com/google/devtools/build/lib/util",
Expand All @@ -176,6 +178,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/com/google/devtools/common/options",
"//src/test/java/com/google/devtools/build/lib/bazel/bzlmod:util",
"//src/test/java/com/google/devtools/build/lib/rules/python:PythonTestUtils",
"//src/test/java/com/google/devtools/build/lib/testutil",
"//src/test/java/com/google/devtools/build/lib/testutil:SkyframeExecutorTestHelper",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
import com.google.devtools.build.lib.analysis.RuleDefinition;
import com.google.devtools.build.lib.analysis.ServerDirectories;
import com.google.devtools.build.lib.bazel.bzlmod.FakeRegistry;
import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileFunction;
import com.google.devtools.build.lib.clock.BlazeClock;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
Expand All @@ -40,6 +42,7 @@
import com.google.devtools.build.lib.runtime.QuiescingExecutorsImpl;
import com.google.devtools.build.lib.skyframe.BazelSkyframeExecutorConstants;
import com.google.devtools.build.lib.skyframe.PrecomputedValue;
import com.google.devtools.build.lib.skyframe.SkyFunctions;
import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
import com.google.devtools.build.lib.testutil.FoundationTestCase;
import com.google.devtools.build.lib.testutil.SkyframeExecutorTestHelper;
Expand All @@ -60,8 +63,8 @@
import org.junit.Before;

/**
* This is a specialization of {@link FoundationTestCase} that's useful for
* implementing tests of the "packages" library.
* This is a specialization of {@link FoundationTestCase} that's useful for implementing tests of
* the "packages" library.
*/
public abstract class PackageLoadingTestCase extends FoundationTestCase {

Expand Down Expand Up @@ -104,6 +107,14 @@ public final void initializeSkyframeExecutor() throws Exception {
packageFactory =
loadingMock
.getPackageFactoryBuilderForTesting(directories)
.setExtraSkyFunctions(
ImmutableMap.of(
SkyFunctions.MODULE_FILE,
new ModuleFileFunction(
ruleClassProvider.getBazelStarlarkEnvironment(),
FakeRegistry.DEFAULT_FACTORY,
directories.getWorkspace(),
ImmutableMap.of())))
.setPackageValidator(
(pkg, handler) -> {
// Delegate to late-bound this.validator.
Expand Down Expand Up @@ -212,8 +223,10 @@ protected void setBuildLanguageOptions(String... options) throws Exception {
}

protected Target getTarget(String label)
throws NoSuchPackageException, NoSuchTargetException,
LabelSyntaxException, InterruptedException {
throws NoSuchPackageException,
NoSuchTargetException,
LabelSyntaxException,
InterruptedException {
return getTarget(Label.parseCanonical(label));
}

Expand Down Expand Up @@ -258,8 +271,9 @@ protected static String genRule(String rule, String name, String... body) {
}

/**
* A utility function which generates the "deps" clause for a build file
* rule from a list of targets.
* A utility function which generates the "deps" clause for a build file rule from a list of
* targets.
*
* @param depTargets the list of targets.
* @return a string containing the deps clause
*/
Expand Down
Loading

0 comments on commit af1766a

Please sign in to comment.