Skip to content

Commit

Permalink
Implement incompatible_autoload_externally
Browse files Browse the repository at this point in the history
Issue: #22928
Incompatible flag issue: #23043

The flag supports Bazel users in migrating the rules from being embedded in Bazel to external repositories. Listing a symbol or a rule in the flag automatically adds a load to the respective repository. Listing it with a '+' keeps the symbol available in the rules_repository. Listing a symbol with "-" forcefully removes it from Bazel.

Cycles are prevented with a list of repositories where autoloads should not be used.

The new environments are injected into BzlCompile, BzlLoad and Package function.

StarlarkBuiltinsFunction with autoloads true key is used to load the external symbols.

Closes #23016.

Downstream test: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/4003

PiperOrigin-RevId: 666710259
Change-Id: Idaa9b6b13c9a474f700e69e22b6162ed59b7fab0
  • Loading branch information
comius authored and copybara-github committed Aug 23, 2024
1 parent 82f029c commit f9ed0b0
Show file tree
Hide file tree
Showing 18 changed files with 1,413 additions and 34 deletions.
13 changes: 9 additions & 4 deletions src/MODULE.tools
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,11 @@

module(name = "bazel_tools")

bazel_dep(name = "rules_cc", version = "0.0.9")
bazel_dep(name = "rules_java", version = "7.9.0")
bazel_dep(name = "rules_license", version = "0.0.3")
bazel_dep(name = "rules_proto", version = "4.0.0")
bazel_dep(name = "rules_python", version = "0.22.1")

bazel_dep(name = "buildozer", version = "7.1.2")
bazel_dep(name = "platforms", version = "0.0.9")
bazel_dep(name = "protobuf", version = "3.19.6", repo_name = "com_google_protobuf")
bazel_dep(name = "zlib", version = "1.3.1.bcr.3")

cc_configure = use_extension("//tools/cpp:cc_configure.bzl", "cc_configure_extension")
Expand Down Expand Up @@ -49,3 +45,12 @@ use_repo(android_sdk_proxy_extensions, "android_external")
# Used by bazel mod tidy (see BazelModTidyFunction).
buildozer_binary = use_extension("@buildozer//:buildozer_binary.bzl", "buildozer_binary")
use_repo(buildozer_binary, "buildozer_binary")

# Dependencies used to auto-load removed symbols and rules from Bazel (due to Starlarkification)
# See also: --incompatible_autoload_externally, AutoloadSymbols
bazel_dep(name = "protobuf", version = "3.19.6", repo_name = "com_google_protobuf")
bazel_dep(name = "rules_java", version = "7.9.0")
bazel_dep(name = "rules_cc", version = "0.0.9")
bazel_dep(name = "rules_python", version = "0.22.1")
# add rules_android
# add apple_support

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/packages/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:detailed_exceptions",
"//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:starlark_builtins_value",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization:abstract-exported-starlark-symbol-codec",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.devtools.build.lib.concurrent.BlazeInterners;
import com.google.devtools.common.options.Converters.CommaSeparatedNonEmptyOptionListConverter;
import com.google.devtools.common.options.Converters.CommaSeparatedOptionListConverter;
import com.google.devtools.common.options.Converters.CommaSeparatedOptionSetConverter;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionDocumentationCategory;
import com.google.devtools.common.options.OptionEffectTag;
Expand Down Expand Up @@ -97,6 +98,58 @@ public final class BuildLanguageOptions extends OptionsBase {
+ "the builtins injection mechanism entirely.")
public String experimentalBuiltinsBzlPath;

