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

Add new flag --incompatible_disable_native_repo_rules #21913

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 @@ -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 @@ -161,6 +161,7 @@ public class BazelRepositoryModule extends BlazeModule {

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

repositoryCache.setHardlink(repoOptions.useHardlinks);
if (repoOptions.experimentalScaleTimeouts > 0.0) {
Expand Down Expand Up @@ -602,7 +604,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.DISABLE_NATIVE_REPO_RULES, disableNativeRepoRules));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ public RepositoryDirectoryValue.Builder fetch(
Map<RepoRecordedInput, String> recordedInputValues,
SkyKey key)
throws RepositoryFunctionException, InterruptedException {
ensureNativeRepoRuleEnabled(rule, env, "the platform defined at @platforms//host");
StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
if (starlarkSemantics == null) {
return null;
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_disable_native_repo_rules",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS,
effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
help =
"If false, 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 disableNativeRepoRules;

@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> DISABLE_NATIVE_REPO_RULES =
new Precomputed<>("disable_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.DISABLE_NATIVE_REPO_RULES.get(env)) {
return;
}
throw new RepositoryFunctionException(
Starlark.errorf(
"Native repo rule %s is disabled since the flag "
+ "--incompatible_disable_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,9 @@
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.RegistryFunction;
import com.google.devtools.build.lib.bazel.bzlmod.RepoSpecFunction;
import com.google.devtools.build.lib.bazel.bzlmod.YankedVersionsFunction;
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 +54,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 +116,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.DISABLE_NATIVE_REPO_RULES, false),
PrecomputedValue.injected(
BazelModuleResolutionFunction.CHECK_DIRECT_DEPENDENCIES,
RepositoryOptions.CheckDirectDepsMode.OFF),
Expand All @@ -136,15 +142,25 @@ 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 or custom built-in
// modules
if (!this.extraSkyFunctions.containsKey(SkyFunctions.MODULE_FILE)) {
addExtraSkyFunctions(
ImmutableMap.of(
SkyFunctions.MODULE_FILE,
new ModuleFileFunction(
ruleClassProvider.getBazelStarlarkEnvironment(),
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.REGISTRY)) {
addExtraSkyFunctions(
ImmutableMap.of(SkyFunctions.REGISTRY, new RegistryFunction(registryFactory)));
}

addExtraSkyFunctions(
Expand All @@ -171,6 +187,8 @@ public BazelPackageLoader buildImpl() {
EXTERNAL_PACKAGE_HELPER))
.put(SkyFunctions.BAZEL_DEP_GRAPH, new BazelDepGraphFunction())
.put(SkyFunctions.BAZEL_MODULE_RESOLUTION, new BazelModuleResolutionFunction())
.put(SkyFunctions.REPO_SPEC, new RepoSpecFunction())
.put(SkyFunctions.YANKED_VERSIONS, new YankedVersionsFunction())
.buildOrThrow());

return new BazelPackageLoader(this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -828,6 +828,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 @@ -205,6 +205,7 @@ public ImmutableList<PrecomputedValue.Injected> getPrecomputedValues() {
RepositoryDelegatorFunction.FORCE_FETCH,
RepositoryDelegatorFunction.FORCE_FETCH_DISABLED),
PrecomputedValue.injected(RepositoryDelegatorFunction.VENDOR_DIRECTORY, Optional.empty()),
PrecomputedValue.injected(RepositoryDelegatorFunction.DISABLE_NATIVE_REPO_RULES, false),
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 @@ -240,6 +240,8 @@ protected void useRuleClassProvider(ConfiguredRuleClassProvider ruleClassProvide
PrecomputedValue.injected(
ModuleFileFunction.REGISTRIES, ImmutableList.of(registry.getUrl())),
PrecomputedValue.injected(ModuleFileFunction.IGNORE_DEV_DEPS, false),
PrecomputedValue.injected(
RepositoryDelegatorFunction.DISABLE_NATIVE_REPO_RULES, false),
PrecomputedValue.injected(
ModuleFileFunction.MODULE_OVERRIDES, ImmutableMap.of()),
PrecomputedValue.injected(
Expand Down Expand Up @@ -292,6 +294,8 @@ private void reinitializeSkyframeExecutor() {
PrecomputedValue.injected(
ModuleFileFunction.REGISTRIES, ImmutableList.of(registry.getUrl())),
PrecomputedValue.injected(ModuleFileFunction.IGNORE_DEV_DEPS, false),
PrecomputedValue.injected(
RepositoryDelegatorFunction.DISABLE_NATIVE_REPO_RULES, false),
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 @@ -157,6 +157,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 @@ -168,6 +169,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 @@ -177,6 +179,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,7 @@
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.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 +41,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 +62,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 +106,13 @@ public final void initializeSkyframeExecutor() throws Exception {
packageFactory =
loadingMock
.getPackageFactoryBuilderForTesting(directories)
.setExtraSkyFunctions(
ImmutableMap.of(
SkyFunctions.MODULE_FILE,
new ModuleFileFunction(
ruleClassProvider.getBazelStarlarkEnvironment(),
directories.getWorkspace(),
ImmutableMap.of())))
.setPackageValidator(
(pkg, handler) -> {
// Delegate to late-bound this.validator.
Expand Down Expand Up @@ -212,8 +221,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 +269,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
Loading