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

Enable autoloads for rules_android #23836

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
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 @@ -22,12 +22,17 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.bazel.bzlmod.BazelDepGraphValue;
import com.google.devtools.build.lib.bazel.bzlmod.ModuleKey;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.Label.RepoContext;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.skyframe.BzlLoadValue;
import com.google.devtools.build.lib.skyframe.PrecomputedValue.Precomputed;
import com.google.devtools.build.lib.skyframe.RepositoryMappingValue;
import com.google.devtools.build.skyframe.SkyFunction;
import java.util.HashSet;
Expand Down Expand Up @@ -83,6 +88,10 @@ public class AutoloadSymbols {
private final boolean bzlmodEnabled;
private final boolean autoloadsEnabled;

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

public AutoloadSymbols(RuleClassProvider ruleClassProvider, StarlarkSemantics semantics) {
ImmutableList<String> symbolConfiguration =
ImmutableList.copyOf(semantics.get(BuildLanguageOptions.INCOMPATIBLE_AUTOLOAD_EXTERNALLY));
Expand Down Expand Up @@ -378,23 +387,71 @@ private static Map<String, Object> convertNativeStructToMap(StarlarkInfo struct)
@Nullable
public ImmutableMap<String, BzlLoadValue.Key> getLoadKeys(SkyFunction.Environment env)
throws InterruptedException {
RepositoryMappingValue repositoryMappingValue =
(RepositoryMappingValue)
env.getValue(RepositoryMappingValue.key(RepositoryName.BAZEL_TOOLS));

if (repositoryMappingValue == null) {
return null;
}
final RepoContext repoContext;
if (bzlmodEnabled) {
BazelDepGraphValue bazelDepGraphValue =
(BazelDepGraphValue) env.getValue(BazelDepGraphValue.KEY);
if (bazelDepGraphValue == null) {
return null;
}

RepoContext repoContext =
Label.RepoContext.of(
RepositoryName.BAZEL_TOOLS, repositoryMappingValue.getRepositoryMapping());
ImmutableMap<String, ModuleKey> highestVersions =
bazelDepGraphValue.getCanonicalRepoNameLookup().values().stream()
.collect(
toImmutableMap(
ModuleKey::name,
moduleKey -> moduleKey,
(m1, m2) -> m1.version().compareTo(m2.version()) >= 0 ? m1 : m1));
RepositoryMapping repositoryMapping =
RepositoryMapping.create(
highestVersions.entrySet().stream()
.collect(
toImmutableMap(
Map.Entry::getKey,
entry ->
bazelDepGraphValue
.getCanonicalRepoNameLookup()
.inverse()
.get(entry.getValue()))),
RepositoryName.MAIN);
repoContext = Label.RepoContext.of(RepositoryName.MAIN, repositoryMapping);
} else {
RepositoryMappingValue repositoryMappingValue =
(RepositoryMappingValue) env.getValue(RepositoryMappingValue.key(RepositoryName.MAIN));
if (repositoryMappingValue == null) {
return null;
}
// Create with owner, so that we can report missing references (isVisible is false if missing)
repoContext =
Label.RepoContext.of(
RepositoryName.MAIN,
RepositoryMapping.create(
repositoryMappingValue.getRepositoryMapping().entries(), RepositoryName.MAIN));
}

// Inject loads for rules and symbols removed from Bazel
ImmutableMap.Builder<String, BzlLoadValue.Key> loadKeysBuilder =
ImmutableMap.builderWithExpectedSize(autoloadedSymbols.size());
ImmutableSet.Builder<String> missingRepositories = ImmutableSet.builder();
for (String symbol : autoloadedSymbols) {
loadKeysBuilder.put(symbol, AUTOLOAD_CONFIG.get(symbol).getKey(repoContext));
Label label = AUTOLOAD_CONFIG.get(symbol).getLabel(repoContext);
// Only load if the dependency is present
if (label.getRepository().isVisible()) {
loadKeysBuilder.put(symbol, BzlLoadValue.keyForBuild(label));
} else {
missingRepositories.add(label.getRepository().getName());
}
}
for (String missingRepository : missingRepositories.build()) {
env.getListener()
.handle(
Event.warn(
String.format(
"Couldn't auto load rules or symbols, because no dependency on"
+ " module/repository '%s' found. This will result in a failure if"
+ " there's a reference to those rules or symbols.",
missingRepository)));
}
return loadKeysBuilder.buildOrThrow();
}
Expand Down Expand Up @@ -480,10 +537,9 @@ public abstract static class SymbolRedirect {

public abstract ImmutableSet<String> getRdeps();

public BzlLoadValue.Key getKey(RepoContext bazelToolsRepoContext) throws InterruptedException {
Label getLabel(RepoContext repoContext) throws InterruptedException {
try {
return BzlLoadValue.keyForBuild(
Label.parseWithRepoContext(getLoadLabel(), bazelToolsRepoContext));
return Label.parseWithRepoContext(getLoadLabel(), repoContext);
} catch (LabelSyntaxException e) {
throw new IllegalStateException(e);
}
Expand Down
23 changes: 22 additions & 1 deletion src/main/java/com/google/devtools/build/lib/packages/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ java_library(
"ConfiguredAttributeMapper.java",
"LabelPrinter.java",
"PackageSpecification.java",
"AutoloadSymbols.java",
],
),
deps = [
Expand Down Expand Up @@ -87,7 +88,6 @@ 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 Expand Up @@ -121,6 +121,27 @@ java_library(
],
)

java_library(
name = "autoload_symbols",
srcs = ["AutoloadSymbols.java"],
deps = [
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:common",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution",
"//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/packages/semantics",
"//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//src/main/java/net/starlark/java/eval",
"//third_party:auto_value",
"//third_party:guava",
"//third_party:jsr305",
],
)

java_library(
name = "bzl_visibility",
srcs = ["BzlVisibility.java"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class FlagConstants {

private FlagConstants() {}

public static final String DEFAULT_INCOMPATIBLE_AUTOLOAD_EXTERNALLY = "+@rules_python";
public static final String DEFAULT_INCOMPATIBLE_AUTOLOAD_EXTERNALLY = "+@rules_python,+@rules_android";

public static final String DEFAULT_INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX = "true";
public static final String DEFAULT_INCOMPATIBLE_FIX_PACKAGE_GROUP_REPOROOT_SYNTAX = "true";
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/skyframe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception",
"//src/main/java/com/google/devtools/build/lib/io:process_package_directory_exception",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/packages:autoload_symbols",
"//src/main/java/com/google/devtools/build/lib/packages:bzl_visibility",
"//src/main/java/com/google/devtools/build/lib/packages:globber",
"//src/main/java/com/google/devtools/build/lib/packages:globber_utils",
Expand Down Expand Up @@ -796,6 +797,7 @@ java_library(
"//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/packages:autoload_symbols",
"//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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ static BzlCompileValue computeInline(

// 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);
AutoloadSymbols autoloadSymbols = AutoloadSymbols.AUTOLOAD_SYMBOLS.get(env);
if (autoloadSymbols == null) {
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,7 @@ private StarlarkBuiltinsValue getBuiltins(
}
return StarlarkBuiltinsValue.createEmpty(starlarkSemantics);
}
AutoloadSymbols autoloadSymbols = PrecomputedValue.AUTOLOAD_SYMBOLS.get(env);
AutoloadSymbols autoloadSymbols = AutoloadSymbols.AUTOLOAD_SYMBOLS.get(env);
if (autoloadSymbols == null) {
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ 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);
AutoloadSymbols autoloadSymbols = AutoloadSymbols.AUTOLOAD_SYMBOLS.get(env);
if (autoloadSymbols == null) {
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
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 @@ -85,10 +84,6 @@ 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 @@ -1351,7 +1351,7 @@ private void setStarlarkSemantics(StarlarkSemantics starlarkSemantics) {
}

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

public void setBaselineConfiguration(BuildOptions buildOptions) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ private static StarlarkBuiltinsValue computeInternal(
if (starlarkSemantics.get(BuildLanguageOptions.EXPERIMENTAL_BUILTINS_BZL_PATH).isEmpty()) {
return StarlarkBuiltinsValue.createEmpty(starlarkSemantics);
}
AutoloadSymbols autoloadSymbols = PrecomputedValue.AUTOLOAD_SYMBOLS.get(env);
AutoloadSymbols autoloadSymbols = AutoloadSymbols.AUTOLOAD_SYMBOLS.get(env);
if (autoloadSymbols == null) {
return null;
}
Expand Down Expand Up @@ -251,7 +251,6 @@ private static StarlarkBuiltinsValue computeInternal(
}
autoBzlLoadValues = autoBzlLoadValuesBuilder.buildOrThrow();
} catch (BzlLoadFailedException ex) {

throw BuiltinsFailedException.errorEvaluatingAutoloadedBzls(
ex, starlarkSemantics.getBool(BuildLanguageOptions.ENABLE_BZLMOD));
}
Expand Down Expand Up @@ -368,7 +367,7 @@ static BuiltinsFailedException errorEvaluatingAutoloadedBzls(
String additionalMessage =
bzlmodEnabled
? ""
: ". Most likely you need to upgrade the version of rules repository in the"
: " Most likely you need to upgrade the version of rules repository in the"
+ " WORKSPACE file.";
return new BuiltinsFailedException(
String.format(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ public void inject(SkyKey key, Delta delta) {
PrecomputedValue.CONFIG_SETTING_VISIBILITY_POLICY.set(
injectable, ConfigSettingVisibilityPolicy.LEGACY_OFF);
PrecomputedValue.STARLARK_SEMANTICS.set(injectable, starlarkSemantics);
PrecomputedValue.AUTOLOAD_SYMBOLS.set(
AutoloadSymbols.AUTOLOAD_SYMBOLS.set(
injectable, new AutoloadSymbols(ruleClassProvider, starlarkSemantics));
return new ImmutableDiff(ImmutableList.of(), valuesToInject);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/io:file_symlink_cycle_uniqueness_function",
"//src/main/java/com/google/devtools/build/lib/io:file_symlink_infinite_expansion_uniqueness_function",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/packages:autoload_symbols",
"//src/main/java/com/google/devtools/build/lib/pkgcache",
"//src/main/java/com/google/devtools/build/lib/repository:external_package_helper",
"//src/main/java/com/google/devtools/build/lib/skyframe:bzl_compile",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ java_library(
"//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/packages:autoload_symbols",
"//src/main/java/com/google/devtools/build/lib/packages/semantics",
"//src/main/java/com/google/devtools/build/lib/pkgcache",
"//src/main/java/com/google/devtools/build/lib/rules:repository/local_repository_rule",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ public void setup() throws Exception {
.set(BuildLanguageOptions.INCOMPATIBLE_AUTOLOAD_EXTERNALLY, ImmutableList.of())
.build();
PrecomputedValue.STARLARK_SEMANTICS.set(differencer, semantics);
PrecomputedValue.AUTOLOAD_SYMBOLS.set(
AutoloadSymbols.AUTOLOAD_SYMBOLS.set(
differencer, new AutoloadSymbols(ruleClassProvider, semantics));
RepositoryDelegatorFunction.REPOSITORY_OVERRIDES.set(differencer, ImmutableMap.of());
RepositoryDelegatorFunction.FORCE_FETCH.set(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ java_library(
"//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/packages:autoload_symbols",
"//src/main/java/com/google/devtools/build/lib/packages/semantics",
"//src/main/java/com/google/devtools/build/lib/pkgcache",
"//src/main/java/com/google/devtools/build/lib/repository:external_package_helper",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ public void setupDelegator() throws Exception {
StarlarkSemantics semantics =
StarlarkSemantics.builder().setBool(BuildLanguageOptions.ENABLE_BZLMOD, true).build();
PrecomputedValue.STARLARK_SEMANTICS.set(differencer, semantics);
PrecomputedValue.AUTOLOAD_SYMBOLS.set(
AutoloadSymbols.AUTOLOAD_SYMBOLS.set(
differencer, new AutoloadSymbols(ruleClassProvider, semantics));
RepositoryDelegatorFunction.RESOLVED_FILE_INSTEAD_OF_WORKSPACE.set(
differencer, Optional.empty());
Expand Down
34 changes: 30 additions & 4 deletions src/test/shell/integration/load_removed_symbols_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,33 @@ local_path_override(
EOF
}

function disabled_test_removed_rule_loaded() {
function test_missing_necessary_bzlmod_dep() {
# Intentionally not adding rules_android to MODULE.bazel
cat > BUILD << EOF
aar_import(
name = 'aar',
aar = 'aar.file',
deps = [],
)
EOF
bazel build --incompatible_autoload_externally=aar_import :aar >&$TEST_log 2>&1 && fail "build unexpectedly succeeded"
expect_log "name 'aar_import' is not defined"
expect_log "WARNING: Couldn't auto load rules or symbols, because no dependency on module/repository 'rules_android' found. This will result in a failure if there's a reference to those rules or symbols."
}

function test_missing_unnecessary_bzmod_dep() {
# Intentionally not adding rules_android to MODULE.bazel
cat > BUILD << EOF
filegroup(
name = 'filegroup',
srcs = [],
)
EOF
bazel build --incompatible_autoload_externally=aar_import :filegroup >&$TEST_log 2>&1 || fail "build failed"
expect_log "WARNING: Couldn't auto load rules or symbols, because no dependency on module/repository 'rules_android' found. This will result in a failure if there's a reference to those rules or symbols."
}

function test_removed_rule_loaded() {
setup_module_dot_bazel
mock_rules_android

Expand All @@ -94,7 +120,7 @@ EOF
# TODO(b/355260271): add test with workspace enabled
}

function disabled_test_removed_rule_loaded_from_bzl() {
function test_removed_rule_loaded_from_bzl() {
setup_module_dot_bazel
mock_rules_android

Expand Down Expand Up @@ -385,7 +411,7 @@ EOF
expect_log "Duplicated symbol 'py_library' in --incompatible_autoload_externally"
}

function disabled_test_missing_symbol_error() {
function test_missing_symbol_error() {
setup_module_dot_bazel
mock_rules_android
rules_android_workspace="${TEST_TMPDIR}/rules_android_workspace"
Expand All @@ -404,7 +430,7 @@ EOF
expect_log "Failed to apply symbols loaded externally: The toplevel symbol 'aar_import' set by --incompatible_load_symbols_externally couldn't be loaded. 'aar_import' not found in auto loaded '@rules_android//rules:rules.bzl'."
}

function disabled_test_missing_bzlfile_error() {
function test_missing_bzlfile_error() {
setup_module_dot_bazel
mock_rules_android
rules_android_workspace="${TEST_TMPDIR}/rules_android_workspace"
Expand Down