@Option(
name = "incompatible_autoload_externally",
converter = CommaSeparatedOptionSetConverter.class,
defaultValue = "",
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
effectTags = {OptionEffectTag.LOSES_INCREMENTAL_STATE, OptionEffectTag.BUILD_FILE_SEMANTICS},
metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
help =
"A comma-separated list of rules (or other symbols) that were previously part of Bazel"
+ " and which are now to be retrieved from their respective external repositories."
+ " This flag is intended to be used to facilitate migration of rules out of Bazel."
+ " See also https://github.com/bazelbuild/bazel/issues/23043.\n"
+ "A symbol that is autoloaded within a file behaves as if its built-into-Bazel"
+ " definition were replaced by its canonical new definition in an external"
+ " repository. For a BUILD file, this essentially means implicitly adding a load()"
+ " statement. For a .bzl file, it's either a load() statement or a change to a field"
+ " of the `native` object, depending on whether the autoloaded symbol is a rule.\n"
+ "Bazel maintains a hardcoded list of all symbols that may be autoloaded; only those"
+ " symbols may appear in this flag. For each symbol, Bazel knows the new definition"
+ " location in an external repository, as well as a set of special-cased"
+ " repositories that must not autoload it to avoid creating cycles.\n"
+ "A list item of \"+foo\" in this flag causes symbol foo to be autoloaded, except in"
+ " foo's exempt repositories, within which the Bazel-defined version of foo is still"
+ " available.\n"
+ "A list item of \"foo\" triggers autoloading as above, but the Bazel-defined"
+ " version of foo is not made available to the excluded repositories. This ensures"
+ " that foo's external repository does not depend on the old Bazel implementation of"
+ " foo\n"
+ "A list item of \"-foo\" does not trigger any autoloading, but makes the"
+ " Bazel-defined version of foo inaccessible throughout the workspace. This is used"
+ " to validate that the workspace is ready for foo's definition to be deleted from"
+ " Bazel.\n"
+ "If a symbol is not named in this flag then it continues to work as normal -- no"
+ " autoloading is done, nor is the Bazel-defined version suppressed. For"
+ " configuration see"
+ " https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java")
public List<String> incompatibleAutoloadExternally;

@Option(
name = "repositories_without_autoloads",
converter = CommaSeparatedOptionSetConverter.class,
defaultValue = "",
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
effectTags = {OptionEffectTag.LOSES_INCREMENTAL_STATE, OptionEffectTag.BUILD_FILE_SEMANTICS},
metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
help =
"A list of additional repositories (beyond the hardcoded ones Bazel knows about) where "
+ "autoloads are not to be added. This should typically contain repositories that are"
+ " transitively depended on by a repository that may be loaded automatically "
+ "(and which can therefore potentially create a cycle).")
public List<String> repositoriesWithoutAutoloads;

