From 868120939e1f125f313e886cba175b78019bc3a6 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 24 Sep 2024 20:52:51 +0200 Subject: [PATCH] Add `--incompatible_no_implicit_watch_label` When the flag is flipped, `ctx.path` no longer watches paths. It used to do so for `Label` arguments only, which is inconsistent with how the other `ctx` methods operate now that they also watch files. --- .../starlark/StarlarkBaseExternalContext.java | 42 +++++++++---------- .../starlark/StarlarkRepositoryContext.java | 14 +++---- .../semantics/BuildLanguageOptions.java | 19 ++++++++- .../build/lib/bazel/repository/starlark/BUILD | 35 +--------------- .../StarlarkRepositoryContextTest.java | 42 ++++++++++++++++++- .../packages/semantics/ConsistencyTest.java | 2 + 6 files changed, 91 insertions(+), 63 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index c715129c70da39..c64101bbacebc1 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -742,7 +742,7 @@ public Object download( checksumValidation = e; } - StarlarkPath outputPath = getPath("download()", output); + StarlarkPath outputPath = getPath(output); WorkspaceRuleEvent w = WorkspaceRuleEvent.newDownloadEvent( urls, @@ -983,7 +983,7 @@ public StructImpl downloadAndExtract( identifyingStringForLogging, thread.getCallerLocation()); - StarlarkPath outputPath = getPath("download_and_extract()", output); + StarlarkPath outputPath = getPath(output); checkInOutputDirectory("write", outputPath); createDirectory(outputPath.getPath()); @@ -1140,7 +1140,7 @@ public void extract( String watchArchive, StarlarkThread thread) throws RepositoryFunctionException, InterruptedException, EvalException { - StarlarkPath archivePath = getPath("extract()", archive); + StarlarkPath archivePath = getPath(archive); if (!archivePath.exists()) { throw new RepositoryFunctionException( @@ -1151,7 +1151,7 @@ public void extract( } maybeWatch(archivePath, ShouldWatch.fromString(watchArchive)); - StarlarkPath outputPath = getPath("extract()", output); + StarlarkPath outputPath = getPath(output); checkInOutputDirectory("write", outputPath); Map renameFilesMap = @@ -1252,7 +1252,7 @@ public boolean isFinished() { public void createFile( Object path, String content, Boolean executable, Boolean legacyUtf8, StarlarkThread thread) throws RepositoryFunctionException, EvalException, InterruptedException { - StarlarkPath p = getPath("file()", path); + StarlarkPath p = getPath(path); byte[] contentBytes; if (legacyUtf8) { contentBytes = content.getBytes(UTF_8); @@ -1352,21 +1352,17 @@ during the analysis phase and thus cannot depends on a target result (the \ + " a path from.") }) public StarlarkPath path(Object path) throws EvalException, InterruptedException { - return getPath("path()", path); + return getPath(path); } - protected StarlarkPath getPath(String method, Object path) - throws EvalException, InterruptedException { - if (path instanceof String) { - return new StarlarkPath(this, workingDirectory.getRelative(path.toString())); - } else if (path instanceof Label label) { - return getPathFromLabel(label); - } else if (path instanceof StarlarkPath starlarkPath) { - return starlarkPath; - } else { + protected StarlarkPath getPath(Object path) throws EvalException, InterruptedException { + return switch (path) { + case String ignored -> new StarlarkPath(this, workingDirectory.getRelative(path.toString())); + case Label label -> getPathFromLabel(label); + case StarlarkPath starlarkPath -> starlarkPath; // This can never happen because we check it in the Starlark interpreter. - throw new IllegalArgumentException("expected string or label for path"); - } + default -> throw new IllegalArgumentException("expected string or label for path"); + }; } @StarlarkMethod( @@ -1399,7 +1395,7 @@ file when it is legal to do so (see watch() docs for more \ }) public String readFile(Object path, String watch, StarlarkThread thread) throws RepositoryFunctionException, EvalException, InterruptedException { - StarlarkPath p = getPath("read()", path); + StarlarkPath p = getPath(path); WorkspaceRuleEvent w = WorkspaceRuleEvent.newReadEvent( p.toString(), identifyingStringForLogging, thread.getCallerLocation()); @@ -1552,7 +1548,7 @@ protected void maybeWatchDirents(Path path, ShouldWatch shouldWatch) }) public void watchForStarlark(Object path) throws RepositoryFunctionException, EvalException, InterruptedException { - maybeWatch(getPath("watch()", path), ShouldWatch.YES); + maybeWatch(getPath(path), ShouldWatch.YES); } // Create parent directories for the given path @@ -1835,7 +1831,7 @@ public StarlarkExecutionResult execute( Path workingDirectoryPath; if (overrideWorkingDirectory != null && !overrideWorkingDirectory.isEmpty()) { - workingDirectoryPath = getPath("execute()", overrideWorkingDirectory).getPath(); + workingDirectoryPath = getPath(overrideWorkingDirectory).getPath(); } else { workingDirectoryPath = workingDirectory; } @@ -1922,7 +1918,11 @@ protected StarlarkPath getPathFromLabel(Label label) throws EvalException, Inter RootedPath rootedPath = RepositoryFunction.getRootedPathFromLabel(label, env); StarlarkPath starlarkPath = new StarlarkPath(this, rootedPath.asPath()); try { - maybeWatch(starlarkPath, ShouldWatch.AUTO); + maybeWatch( + starlarkPath, + starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_WATCH_LABEL) + ? ShouldWatch.NO + : ShouldWatch.AUTO); } catch (RepositoryFunctionException e) { throw Starlark.errorf("%s", e.getCause().getMessage()); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java index 4c5ed3d3a6f040..bac46b6f9314dd 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java @@ -169,7 +169,7 @@ public StructImpl getAttr() { private StarlarkPath externalPath(String method, Object pathObject) throws EvalException, InterruptedException { - StarlarkPath starlarkPath = getPath(method, pathObject); + StarlarkPath starlarkPath = getPath(pathObject); Path path = starlarkPath.getPath(); if (packageLocator.getPathEntries().stream().noneMatch(root -> path.startsWith(root.asPath())) || path.startsWith(workingDirectory)) { @@ -212,8 +212,8 @@ private StarlarkPath externalPath(String method, Object pathObject) }) public void symlink(Object target, Object linkName, StarlarkThread thread) throws RepositoryFunctionException, EvalException, InterruptedException { - StarlarkPath targetPath = getPath("symlink()", target); - StarlarkPath linkPath = getPath("symlink()", linkName); + StarlarkPath targetPath = getPath(target); + StarlarkPath linkPath = getPath(linkName); WorkspaceRuleEvent w = WorkspaceRuleEvent.newSymlinkEvent( targetPath.toString(), @@ -304,8 +304,8 @@ public void createFileFromTemplate( String watchTemplate, StarlarkThread thread) throws RepositoryFunctionException, EvalException, InterruptedException { - StarlarkPath p = getPath("template()", path); - StarlarkPath t = getPath("template()", template); + StarlarkPath p = getPath(path); + StarlarkPath t = getPath(template); Map substitutionMap = Dict.cast(substitutions, String.class, String.class, "substitutions"); WorkspaceRuleEvent w = @@ -442,7 +442,7 @@ the file when it is legal to do so (see watch() docs for more \ public void patch(Object patchFile, StarlarkInt stripI, String watchPatch, StarlarkThread thread) throws EvalException, RepositoryFunctionException, InterruptedException { int strip = Starlark.toInt(stripI, "strip"); - StarlarkPath starlarkPath = getPath("patch()", patchFile); + StarlarkPath starlarkPath = getPath(patchFile); WorkspaceRuleEvent w = WorkspaceRuleEvent.newPatchEvent( starlarkPath.toString(), @@ -487,7 +487,7 @@ public void patch(Object patchFile, StarlarkInt stripI, String watchPatch, Starl }) public void watchTree(Object path) throws EvalException, InterruptedException, RepositoryFunctionException { - StarlarkPath p = getPath("watch_tree()", path); + StarlarkPath p = getPath(path); if (!p.isDir()) { throw Starlark.errorf("can't call watch_tree() on non-directory %s", p); } diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java index 2e2d72b6eecec0..ab8585adcd6744 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java @@ -53,7 +53,8 @@ *
  • Boolean semantic flags can toggle StarlarkMethod-annotated Java methods (or their * parameters) on or off, making them selectively invisible to Starlark. To do this, add a new * entry to {@link BuildLanguageOptions}, then specify the identifier in {@link - * StarlarkMethod#enableOnlyWithFlag} or {@link StarlarkMethod#disableWithFlag}. + * net.starlark.java.annot.StarlarkMethod#enableOnlyWithFlag} or {@link + * net.starlark.java.annot.StarlarkMethod#disableWithFlag}. * */ public final class BuildLanguageOptions extends OptionsBase { @@ -282,6 +283,19 @@ public final class BuildLanguageOptions extends OptionsBase { + " function.") public boolean experimentalIsolatedExtensionUsages; + @Option( + name = "incompatible_no_implicit_watch_label", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + metadataTags = OptionMetadataTag.INCOMPATIBLE_CHANGE, + effectTags = OptionEffectTag.LOADING_AND_ANALYSIS, + help = + "If true, then methods on repository_ctx that are passed a Label will no" + + " longer automatically watch the file under that label for changes even if" + + " watch = \"no\" and repository_ctx.path no longer causes" + + " the returned path to be watched. Use repository_ctx.watch instead.") + public boolean incompatibleNoImplicitWatchLabel; + @Option( name = "incompatible_stop_exporting_build_file_path", defaultValue = "false", @@ -820,6 +834,7 @@ public StarlarkSemantics toStarlarkSemantics() { .setBool(ENABLE_BZLMOD, enableBzlmod) .setBool(ENABLE_WORKSPACE, enableWorkspace) .setBool(EXPERIMENTAL_ISOLATED_EXTENSION_USAGES, experimentalIsolatedExtensionUsages) + .setBool(INCOMPATIBLE_NO_IMPLICIT_WATCH_LABEL, incompatibleNoImplicitWatchLabel) .setBool(EXPERIMENTAL_ACTION_RESOURCE_SET, experimentalActionResourceSet) .setBool(EXPERIMENTAL_GOOGLE_LEGACY_API, experimentalGoogleLegacyApi) .setBool(EXPERIMENTAL_PLATFORMS_API, experimentalPlatformsApi) @@ -930,6 +945,8 @@ public StarlarkSemantics toStarlarkSemantics() { public static final String ENABLE_WORKSPACE = "-enable_workspace"; public static final String EXPERIMENTAL_ISOLATED_EXTENSION_USAGES = "-experimental_isolated_extension_usages"; + public static final String INCOMPATIBLE_NO_IMPLICIT_WATCH_LABEL = + "-incompatible_no_implicit_watch_label"; public static final String EXPERIMENTAL_GOOGLE_LEGACY_API = "-experimental_google_legacy_api"; public static final String EXPERIMENTAL_PLATFORMS_API = "-experimental_platforms_api"; public static final String EXPERIMENTAL_REPO_REMOTE_EXEC = "-experimental_repo_remote_exec"; diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD index 2d452a8f230929..cc5f3a3fd0d10d 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD @@ -19,6 +19,7 @@ java_library( srcs = glob(["*.java"]), deps = [ "//src/main/java/com/google/devtools/build/lib:runtime", + "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//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", @@ -31,6 +32,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data", + "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_value", "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", @@ -54,7 +56,6 @@ java_library( java_test( name = "StarlarkTests", testonly = 1, - srcs = glob(["*.java"]), tags = [ "rules", ], @@ -63,36 +64,4 @@ java_test( ":StarlarkTests_lib", "//src/test/java/com/google/devtools/build/lib:test_runner", ], - deps = [ - "//src/main/java/com/google/devtools/build/lib:runtime", - "//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/repository/downloader", - "//src/main/java/com/google/devtools/build/lib/bazel/repository/starlark", - "//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/pkgcache", - "//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function", - "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data", - "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster", - "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository", - "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", - "//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/build/lib/vfs/inmemoryfs", - "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", - "//src/main/java/net/starlark/java/eval", - "//src/main/java/net/starlark/java/syntax", - "//src/test/java/com/google/devtools/build/lib/analysis/util", - "//src/test/java/com/google/devtools/build/lib/starlark/util", - "//src/test/java/com/google/devtools/build/lib/testutil", - "//third_party:guava", - "//third_party:jsr305", - "//third_party:junit4", - "//third_party:mockito", - "//third_party:truth", - ], ) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java index 15a8b13fe723fc..2bfccb891fd0d4 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java @@ -17,6 +17,7 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -25,13 +26,16 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; import com.google.common.io.CharStreams; +import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ServerDirectories; import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.packages.Attribute; +import com.google.devtools.build.lib.packages.BuildFileName; import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.Package.Builder.PackageSettings; import com.google.devtools.build.lib.packages.PackageOverheadEstimator; @@ -48,6 +52,7 @@ import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor.ExecutionResult; import com.google.devtools.build.lib.skyframe.BazelSkyframeExecutorConstants; +import com.google.devtools.build.lib.skyframe.PackageLookupValue; import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -91,6 +96,7 @@ public final class StarlarkRepositoryContextTest { private Root root; private Path workspaceFile; private StarlarkRepositoryContext context; + private Label fakeFileLabel; private static final StarlarkThread thread = StarlarkThread.createTransient(Mutability.create("test"), StarlarkSemantics.DEFAULT); @@ -159,6 +165,13 @@ private void setUpContextForRule( DownloadManager downloader = Mockito.mock(DownloadManager.class); SkyFunction.Environment environment = Mockito.mock(SkyFunction.Environment.class); when(environment.getListener()).thenReturn(listener); + fakeFileLabel = Label.parseCanonical("//:foo"); + when(environment.getValue(PackageLookupValue.key(fakeFileLabel.getPackageIdentifier()))) + .thenReturn( + PackageLookupValue.success( + Root.fromPath(workspaceFile.getParentDirectory()), BuildFileName.BUILD)); + when(environment.getValueOrThrow(any(), eq(IOException.class))) + .thenReturn(Mockito.mock(FileValue.class)); PathPackageLocator packageLocator = new PathPackageLocator( outputDirectory, @@ -188,11 +201,16 @@ private void setUpContextForRule( } private void setUpContextForRule(String name) throws Exception { + setUpContextForRule(name, StarlarkSemantics.DEFAULT); + } + + private void setUpContextForRule(String name, StarlarkSemantics starlarkSemantics) + throws Exception { setUpContextForRule( ImmutableMap.of("name", name), ImmutableSet.of(), ImmutableMap.of("FOO", "BAR"), - StarlarkSemantics.DEFAULT, + starlarkSemantics, /* repoRemoteExecutor= */ null); } @@ -491,4 +509,26 @@ public void testWorkspaceRoot() throws Exception { setUpContextForRule("test"); assertThat(context.getWorkspaceRoot().getPath()).isEqualTo(root.asPath()); } + + @Test + public void testNoIncompatibleNoImplicitWatchLabel() throws Exception { + setUpContextForRule("test"); + scratch.file(root.getRelative("foo").getPathString()); + context.path(fakeFileLabel); + context.readFile(fakeFileLabel, "no", thread); + assertThat(context.getRecordedFileInputs()).isNotEmpty(); + } + + @Test + public void testIncompatibleNoImplicitWatchLabel() throws Exception { + setUpContextForRule( + "test", + StarlarkSemantics.DEFAULT.toBuilder() + .setBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_WATCH_LABEL, true) + .build()); + scratch.file(root.getRelative("foo").getPathString()); + context.path(fakeFileLabel); + context.readFile(fakeFileLabel, "no", thread); + assertThat(context.getRecordedFileInputs()).isEmpty(); + } } diff --git a/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java index e3b7a5e4db7af9..d2b8299d5cd777 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java @@ -127,6 +127,7 @@ private static BuildLanguageOptions buildRandomOptions(Random rand) throws Excep "--enable_bzlmod=" + rand.nextBoolean(), "--enable_workspace=" + rand.nextBoolean(), "--experimental_isolated_extension_usages=" + rand.nextBoolean(), + "--incompatible_no_implicit_watch_label=" + rand.nextBoolean(), "--experimental_google_legacy_api=" + rand.nextBoolean(), "--experimental_platforms_api=" + rand.nextBoolean(), "--incompatible_allow_tags_propagation=" + rand.nextBoolean(), // flag, Java names differ @@ -174,6 +175,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) { .setBool(BuildLanguageOptions.ENABLE_BZLMOD, rand.nextBoolean()) .setBool(BuildLanguageOptions.ENABLE_WORKSPACE, rand.nextBoolean()) .setBool(BuildLanguageOptions.EXPERIMENTAL_ISOLATED_EXTENSION_USAGES, rand.nextBoolean()) + .setBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_WATCH_LABEL, rand.nextBoolean()) .setBool(BuildLanguageOptions.EXPERIMENTAL_GOOGLE_LEGACY_API, rand.nextBoolean()) .setBool(BuildLanguageOptions.EXPERIMENTAL_PLATFORMS_API, rand.nextBoolean()) .setBool(BuildLanguageOptions.INCOMPATIBLE_ALLOW_TAGS_PROPAGATION, rand.nextBoolean())