diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index dfddded97ea569..25b2cf7d6c0934 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -1214,9 +1214,9 @@ java_library( name = "project_function", srcs = ["ProjectFunction.java"], deps = [ + ":bzl_load_failed_exception", + ":bzl_load_value", ":project_value", - "//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_failed_exception", - "//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_value", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/main/java/net/starlark/java/eval", "//third_party:guava", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ProjectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ProjectFunction.java index a3a65e62a01914..74e4fc6d716c4e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ProjectFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ProjectFunction.java @@ -33,9 +33,15 @@ public class ProjectFunction implements SkyFunction { /** The top level reserved globals in the PROJECT.scl file. */ private enum ReservedGlobals { - OWNED_CODE_PATHS("owned_code_paths"), + /** Deprecated PROJECT.scl structure. See {@link #PROJECT}. */ + ACTIVE_DIRECTORIES("active_directories"), - ACTIVE_DIRECTORIES("active_directories"); + /** + * Forward-facing PROJECT.scl structure: a single top-level "project" variable that contains all + * project data in nested data structures. + */ + // TODO: b/345100818 - delete deprecated form when no depot instances use it. + PROJECT("project"); private final String key; @@ -67,8 +73,39 @@ public SkyValue compute(SkyKey skyKey, Environment env) return null; } + Object projectRaw = bzlLoadValue.getModule().getGlobal(ReservedGlobals.PROJECT.getKey()); + ImmutableMap project = + switch (projectRaw) { + case null -> ImmutableMap.of(); + case Dict dict -> { + ImmutableMap.Builder projectBuilder = ImmutableMap.builder(); + for (Object k : dict.keySet()) { + if (!(k instanceof String stringKey)) { + throw new ProjectFunctionException( + new TypecheckFailureException( + String.format( + "%s variable: expected string key, got element of %s", + ReservedGlobals.PROJECT.getKey(), k.getClass()))); + } + projectBuilder.put(stringKey, dict.get(stringKey)); + } + yield projectBuilder.buildOrThrow(); + } + default -> + throw new ProjectFunctionException( + new TypecheckFailureException( + String.format( + "%s variable: expected a map of string to objects, got %s", + ReservedGlobals.PROJECT.getKey(), projectRaw.getClass()))); + }; + Object activeDirectoriesRaw = bzlLoadValue.getModule().getGlobal(ReservedGlobals.ACTIVE_DIRECTORIES.getKey()); + if (activeDirectoriesRaw == null && project != null) { + // TODO: b/345100818 - make this the only way to retrieve "active_directoreis" when all + // instances of the old form are gone. + activeDirectoriesRaw = project.get(ReservedGlobals.ACTIVE_DIRECTORIES.getKey()); + } // Crude typechecking to prevent server crashes. // TODO: all of these typechecking should probably be handled by a proto spec. @@ -115,6 +152,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) + activeDirectoriesRaw.getClass())); }; + // TODO: b/345100818 - remove residual global support when everything is under "project". ImmutableMap residualGlobals = bzlLoadValue.getModule().getGlobals().entrySet().stream() .filter( @@ -129,7 +167,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) "non-empty active_directories must contain the 'default' key")); } - return new ProjectValue(activeDirectories, residualGlobals); + return new ProjectValue(project, activeDirectories, residualGlobals); } private static final class TypecheckFailureException extends Exception { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ProjectValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ProjectValue.java index 7442cb9e15f642..99914cfc94c2de 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ProjectValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ProjectValue.java @@ -26,14 +26,17 @@ /** A SkyValue representing the parsed definitions from a PROJECT.scl file. */ public final class ProjectValue implements SkyValue { + private final ImmutableMap project; private final ImmutableMap> activeDirectories; private final ImmutableMap residualGlobals; public ProjectValue( + ImmutableMap project, ImmutableMap> activeDirectories, ImmutableMap residualGlobals) { + this.project = project; this.activeDirectories = activeDirectories; this.residualGlobals = residualGlobals; } @@ -68,6 +71,16 @@ public ImmutableSet getDefaultActiveDirectory() { return ImmutableSet.copyOf(activeDirectories.get("default")); } + /** + * Returns the top-level project definition. Entries are currently self-typed: it's up to the + * entry's consumer to validate and correctly read it. + * + *

If {@code "project"} is not defined in the file, returns an empty map. + */ + public ImmutableMap getProject() { + return project; + } + /** * Returns the map of named active directories in the project. If the map is not defined in the * file, returns an empty map. diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/config/FlagSetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/config/FlagSetFunction.java index 6aa2ff96f2512e..86f97ccc38e528 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/config/FlagSetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/config/FlagSetFunction.java @@ -21,7 +21,6 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.analysis.config.BuildOptions; -import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.config.FragmentOptions; import com.google.devtools.build.lib.analysis.test.TestConfiguration; import com.google.devtools.build.lib.cmdline.Label; @@ -111,18 +110,58 @@ private ImmutableList getSclConfig( ImmutableMap userOptions) throws FlagSetFunctionException { - var configs = (Dict>) sclContent.getResidualGlobal(CONFIGS); + var unTypeCheckedConfigs = + sclContent.getProject().isEmpty() + ? sclContent.getResidualGlobal(CONFIGS) + : sclContent.getProject().get(CONFIGS); // This project file doesn't define configs, so it must not be used for canonical configs. - if (configs == null) { + if (unTypeCheckedConfigs == null) { return ImmutableList.of(); } + boolean expectedConfigsType = false; + if (unTypeCheckedConfigs instanceof Dict configsAsDict) { + expectedConfigsType = true; + for (var entry : configsAsDict.entrySet()) { + if (!(entry.getKey() instanceof String + && entry.getValue() instanceof Collection values)) { + expectedConfigsType = false; + break; + } + for (var value : values) { + if (!(value instanceof String)) { + expectedConfigsType = false; + break; + } + } + } + } + if (!expectedConfigsType) { + throw new FlagSetFunctionException( + new InvalidProjectFileException( + String.format("%s variable must be a map of strings to lists of strings", CONFIGS)), + Transience.PERSISTENT); + } + var configs = (Dict>) unTypeCheckedConfigs; String sclConfigNameForMessage = sclConfigName; Collection sclConfigValue = null; if (sclConfigName.isEmpty()) { // If there's no --scl_config, try to use the default_config. - var defaultConfigName = (String) sclContent.getResidualGlobal(DEFAULT_CONFIG); + var defaultConfigNameRaw = + sclContent.getProject().isEmpty() + ? sclContent.getResidualGlobal(DEFAULT_CONFIG) + : sclContent.getProject().get(DEFAULT_CONFIG); try { + if (defaultConfigNameRaw != null && !(defaultConfigNameRaw instanceof String)) { + throw new FlagSetFunctionException( + new InvalidProjectFileException( + String.format( + "%s must be a string matching a %s variable definition", + DEFAULT_CONFIG, CONFIGS)), + Transience.PERSISTENT); + } + + String defaultConfigName = (String) defaultConfigNameRaw; sclConfigValue = validateDefaultConfig(defaultConfigName, configs); sclConfigNameForMessage = defaultConfigName; } catch (InvalidProjectFileException e) { diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ProjectFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ProjectFunctionTest.java index 4ec6543a94a70d..f31b9a27072c9a 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ProjectFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ProjectFunctionTest.java @@ -91,6 +91,30 @@ public void projectFunction_returnsDefaultActiveDirectories() throws Exception { assertThat(trie.includes(PathFragment.create("d"))).isFalse(); } + @Test + public void projectFunction_returnsDefaultActiveDirectories_topLevelProjectSchema() + throws Exception { + scratch.file( + "test/PROJECT.scl", + """ + project = { + "active_directories": { "default": ["a", "b/c"] } + } + """); + scratch.file("test/BUILD"); + ProjectValue.Key key = new ProjectValue.Key(Label.parseCanonical("//test:PROJECT.scl")); + + EvaluationResult result = + SkyframeExecutorTestUtils.evaluate(skyframeExecutor, key, false, reporter); + assertThat(result.hasError()).isFalse(); + + ProjectValue value = result.get(key); + PathFragmentPrefixTrie trie = PathFragmentPrefixTrie.of(value.getDefaultActiveDirectory()); + assertThat(trie.includes(PathFragment.create("a"))).isTrue(); + assertThat(trie.includes(PathFragment.create("b/c"))).isTrue(); + assertThat(trie.includes(PathFragment.create("d"))).isFalse(); + } + @Test public void projectFunction_nonEmptyActiveDirectoriesMustHaveADefault() throws Exception { scratch.file("test/PROJECT.scl", "active_directories = { 'foo': ['a', 'b/c'] }"); @@ -133,6 +157,44 @@ public void projectFunction_incorrectType_inList() throws Exception { .matches("expected a list of strings, got element of .+Int32"); } + @Test + public void projectFunction_incorrectProjectType() throws Exception { + scratch.file( + "test/PROJECT.scl", + """ + project = 1 + """); + + scratch.file("test/BUILD"); + ProjectValue.Key key = new ProjectValue.Key(Label.parseCanonical("//test:PROJECT.scl")); + + EvaluationResult result = + SkyframeExecutorTestUtils.evaluate(skyframeExecutor, key, false, reporter); + assertThat(result.hasError()).isTrue(); + assertThat(result.getError().getException()) + .hasMessageThat() + .matches("project variable: expected a map of string to objects, got .+Int32"); + } + + @Test + public void projectFunction_incorrectProjectKeyType() throws Exception { + scratch.file( + "test/PROJECT.scl", + """ + project = {1: [] } + """); + + scratch.file("test/BUILD"); + ProjectValue.Key key = new ProjectValue.Key(Label.parseCanonical("//test:PROJECT.scl")); + + EvaluationResult result = + SkyframeExecutorTestUtils.evaluate(skyframeExecutor, key, false, reporter); + assertThat(result.hasError()).isTrue(); + assertThat(result.getError().getException()) + .hasMessageThat() + .matches("project variable: expected string key, got element of .+Int32"); + } + @Test public void projectFunction_parsesResidualGlobals() throws Exception { scratch.file( diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/config/FlagSetsFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/config/FlagSetsFunctionTest.java index c1f626c3d2a965..92c75cc8230478 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/config/FlagSetsFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/config/FlagSetsFunctionTest.java @@ -297,6 +297,85 @@ public void enforceCanonicalConfigsExtraNativeFlag_withSclConfig_fails() throws assertThat(thrown).hasMessageThat().contains("Found ['--define=foo=bar']"); } + @Test + public void enforceCanonicalConfigs_wrongConfigsType() throws Exception { + scratch.file("test/BUILD"); + scratch.file( + "test/PROJECT.scl", + """ + configs = 1 + """); + setBuildLanguageOptions("--experimental_enable_scl_dialect=true"); + BuildOptions buildOptions = createBuildOptions("--define=foo=bar"); + + FlagSetValue.Key key = + FlagSetValue.Key.create( + Label.parseCanonical("//test:PROJECT.scl"), + "test_config", + buildOptions, + /* userOptions= */ ImmutableMap.of(), + /* enforceCanonical= */ true); + + var thrown = assertThrows(Exception.class, () -> executeFunction(key)); + assertThat(thrown) + .hasMessageThat() + .contains("configs variable must be a map of strings to lists of strings"); + } + + @Test + public void enforceCanonicalConfigs_wrongConfigsKeyType() throws Exception { + scratch.file("test/BUILD"); + scratch.file( + "test/PROJECT.scl", + """ + configs = { + 123: ["--compilation_mode=opt"], + } + """); + setBuildLanguageOptions("--experimental_enable_scl_dialect=true"); + BuildOptions buildOptions = createBuildOptions("--define=foo=bar"); + + FlagSetValue.Key key = + FlagSetValue.Key.create( + Label.parseCanonical("//test:PROJECT.scl"), + "test_config", + buildOptions, + /* userOptions= */ ImmutableMap.of(), + /* enforceCanonical= */ true); + + var thrown = assertThrows(Exception.class, () -> executeFunction(key)); + assertThat(thrown) + .hasMessageThat() + .contains("configs variable must be a map of strings to lists of strings"); + } + + @Test + public void enforceCanonicalConfigs_wrongConfigsValueType() throws Exception { + scratch.file("test/BUILD"); + scratch.file( + "test/PROJECT.scl", + """ + configs = { + "test_config": 123, + } + """); + setBuildLanguageOptions("--experimental_enable_scl_dialect=true"); + BuildOptions buildOptions = createBuildOptions("--define=foo=bar"); + + FlagSetValue.Key key = + FlagSetValue.Key.create( + Label.parseCanonical("//test:PROJECT.scl"), + "test_config", + buildOptions, + /* userOptions= */ ImmutableMap.of(), + /* enforceCanonical= */ true); + + var thrown = assertThrows(Exception.class, () -> executeFunction(key)); + assertThat(thrown) + .hasMessageThat() + .contains("configs variable must be a map of strings to lists of strings"); + } + @Test public void enforceCanonicalConfigsExtraFakeExpandedFlag_withSclConfig_fails() throws Exception { scratch.file( @@ -639,6 +718,37 @@ public void enforceCanonicalConfigsNoSclConfigFlagNonexistentDefaultConfig() thr + " nonexistent config: nonexistent_config"); } + @Test + public void enforceCanonicalConfigs_wrongDefaultConfigType() throws Exception { + createStringFlag("//test:myflag", /* defaultValue= */ "default"); + scratch.file( + "test/PROJECT.scl", + """ + configs = { + "test_config": ['--//test:myflag=test_config_value'], + "other_config": ['--//test:myflag=other_config_value'], + } + default_config = ["test_config"] + """); + BuildOptions buildOptions = + BuildOptions.getDefaultBuildOptionsForFragments( + ruleClassProvider.getFragmentRegistry().getOptionsClasses()); + setBuildLanguageOptions("--experimental_enable_scl_dialect=true"); + + FlagSetValue.Key key = + FlagSetValue.Key.create( + Label.parseCanonical("//test:PROJECT.scl"), + /* sclConfig= */ "", + buildOptions, + /* userOptions= */ ImmutableMap.of(), + /* enforceCanonical= */ true); + + var thrown = assertThrows(Exception.class, () -> executeFunction(key)); + assertThat(thrown) + .hasMessageThat() + .contains("default_config must be a string matching a configs variable definition"); + } + @Test public void enforceCanonicalConfigsNoSclConfigFlagValidDefaultConfig() throws Exception { createStringFlag("//test:myflag", /* defaultValue= */ "default"); @@ -676,6 +786,42 @@ public void enforceCanonicalConfigsNoSclConfigFlagValidDefaultConfig() throws Ex // assertContainsEventWithFrequency("Applying flags from the config 'test_config'", 1); } + @Test + public void basicFlagsetFunctionalityWithTopLevelProjectSchema() throws Exception { + createStringFlag("//test:myflag", /* defaultValue= */ "default"); + scratch.file( + "test/PROJECT.scl", + """ + project = { + "configs": { + "test_config": ['--//test:myflag=test_config_value'], + }, + "default_config": "test_config", + } + """); + BuildOptions buildOptions = + BuildOptions.getDefaultBuildOptionsForFragments( + ruleClassProvider.getFragmentRegistry().getOptionsClasses()); + setBuildLanguageOptions("--experimental_enable_scl_dialect=true"); + + FlagSetValue.Key key = + FlagSetValue.Key.create( + Label.parseCanonical("//test:PROJECT.scl"), + /* sclConfig= */ "", + buildOptions, + /* userOptions= */ ImmutableMap.of(), + /* enforceCanonical= */ true); + FlagSetValue flagSetsValue = executeFunction(key); + + assertThat( + flagSetsValue + .getTopLevelBuildOptions() + .getStarlarkOptions() + .get(Label.parseCanonical("//test:myflag"))) + .isEqualTo("test_config_value"); + assertContainsEvent("Applying flags from the config 'test_config'"); + } + @Test public void clearUserDocumentationOfValidConfigs() throws Exception { createStringFlag("//test:myflag", /* defaultValue= */ "default");