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 5, 2024
1 parent 89e602a commit 834ac67
Show file tree
Hide file tree
Showing 28 changed files with 129 additions and 64 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 = "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, "the version in @bazel_tools//tools/build_defs/repo:local.bzl");
// 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,8 @@ public RepositoryDirectoryValue.Builder fetch(
Map<RepoRecordedInput, String> recordedInputValues,
SkyKey key)
throws InterruptedException, RepositoryFunctionException {
ensureNativeRepoRuleEnabled(
rule, env, "the version in @bazel_tools//tools/build_defs/repo:local.bzl");
// 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 @@ -26,6 +26,7 @@
import com.google.devtools.build.lib.cmdline.LabelConstants;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.Type;
Expand Down Expand Up @@ -187,6 +188,26 @@ 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 --noenable_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'");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,7 @@ protected SkyframeExecutor createSkyframeExecutor(ConfiguredRuleClassProvider ru
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 @@ -156,6 +156,7 @@ public void setupDelegator() 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 @@ -47,59 +47,6 @@
@RunWith(JUnit4.class)
public class RepositoryFunctionTest extends BuildViewTestCase {

/**
* Exposes RepositoryFunction's protected methods to this class.
*/
@VisibleForTesting
static class TestingRepositoryFunction extends RepositoryFunction {
@Nullable
@Override
public RepositoryDirectoryValue.Builder fetch(
Rule rule,
Path outputDirectory,
BlazeDirectories directories,
SkyFunction.Environment env,
Map<RepoRecordedInput, String> recordedInputValues,
SkyKey key)
throws InterruptedException {
return null;
}

@Override
protected boolean isLocal(Rule rule) {
return false;
}

@Override
public Class<? extends RuleDefinition> getRuleDefinition() {
return null;
}
}

@Test
public void testGetTargetPathRelative() throws Exception {
Rule rule = scratchRule("external", "z", "local_repository(",
" name = 'z',",
" path = 'a/b/c',",
")");
assertThat(
TestingRepositoryFunction.getTargetPath(
TestingRepositoryFunction.getPathAttr(rule), rootDirectory))
.isEqualTo(rootDirectory.getRelative("a/b/c").asFragment());
}

@Test
public void testGetTargetPathAbsolute() throws Exception {
Rule rule = scratchRule("external", "w", "local_repository(",
" name = 'w',",
" path = '/a/b/c',",
")");
assertThat(
TestingRepositoryFunction.getTargetPath(
TestingRepositoryFunction.getPathAttr(rule), rootDirectory))
.isEqualTo(PathFragment.create("/a/b/c"));
}

private static void assertMarkerFileEscaping(String testCase) {
String escaped = RepositoryDelegatorFunction.escape(testCase);
assertThat(RepositoryDelegatorFunction.unescape(escaped)).isEqualTo(testCase);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) {
RepositoryDelegatorFunction.REPOSITORY_OVERRIDES.set(differencer, ImmutableMap.of());
RepositoryDelegatorFunction.FORCE_FETCH.set(
differencer, RepositoryDelegatorFunction.FORCE_FETCH_DISABLED);
RepositoryDelegatorFunction.ENABLE_NATIVE_REPO_RULES.set(differencer, true);
RepositoryDelegatorFunction.VENDOR_DIRECTORY.set(differencer, Optional.empty());

RepositoryDelegatorFunction.RESOLVED_FILE_INSTEAD_OF_WORKSPACE.set(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) {
RepositoryDelegatorFunction.REPOSITORY_OVERRIDES.set(differencer, ImmutableMap.of());
RepositoryDelegatorFunction.FORCE_FETCH.set(
differencer, RepositoryDelegatorFunction.FORCE_FETCH_DISABLED);
RepositoryDelegatorFunction.ENABLE_NATIVE_REPO_RULES.set(differencer, true);
RepositoryDelegatorFunction.VENDOR_DIRECTORY.set(differencer, Optional.empty());

RepositoryDelegatorFunction.RESOLVED_FILE_INSTEAD_OF_WORKSPACE.set(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ private static void mockEmbeddedTools(Path embeddedBinaries) throws IOException
tools.getRelative("tools/cpp").createDirectoryAndParents();
tools.getRelative("tools/osx").createDirectoryAndParents();
FileSystemUtils.writeIsoLatin1(tools.getRelative("WORKSPACE"), "");
FileSystemUtils.writeIsoLatin1(tools.getRelative("MODULE.bazel"),
"module(name='bazel_tools')");
FileSystemUtils.writeIsoLatin1(tools.getRelative("tools/cpp/BUILD"), "");
FileSystemUtils.writeIsoLatin1(
tools.getRelative("tools/cpp/cc_configure.bzl"),
Expand All @@ -102,6 +104,13 @@ private static void mockEmbeddedTools(Path embeddedBinaries) throws IOException
"",
"def http_jar(**kwargs):",
" pass");
FileSystemUtils.writeIsoLatin1(
tools.getRelative("tools/build_defs/repo/local.bzl"),
"def local_repository(**kwargs):",
" pass",
"",
"def new_local_repository(**kwargs):",
" pass");
FileSystemUtils.writeIsoLatin1(
tools.getRelative("tools/build_defs/repo/utils.bzl"),
"def maybe(repo_rule, name, **kwargs):",
Expand Down
Loading

0 comments on commit 834ac67

Please sign in to comment.