@Option(
name = "experimental_builtins_dummy",
defaultValue = "false",
Expand Down Expand Up @@ -737,6 +790,8 @@ public StarlarkSemantics toStarlarkSemantics() {
incompatibleStopExportingLanguageModules)
.setBool(INCOMPATIBLE_ALLOW_TAGS_PROPAGATION, experimentalAllowTagsPropagation)
.set(EXPERIMENTAL_BUILTINS_BZL_PATH, experimentalBuiltinsBzlPath)
.set(INCOMPATIBLE_AUTOLOAD_EXTERNALLY, incompatibleAutoloadExternally)
.set(REPOSITORIES_WITHOUT_AUTOLOAD, repositoriesWithoutAutoloads)
.setBool(EXPERIMENTAL_BUILTINS_DUMMY, experimentalBuiltinsDummy)
.set(EXPERIMENTAL_BUILTINS_INJECTION_OVERRIDE, experimentalBuiltinsInjectionOverride)
.setBool(EXPERIMENTAL_BZL_VISIBILITY, experimentalBzlVisibility)
Expand Down Expand Up @@ -927,6 +982,10 @@ public StarlarkSemantics toStarlarkSemantics() {
// non-booleans
public static final StarlarkSemantics.Key<String> EXPERIMENTAL_BUILTINS_BZL_PATH =
new StarlarkSemantics.Key<>("experimental_builtins_bzl_path", "%bundled%");
public static final StarlarkSemantics.Key<List<String>> INCOMPATIBLE_AUTOLOAD_EXTERNALLY =
new StarlarkSemantics.Key<>("incompatible_autoload_externally", ImmutableList.of());
public static final StarlarkSemantics.Key<List<String>> REPOSITORIES_WITHOUT_AUTOLOAD =
new StarlarkSemantics.Key<>("repositories_without_autoloads", ImmutableList.of());
public static final StarlarkSemantics.Key<List<String>> EXPERIMENTAL_BUILTINS_INJECTION_OVERRIDE =
new StarlarkSemantics.Key<>("experimental_builtins_injection_override", ImmutableList.of());
public static final StarlarkSemantics.Key<Long> MAX_COMPUTATION_STEPS =
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/skyframe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -2621,6 +2621,7 @@ java_library(
":bzl_load_value",
":repository_mapping_value",
":sky_functions",
":starlark_builtins_value",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:module_extension",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:repo_rule_value",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.devtools.build.lib.cmdline.BazelCompileContext;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.packages.AutoloadSymbols;
import com.google.devtools.build.lib.packages.BazelStarlarkEnvironment;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
Expand Down Expand Up @@ -157,7 +158,14 @@ static BzlCompileValue computeInline(
// For WORKSPACE-loaded bzl files, the env isn't quite right not because of injection but
// because the "native" object is different. But A) that will be fixed with #11954, and B) we
// don't care for the same reason as above.
predeclared = bazelStarlarkEnvironment.getUninjectedBuildBzlEnv();

// Takes into account --incompatible_autoload_externally, similarly to the comment above, this
// only defines the correct set of symbols, but does not load them yet.
AutoloadSymbols autoloadSymbols = PrecomputedValue.AUTOLOAD_SYMBOLS.get(env);
if (autoloadSymbols == null) {
return null;
}
predeclared = autoloadSymbols.getUninjectedBuildBzlEnv(key.getLabel());
}

// We have all deps. Parse, resolve, and return.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import com.google.devtools.build.lib.events.EventKind;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.io.InconsistentFilesystemException;
import com.google.devtools.build.lib.packages.AutoloadSymbols;
import com.google.devtools.build.lib.packages.BazelStarlarkEnvironment;
import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
import com.google.devtools.build.lib.packages.BzlInitThreadContext;
Expand Down Expand Up @@ -609,13 +610,19 @@ private StarlarkBuiltinsValue getBuiltins(
}
return StarlarkBuiltinsValue.createEmpty(starlarkSemantics);
}
AutoloadSymbols autoloadSymbols = PrecomputedValue.AUTOLOAD_SYMBOLS.get(env);
if (autoloadSymbols == null) {
return null;
}
try {
boolean withAutoloads = requiresAutoloads(key, autoloadSymbols);
if (inliningState == null) {
return (StarlarkBuiltinsValue)
env.getValueOrThrow(StarlarkBuiltinsValue.key(), BuiltinsFailedException.class);
env.getValueOrThrow(
StarlarkBuiltinsValue.key(withAutoloads), BuiltinsFailedException.class);
} else {
return StarlarkBuiltinsFunction.computeInline(
StarlarkBuiltinsValue.key(),
StarlarkBuiltinsValue.key(withAutoloads),
inliningState,
ruleClassProvider.getBazelStarlarkEnvironment(),
/* bzlLoadFunction= */ this);
Expand All @@ -635,6 +642,23 @@ private static boolean requiresBuiltinsInjection(BzlLoadValue.Key key) {
&& !(key instanceof BzlLoadValue.KeyForBzlmodBootstrap));
}

private static boolean requiresAutoloads(BzlLoadValue.Key key, AutoloadSymbols autoloadSymbols) {
// We do autoloads for all BUILD files and BUILD-loaded .bzl files, except for files in
// certain rule repos (see AutoloadSymbols#reposDisallowingAutoloads).
//
// We don't do autoloads for the WORKSPACE file, Bzlmod files, or .bzls loaded by them,
// because in general the rules repositories that we would load are not yet available.
//
// We never do autoloads for builtins bzls.
//
// We don't do autoloads for the prelude file, but that's a single file so users can migrate it
// easily. (We do autoloads in .bzl files that are loaded by the prelude file.)
return autoloadSymbols.isEnabled()
&& key instanceof BzlLoadValue.KeyForBuild
&& !key.isBuildPrelude()
&& !autoloadSymbols.autoloadsDisabledForRepo(key.getLabel());
}

/**
* Given a bzl key, validates that the corresponding package exists (if required) and returns the
* associated compile key based on the package's root. Returns null for a missing Skyframe dep or
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,12 +186,25 @@ public boolean maybeReportCycle(
} else if (Iterables.any(cycle, IS_BZL_LOAD)) {
Label fileLabel =
((BzlLoadValue.Key) Iterables.getLast(Iterables.filter(cycle, IS_BZL_LOAD))).getLabel();
eventHandler.handle(
Event.error(
null,
String.format(
"Failed to load .bzl file '%s': possible dependency cycle detected.\n",
fileLabel)));
final String errorMessage;
if (cycle.get(0).equals(StarlarkBuiltinsValue.key(true))) {
// We know `fileLabel` is the last .bzl visited in the cycle. We also know that
// BzlLoadFunction triggered the cycle by requesting StarlarkBuiltinsValue w/autoloads.
// We know that we're not in builtins .bzls, because they don't request w/autoloads.
// Thus, `fileLabel` is a .bzl transitively depended on by an autoload.
errorMessage =
String.format(
"Cycle caused by autoloads, failed to load .bzl file '%s'.\n"
+ "Add '%s' to --repositories_without_autoloads or disable autoloads by setting"
+ " '--incompatible_autoload_externally='\n"
+ "More information on https://github.com/bazelbuild/bazel/issues/23043.\n",
fileLabel, fileLabel.getRepository().getName());
} else {
errorMessage =
String.format(
"Failed to load .bzl file '%s': possible dependency cycle detected.\n", fileLabel);
}
eventHandler.handle(Event.error(null, errorMessage));
return true;
} else if (Iterables.any(cycle, IS_PACKAGE_LOOKUP)) {
PackageIdentifier pkg =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.io.FileSymlinkException;
import com.google.devtools.build.lib.io.InconsistentFilesystemException;
import com.google.devtools.build.lib.packages.AutoloadSymbols;
import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
import com.google.devtools.build.lib.packages.CachingPackageLocator;
Expand Down Expand Up @@ -457,14 +458,21 @@ public SkyValue compute(SkyKey key, Environment env)

StarlarkBuiltinsValue starlarkBuiltinsValue;
try {
// Bazel: we do autoloads for all BUILD files if enabled
AutoloadSymbols autoloadSymbols = PrecomputedValue.AUTOLOAD_SYMBOLS.get(env);
if (autoloadSymbols == null) {
return null;
}
if (bzlLoadFunctionForInlining == null) {
starlarkBuiltinsValue =
(StarlarkBuiltinsValue)
env.getValueOrThrow(StarlarkBuiltinsValue.key(), BuiltinsFailedException.class);
env.getValueOrThrow(
StarlarkBuiltinsValue.key(/* withAutoloads= */ autoloadSymbols.isEnabled()),
BuiltinsFailedException.class);
} else {
starlarkBuiltinsValue =
StarlarkBuiltinsFunction.computeInline(
StarlarkBuiltinsValue.key(),
StarlarkBuiltinsValue.key(/* withAutoloads= */ autoloadSymbols.isEnabled()),
BzlLoadFunction.InliningState.create(env),
packageFactory.getRuleClassProvider().getBazelStarlarkEnvironment(),
bzlLoadFunctionForInlining);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.google.common.base.Supplier;
import com.google.common.base.Suppliers;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.packages.AutoloadSymbols;
import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy;
import com.google.devtools.build.lib.packages.RuleVisibility;
import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
Expand Down Expand Up @@ -84,6 +85,10 @@ public static <T> Injected injected(Precomputed<T> precomputed, T value) {
public static final Precomputed<StarlarkSemantics> STARLARK_SEMANTICS =
new Precomputed<>("starlark_semantics");

// Configuration of --incompatible_load_externally
public static final Precomputed<AutoloadSymbols> AUTOLOAD_SYMBOLS =
new Precomputed<>("autoload_symbols");

public static final Precomputed<UUID> BUILD_ID =
new Precomputed<>("build_id", /* shareable= */ false);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@
import com.google.devtools.build.lib.events.Reporter;
import com.google.devtools.build.lib.io.FileSymlinkCycleUniquenessFunction;
import com.google.devtools.build.lib.io.FileSymlinkInfiniteExpansionUniquenessFunction;
import com.google.devtools.build.lib.packages.AutoloadSymbols;
import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.BuildFileName;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
Expand Down Expand Up @@ -1334,6 +1335,10 @@ private void setStarlarkSemantics(StarlarkSemantics starlarkSemantics) {
PrecomputedValue.STARLARK_SEMANTICS.set(injectable(), starlarkSemantics);
}

private void setAutoloadsConfiguration(AutoloadSymbols autoloadSymbols) {
PrecomputedValue.AUTOLOAD_SYMBOLS.set(injectable(), autoloadSymbols);
}

public void setBaselineConfiguration(BuildOptions buildOptions) {
PrecomputedValue.BASELINE_CONFIGURATION.set(injectable(), buildOptions);
}
Expand Down Expand Up @@ -1476,6 +1481,7 @@ public void preparePackageLoading(

StarlarkSemantics starlarkSemantics = getEffectiveStarlarkSemantics(buildLanguageOptions);
setStarlarkSemantics(starlarkSemantics);
setAutoloadsConfiguration(new AutoloadSymbols(ruleClassProvider, starlarkSemantics));
setSiblingDirectoryLayout(
starlarkSemantics.getBool(BuildLanguageOptions.EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT));
setPackageLocator(pkgLocator);
Expand Down
Loading

0 comments on commit f9ed0b0

Please sign in to comment.