From 404eb64eaafadce17ae752fa527068c92d8bec7f Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 1 Aug 2023 00:52:05 -0700 Subject: [PATCH 01/17] Automatic code cleanup. PiperOrigin-RevId: 552718593 Change-Id: Ia1520f0524810a088d18e30ff81450dba1bfa28c --- .../rules/java/proto/GeneratedExtensionRegistryProvider.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/proto/GeneratedExtensionRegistryProvider.java b/src/main/java/com/google/devtools/build/lib/rules/java/proto/GeneratedExtensionRegistryProvider.java index ef0386692d7f0f..f908f75e3fb88e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/proto/GeneratedExtensionRegistryProvider.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/proto/GeneratedExtensionRegistryProvider.java @@ -158,10 +158,6 @@ private Provider() { super(NAME, GeneratedExtensionRegistryProvider.class); } - public String getName() { - return NAME; - } - @Override public GeneratedExtensionRegistryProvider create( Label generatingRuleLabel, From 2317f3798a8532fd8bf29a432aa7c9c55774c925 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 1 Aug 2023 02:16:27 -0700 Subject: [PATCH 02/17] [Skymeld] Account for the GLOB -> GLOB inconsistency. This can happen with `--discard_analysis_cache`, when a GLOB node (execution phase) refers to another GLOB node (analysis phase, now cleared). PiperOrigin-RevId: 552736907 Change-Id: I039fe5395df7c5aaa496ccaddb508bdad62a67a1 --- .../build/lib/skyframe/SkymeldInconsistencyReceiver.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldInconsistencyReceiver.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldInconsistencyReceiver.java index 4c3c15d50c484a..70ca31d2e414c3 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldInconsistencyReceiver.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldInconsistencyReceiver.java @@ -30,7 +30,9 @@ public class SkymeldInconsistencyReceiver implements GraphInconsistencyReceiver { private static final ImmutableMap SKYMELD_EXPECTED_MISSING_CHILDREN = - ImmutableMap.of(SkyFunctions.ACTION_EXECUTION, SkyFunctions.GLOB); + ImmutableMap.of( + SkyFunctions.ACTION_EXECUTION, SkyFunctions.GLOB, + SkyFunctions.GLOB, SkyFunctions.GLOB); private final boolean heuristicallyDropNodes; From 681a0c181510674a69035138735026156e2e6dd8 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 1 Aug 2023 02:31:50 -0700 Subject: [PATCH 03/17] Remove the no-op flag `--experimental_skymeld_ui`. PiperOrigin-RevId: 552739888 Change-Id: Ie5e7acb5ce980bb93545db5bf31a09a8d080d76b --- .../devtools/build/lib/bazel/rules/BazelRulesModule.java | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java index 459360be22ef57..dbf390fe874966 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java @@ -364,14 +364,6 @@ public static class BuildGraveyardOptions extends OptionsBase { help = "Deprecated no-op.") public List availabilityInfoExempt; - @Option( - name = "experimental_skymeld_ui", - defaultValue = "false", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - effectTags = {OptionEffectTag.NO_OP}, - help = "No-op. To be removed.") - public boolean skymeldUi; - @Option( name = "experimental_collect_local_action_metrics", defaultValue = "true", From 5563046420a0bca89f8f90e91789df957221f754 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 1 Aug 2023 02:38:38 -0700 Subject: [PATCH 04/17] Stop creating new empty instances of `JavaPluginDataInfo` in Starlark `JavaInfo` PiperOrigin-RevId: 552741357 Change-Id: I1ad9be47fac43e15fe3d33d8f9dd1c99f12f6663 --- .../builtins_bzl/common/java/java_info.bzl | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/main/starlark/builtins_bzl/common/java/java_info.bzl b/src/main/starlark/builtins_bzl/common/java/java_info.bzl index ada4d6098e7f68..289540177a41c8 100644 --- a/src/main/starlark/builtins_bzl/common/java/java_info.bzl +++ b/src/main/starlark/builtins_bzl/common/java/java_info.bzl @@ -817,6 +817,16 @@ _EMPTY_PLUGIN_DATA = _JavaPluginDataInfo( processor_data = depset(), ) +def _create_plugin_data_info(*, processor_classes, processor_jars, processor_data): + if processor_classes or processor_jars or processor_data: + return _JavaPluginDataInfo( + processor_classes = processor_classes, + processor_jars = processor_jars, + processor_data = processor_data, + ) + else: + return _EMPTY_PLUGIN_DATA + def disable_plugin_info_annotation_processing(plugin_info): """Returns a copy of the provided JavaPluginInfo without annotation processing info @@ -827,7 +837,7 @@ def disable_plugin_info_annotation_processing(plugin_info): (JavaPluginInfo) a new, transformed instance. """ return _new_javaplugininfo( - plugins = _JavaPluginDataInfo( + plugins = _create_plugin_data_info( processor_classes = depset(order = "preorder"), # Preserve the processor path, since it may contain Error Prone plugins # which will be service-loaded by JavaBuilder. @@ -869,7 +879,7 @@ def _has_plugin_data(plugin_data): ) def _merge_plugin_data(datas): - return _JavaPluginDataInfo( + return _create_plugin_data_info( processor_classes = depset(transitive = [p.processor_classes for p in datas]), processor_jars = depset(transitive = [p.processor_jars for p in datas]), processor_data = depset(transitive = [p.processor_data for p in datas]), @@ -903,7 +913,7 @@ def _javaplugininfo_init( java_infos = merge(runtime_deps) processor_data = data if type(data) == "depset" else depset(data) - plugins = _JavaPluginDataInfo( + plugins = _create_plugin_data_info( processor_classes = depset([processor_class]) if processor_class else depset(), processor_jars = java_infos.transitive_runtime_jars, processor_data = processor_data, From ef9c3b39d2655f2b3bf9923b4a5efdc442e77d33 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 1 Aug 2023 02:50:44 -0700 Subject: [PATCH 05/17] Resolve symlinks correctly even when they cross filesystems. Instead of delegating RemoteActionFileSystem#resolveSymbolicLinks to the local filesystem, use its base implementation together with a custom implementation of FileSystem#resolveOneLink which is able to stitch together the local and remote filesystems. In addition, have every filesystem method that needs to resolve symlinks symlinks call resolveSymbolicLinks explicitly (as delegating to the underlying filesystems would cause cross-filesystem symlinks not to be followed correctly). The end goal of these changes is to let SkyframeActionExecutor traverse the RemoteActionFileSystem normally when collecting metadata for non-symlink outputs materialized as symlinks, instead of relying on the latter to provide pre-constructed metadata. This is only a step on the way there; the way SkyframeActionExecutor collects metadata remains unchanged for now. PiperOrigin-RevId: 552743574 Change-Id: I5921ded8f7b9c51c31406daf0441ee3c2e17308b --- .../lib/remote/RemoteActionFileSystem.java | 112 +++++++---- .../google/devtools/build/lib/remote/BUILD | 1 + .../remote/RemoteActionFileSystemTest.java | 181 +++++++++++++++++- 3 files changed, 250 insertions(+), 44 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java index ab65bf486a58c8..832199e4969b9a 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java @@ -88,6 +88,16 @@ public class RemoteActionFileSystem extends AbstractFileSystemWithCustomStat { @Nullable private ActionExecutionMetadata action = null; @Nullable private MetadataInjector metadataInjector = null; + /** Describes how to handle symlinks when calling {@link #statUnchecked}. */ + private enum FollowMode { + /** Dereference the entire path. This is equivalent to {@link Symlinks.FOLLOW}. */ + FOLLOW_ALL, + /** Dereference the parent path. This is equivalent to {@link Symlinks.NOFOLLOW}. */ + FOLLOW_PARENT, + /** Do not dereference. This is only used internally to resolve symlinks efficiently. */ + FOLLOW_NONE + }; + RemoteActionFileSystem( FileSystem localFs, PathFragment execRootFragment, @@ -144,7 +154,7 @@ boolean isRemote(Path path) { } private boolean isRemote(PathFragment path) { - var status = statInMemory(path, /* followSymlinks= */ true); + var status = statInMemory(path, FollowMode.FOLLOW_ALL); return (status instanceof FileStatusWithMetadata) && ((FileStatusWithMetadata) status).getMetadata().isRemote(); } @@ -243,7 +253,7 @@ private void maybeInjectMetadataForSymlinkOrDownload(PathFragment linkPath, Arti } else { RemoteFileArtifactValue metadata = null; - var status = statInMemory(targetPath, /* followSymlinks= */ true); + var status = statInMemory(targetPath, FollowMode.FOLLOW_ALL); if (status instanceof FileStatusWithMetadata && ((FileStatusWithMetadata) status).getMetadata().isRemote()) { metadata = (RemoteFileArtifactValue) ((FileStatusWithMetadata) status).getMetadata(); @@ -340,7 +350,7 @@ public byte[] getxattr(PathFragment path, String name, boolean followSymlinks) @Override protected byte[] getFastDigest(PathFragment path) throws IOException { - var stat = statInMemory(path, /* followSymlinks= */ true); + var stat = statInMemory(path, FollowMode.FOLLOW_ALL); if (stat instanceof FileStatusWithDigest) { return ((FileStatusWithDigest) stat).getDigest(); } @@ -349,7 +359,7 @@ protected byte[] getFastDigest(PathFragment path) throws IOException { @Override protected byte[] getDigest(PathFragment path) throws IOException { - var status = statInMemory(path, /* followSymlinks= */ true); + var status = statInMemory(path, FollowMode.FOLLOW_ALL); if (status instanceof FileStatusWithDigest) { return ((FileStatusWithDigest) status).getDigest(); } @@ -361,7 +371,7 @@ protected byte[] getDigest(PathFragment path) throws IOException { // permissions. private boolean existsInMemory(PathFragment path) { - return statInMemory(path, /* followSymlinks= */ true) != null; + return statInMemory(path, FollowMode.FOLLOW_ALL) != null; } @Override @@ -443,9 +453,12 @@ protected void chmod(PathFragment path, int mode) throws IOException { @Override protected PathFragment readSymbolicLink(PathFragment path) throws IOException { - if (isRemote(path)) { - // We don't support symlinks as remote action outputs. - throw new IOException(path + " is not a symbolic link"); + if (isOutput(path)) { + try { + return remoteOutputTree.getPath(path).readSymbolicLink(); + } catch (FileNotFoundException e) { + // Intentionally ignored. + } } return localFs.getPath(path).readSymbolicLink(); } @@ -453,33 +466,32 @@ protected PathFragment readSymbolicLink(PathFragment path) throws IOException { @Override protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFragment) throws IOException { - localFs.getPath(linkPath).createSymbolicLink(targetFragment); if (isOutput(linkPath)) { remoteOutputTree.getPath(linkPath).createSymbolicLink(targetFragment); } + + localFs.getPath(linkPath).createSymbolicLink(targetFragment); } + @Nullable @Override - protected Path resolveSymbolicLinks(PathFragment path) throws IOException { - return localFs.getPath(path).resolveSymbolicLinks(); + protected PathFragment resolveOneLink(PathFragment path) throws IOException { + // The base implementation attempts to readSymbolicLink first and falls back to stat, but that + // unnecessarily allocates a NotASymlinkException in the overwhelmingly likely non-symlink case. + // It's more efficient to stat unconditionally. + // + // The parent path has already been canonicalized, so FOLLOW_NONE is effectively the same as + // FOLLOW_PARENT, but much more efficient as it doesn't call stat recursively. + var stat = statUnchecked(path, FollowMode.FOLLOW_NONE); + return stat.isSymbolicLink() ? readSymbolicLink(path) : null; } // -------------------- Implementations based on stat() -------------------- @Override protected long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException { - try { - // We can't get mtime for a remote file, use mtime of in-memory file node instead. - return remoteOutputTree - .getPath(path) - .getLastModifiedTime(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW); - } catch (FileNotFoundException e) { - // Intentionally ignored - } - - return localFs - .getPath(path) - .getLastModifiedTime(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW); + FileStatus stat = stat(path, followSymlinks); + return stat.getLastModifiedTime(); } @Override @@ -497,11 +509,6 @@ protected boolean exists(PathFragment path, boolean followSymlinks) { } } - @Override - public boolean exists(PathFragment path) { - return exists(path, /* followSymlinks= */ true); - } - @Nullable @Override protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException { @@ -524,15 +531,30 @@ protected FileStatus statNullable(PathFragment path, boolean followSymlinks) { @Override protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException { - var status = statInMemory(path, followSymlinks); + return statUnchecked(path, followSymlinks ? FollowMode.FOLLOW_ALL : FollowMode.FOLLOW_PARENT); + } + + @Nullable + private FileStatus statUnchecked(PathFragment path, FollowMode followMode) throws IOException { + if (followMode == FollowMode.FOLLOW_ALL) { + path = resolveSymbolicLinks(path).asFragment(); + } else if (followMode == FollowMode.FOLLOW_PARENT) { + PathFragment parent = path.getParentDirectory(); + if (parent != null) { + path = resolveSymbolicLinks(parent).asFragment().getChild(path.getBaseName()); + } + } + + var status = statInMemory(path, followMode); if (status != null) { return status; } - return localFs.getPath(path).stat(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW); + // The path has already been canonicalized above. + return localFs.getPath(path).stat(Symlinks.NOFOLLOW); } @Nullable - private FileStatus statInMemory(PathFragment path, boolean followSymlinks) { + private FileStatus statInMemory(PathFragment path, FollowMode followMode) { if (path.startsWith(execRoot)) { var execPath = path.relativeTo(execRoot); var metadata = inputArtifactData.getMetadata(execPath); @@ -541,7 +563,8 @@ private FileStatus statInMemory(PathFragment path, boolean followSymlinks) { } } - return remoteOutputTree.statNullable(path, followSymlinks); + return remoteOutputTree.statNullable( + path, /* followSymlinks= */ followMode == FollowMode.FOLLOW_ALL); } private static FileStatusWithMetadata statFromMetadata(FileArtifactValue m) { @@ -730,12 +753,12 @@ protected Collection readdir(PathFragment path, boolean followSymlinks) boolean existsRemotely = false; + path = resolveSymbolicLinks(path).asFragment(); + if (isOutput(path)) { try { - for (var entry : - remoteOutputTree - .getPath(path) - .readdir(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW)) { + for (var entry : remoteOutputTree.getPath(path).readdir(Symlinks.NOFOLLOW)) { + entry = maybeFollowSymlinkForDirent(path, entry, followSymlinks); entries.put(entry.getName(), entry); } existsRemotely = true; @@ -745,8 +768,8 @@ protected Collection readdir(PathFragment path, boolean followSymlinks) } try { - for (var entry : - localFs.getPath(path).readdir(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW)) { + for (var entry : localFs.getPath(path).readdir(Symlinks.NOFOLLOW)) { + entry = maybeFollowSymlinkForDirent(path, entry, followSymlinks); entries.put(entry.getName(), entry); } } catch (FileNotFoundException e) { @@ -759,6 +782,19 @@ protected Collection readdir(PathFragment path, boolean followSymlinks) return ImmutableList.sortedCopyOf(entries.values()); } + private Dirent maybeFollowSymlinkForDirent( + PathFragment dirPath, Dirent entry, boolean followSymlinks) { + if (!followSymlinks || !entry.getType().equals(Dirent.Type.SYMLINK)) { + return entry; + } + PathFragment path = dirPath.getChild(entry.getName()); + FileStatus st = statNullable(path, /* followSymlinks= */ true); + if (st == null) { + return new Dirent(entry.getName(), Dirent.Type.UNKNOWN); + } + return new Dirent(entry.getName(), direntFromStat(st)); + } + /* * -------------------- TODO(buchgr): Not yet implemented -------------------- * diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index 58f0345d83c454..874d5a7500e8b9 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -124,6 +124,7 @@ java_test( "@googleapis//:google_rpc_code_java_proto", "@googleapis//:google_rpc_error_details_java_proto", "@googleapis//:google_rpc_status_java_proto", + "@maven//:com_google_testparameterinjector_test_parameter_injector", "@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_grpc", "@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto", "@remoteapis//:build_bazel_semver_semver_java_proto", diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java index 9f5eea83d5800e..9f4de20aba844d 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java @@ -58,6 +58,7 @@ import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileStatusWithDigest; import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.FileSystem.NotASymlinkException; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -66,18 +67,19 @@ import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import com.google.errorprone.annotations.CanIgnoreReturnValue; +import com.google.testing.junit.testparameterinjector.TestParameter; +import com.google.testing.junit.testparameterinjector.TestParameterInjector; import java.io.FileNotFoundException; import java.io.IOException; import java.util.Map; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; import org.mockito.ArgumentCaptor; import org.mockito.stubbing.Answer; /** Tests for {@link RemoteActionFileSystem} */ -@RunWith(JUnit4.class) +@RunWith(TestParameterInjector.class) public final class RemoteActionFileSystemTest extends RemoteActionFileSystemTestBase { private static final RemoteOutputChecker DUMMY_REMOTE_OUTPUT_CHECKER = new RemoteOutputChecker( @@ -95,6 +97,21 @@ public final class RemoteActionFileSystemTest extends RemoteActionFileSystemTest private final ArtifactRoot outputRoot = ArtifactRoot.asDerivedRoot(execRoot, RootType.Output, RELATIVE_OUTPUT_PATH); + enum FilesystemTestParam { + LOCAL, + REMOTE; + + FileSystem getFilesystem(RemoteActionFileSystem actionFs) { + switch (this) { + case LOCAL: + return actionFs.getLocalFileSystem(); + case REMOTE: + return actionFs.getRemoteOutputTree(); + } + throw new IllegalStateException(); + } + }; + @Before public void setUp() throws IOException { outputRoot.getRoot().asPath().createDirectoryAndParents(); @@ -355,7 +372,7 @@ public void statAndExists_fromInputArtifactData() throws Exception { FileArtifactValue metadata = checkNotNull(inputs.getInputMetadata(artifact)); RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(inputs); - assertThat(actionFs.exists(path)).isTrue(); + assertThat(actionFs.exists(path, /* followSymlinks= */ true)).isTrue(); FileStatus st = actionFs.stat(path, /* followSymlinks= */ true); assertThat(st.isFile()).isTrue(); @@ -371,7 +388,7 @@ public void statAndExists_fromRemoteOutputTree() throws Exception { FileArtifactValue metadata = injectRemoteFile(actionFs, artifact.getPath().asFragment(), "remote contents"); - assertThat(actionFs.exists(path)).isTrue(); + assertThat(actionFs.exists(path, /* followSymlinks= */ true)).isTrue(); FileStatus st = actionFs.stat(path, /* followSymlinks= */ true); assertThat(st.isFile()).isTrue(); @@ -393,6 +410,36 @@ public void statAndExists_fromLocalFilesystem() throws Exception { assertThat(st.getSize()).isEqualTo("local contents".getBytes(UTF_8).length); } + @Test + public void statAndExists_followSymlinks( + @TestParameter FilesystemTestParam from, @TestParameter FilesystemTestParam to) + throws Exception { + RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(); + FileSystem fromFs = from.getFilesystem(actionFs); + FileSystem toFs = to.getFilesystem(actionFs); + + PathFragment linkPath = getOutputPath("sym"); + PathFragment targetPath = getOutputPath("target"); + fromFs.getPath(linkPath).createSymbolicLink(execRoot.getRelative(targetPath).asFragment()); + + assertThat(actionFs.exists(linkPath, /* followSymlinks= */ false)).isTrue(); + assertThat(actionFs.exists(linkPath, /* followSymlinks= */ true)).isFalse(); + assertThat(actionFs.stat(linkPath, /* followSymlinks= */ false).isSymbolicLink()).isTrue(); + assertThrows( + FileNotFoundException.class, () -> actionFs.stat(linkPath, /* followSymlinks= */ true)); + + if (toFs.equals(actionFs.getLocalFileSystem())) { + writeLocalFile(actionFs, targetPath, "content"); + } else { + injectRemoteFile(actionFs, targetPath, "content"); + } + + assertThat(actionFs.exists(linkPath, /* followSymlinks= */ false)).isTrue(); + assertThat(actionFs.stat(linkPath, /* followSymlinks= */ false).isSymbolicLink()).isTrue(); + assertThat(actionFs.exists(linkPath, /* followSymlinks= */ true)).isTrue(); + assertThat(actionFs.stat(linkPath, /* followSymlinks= */ true).isFile()).isTrue(); + } + @Test public void statAndExists_notFound() throws Exception { RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(); @@ -419,6 +466,7 @@ public void readdir_fromRemoteOutputTree() throws Exception { assertReaddir( actionFs, dirPath, + /* followSymlinks= */ true, new Dirent("out1", Dirent.Type.FILE), new Dirent("out2", Dirent.Type.FILE), new Dirent("subdir", Dirent.Type.DIRECTORY)); @@ -438,6 +486,7 @@ public void readdir_fromLocalFilesystem() throws Exception { assertReaddir( actionFs, dirPath, + /* followSymlinks= */ true, new Dirent("out1", Dirent.Type.FILE), new Dirent("out2", Dirent.Type.FILE), new Dirent("subdir", Dirent.Type.DIRECTORY)); @@ -460,11 +509,44 @@ public void readdir_fromBothFilesystems() throws Exception { assertReaddir( actionFs, dirPath, + /* followSymlinks= */ true, new Dirent("out1", Dirent.Type.FILE), new Dirent("out2", Dirent.Type.FILE), new Dirent("subdir", Dirent.Type.DIRECTORY)); } + @Test + public void readdir_followSymlinks( + @TestParameter FilesystemTestParam from, @TestParameter FilesystemTestParam to) + throws Exception { + RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(); + FileSystem fromFs = from.getFilesystem(actionFs); + FileSystem toFs = to.getFilesystem(actionFs); + + PathFragment dirPath = getOutputPath("dir"); + PathFragment linkPath = getOutputPath("dir/sym"); + PathFragment targetPath = getOutputPath("target"); + + fromFs.getPath(dirPath).createDirectory(); + fromFs.getPath(linkPath).createSymbolicLink(execRoot.getRelative(targetPath).asFragment()); + + assertReaddir( + actionFs, dirPath, /* followSymlinks= */ false, new Dirent("sym", Dirent.Type.SYMLINK)); + assertReaddir( + actionFs, dirPath, /* followSymlinks= */ true, new Dirent("sym", Dirent.Type.UNKNOWN)); + + if (toFs.equals(actionFs.getLocalFileSystem())) { + writeLocalFile(actionFs, targetPath, "content"); + } else { + injectRemoteFile(actionFs, targetPath, "content"); + } + + assertReaddir( + actionFs, dirPath, /* followSymlinks= */ false, new Dirent("sym", Dirent.Type.SYMLINK)); + assertReaddir( + actionFs, dirPath, /* followSymlinks= */ true, new Dirent("sym", Dirent.Type.FILE)); + } + @Test public void readdir_notFound() throws Exception { RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(); @@ -477,8 +559,12 @@ public void readdir_notFound() throws Exception { } private void assertReaddir( - RemoteActionFileSystem actionFs, PathFragment dirPath, Dirent... expected) throws Exception { - assertThat(actionFs.readdir(dirPath, /* followSymlinks= */ true)) + RemoteActionFileSystem actionFs, + PathFragment dirPath, + boolean followSymlinks, + Dirent... expected) + throws Exception { + assertThat(actionFs.readdir(dirPath, followSymlinks)) .containsExactlyElementsIn(expected) .inOrder(); assertThat(actionFs.getDirectoryEntries(dirPath)) @@ -486,6 +572,42 @@ private void assertReaddir( .inOrder(); } + @Test + public void readSymbolicLink_fromLocalFilesystem() throws Exception { + RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(); + PathFragment filePath = getOutputPath("file"); + PathFragment linkPath = getOutputPath("sym"); + PathFragment targetPath = PathFragment.create("/some/path"); + actionFs.getLocalFileSystem().getPath(linkPath).createSymbolicLink(targetPath); + writeLocalFile(actionFs, filePath, "contents"); + + assertThat(actionFs.readSymbolicLink(linkPath)).isEqualTo(targetPath); + + assertThrows(NotASymlinkException.class, () -> actionFs.readSymbolicLink(filePath)); + } + + @Test + public void readSymbolicLink_fromRemoteFilesystem() throws Exception { + RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(); + PathFragment filePath = getOutputPath("file"); + PathFragment linkPath = getOutputPath("sym"); + PathFragment targetPath = PathFragment.create("/some/path"); + actionFs.getRemoteOutputTree().getPath(linkPath).createSymbolicLink(targetPath); + injectRemoteFile(actionFs, filePath, "contents"); + + assertThat(actionFs.readSymbolicLink(linkPath)).isEqualTo(targetPath); + + assertThrows(NotASymlinkException.class, () -> actionFs.readSymbolicLink(filePath)); + } + + @Test + public void readSymbolicLink_notFound() throws Exception { + RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(); + PathFragment linkPath = getOutputPath("sym"); + + assertThrows(FileNotFoundException.class, () -> actionFs.readSymbolicLink(linkPath)); + } + @Test public void createSymbolicLink_localFileArtifact() throws Exception { // arrange @@ -654,6 +776,53 @@ public void createSymbolicLink_unresolvedSymlink() throws Exception { verifyNoInteractions(metadataInjector); } + @Test + public void resolveSymbolicLinks( + @TestParameter FilesystemTestParam a, @TestParameter FilesystemTestParam b) throws Exception { + RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(); + FileSystem aFs = a.getFilesystem(actionFs); + FileSystem bFs = b.getFilesystem(actionFs); + + // /a + // |- asub + // | `- afile + // `- abssym -> /b/bsub + // `- relsym -> asub + // /b + // `- bsub + // `- bfile + + aFs.getPath(getOutputPath("a/asub")).createDirectoryAndParents(); + aFs.getPath(getOutputPath("a/abssym")).createSymbolicLink(getOutputPath("b/bsub")); + aFs.getPath(getOutputPath("a/relsym")).createSymbolicLink(PathFragment.create("asub")); + if (aFs.equals(actionFs.getLocalFileSystem())) { + writeLocalFile(actionFs, getOutputPath("a/asub/afile"), "content"); + } else { + injectRemoteFile(actionFs, getOutputPath("a/asub/afile"), "content"); + } + + bFs.getPath(getOutputPath("b/bsub")).createDirectoryAndParents(); + if (bFs.equals(actionFs.getLocalFileSystem())) { + writeLocalFile(actionFs, getOutputPath("b/bsub/bfile"), "content"); + } else { + injectRemoteFile(actionFs, getOutputPath("b/bsub/bfile"), "content"); + } + + assertThat(actionFs.getPath(getOutputPath("a/relsym/afile")).resolveSymbolicLinks()) + .isEqualTo(actionFs.getPath(getOutputPath("a/asub/afile"))); + + assertThrows( + FileNotFoundException.class, + () -> actionFs.getPath(getOutputPath("a/bsub/nofile")).resolveSymbolicLinks()); + + assertThat(actionFs.getPath(getOutputPath("a/abssym/bfile")).resolveSymbolicLinks()) + .isEqualTo(actionFs.getPath(getOutputPath("b/bsub/bfile"))); + + assertThrows( + FileNotFoundException.class, + () -> actionFs.getPath(getOutputPath("b/bsub/nofile")).resolveSymbolicLinks()); + } + @Test public void renameTo_onlyLocalFile_renameLocalFile() throws Exception { FileSystem actionFs = createActionFileSystem(); From 282fffaafa019c17fa81d8f9a2c63207a76f95cc Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 1 Aug 2023 05:38:19 -0700 Subject: [PATCH 06/17] [Skymeld] Log an unexpected inconsistency instead of crashing in `SkymeldInconsistencyReceiver`. ... and allow the build to continue. In the recent past, the crashes here were a little excessive as the solution were always to add more cases into the allowlist. This provides a less disruptive way to discover the not-yet-expected cases. PiperOrigin-RevId: 552774378 Change-Id: I588c2bdad5a37d01bd3716d7a22ee5a9d029f7fd --- .../com/google/devtools/build/lib/skyframe/BUILD | 1 + .../skyframe/SkymeldInconsistencyReceiver.java | 16 ++++++++-------- 2 files changed, 9 insertions(+), 8 deletions(-) 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 cb4d9c7abab3fc..64b14d683244ea 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -3018,6 +3018,7 @@ java_library( deps = [ ":node_dropping_inconsistency_receiver", ":sky_functions", + "//src/main/java/com/google/devtools/build/lib/bugreport", "//src/main/java/com/google/devtools/build/skyframe", "//src/main/java/com/google/devtools/build/skyframe:graph_inconsistency_java_proto", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldInconsistencyReceiver.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldInconsistencyReceiver.java index 70ca31d2e414c3..c25984e98f064f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldInconsistencyReceiver.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldInconsistencyReceiver.java @@ -13,9 +13,9 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; -import static com.google.common.base.Preconditions.checkState; import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.bugreport.BugReport; import com.google.devtools.build.skyframe.GraphInconsistencyReceiver; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; @@ -51,12 +51,12 @@ public void noteInconsistencyAndMaybeThrow( return; } - checkState( - NodeDroppingInconsistencyReceiver.isExpectedInconsistency( - key, otherKeys, inconsistency, SKYMELD_EXPECTED_MISSING_CHILDREN), - "Unexpected inconsistency: %s, %s, %s", - key, - otherKeys, - inconsistency); + if (!NodeDroppingInconsistencyReceiver.isExpectedInconsistency( + key, otherKeys, inconsistency, SKYMELD_EXPECTED_MISSING_CHILDREN)) { + // Instead of crashing, simply send a bug report here so we can evaluate whether this is an + // actual bug or just something else to be added to the expected list. + BugReport.logUnexpected( + "Unexpected inconsistency: %s, %s, %s", key, otherKeys, inconsistency); + } } } From b20085a4f40b4061e6c0143834c16846c55dd7e7 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 1 Aug 2023 06:02:58 -0700 Subject: [PATCH 07/17] integration_test_setup.sh: load bash runfiles library Instead of getting the rlocation function from [test-setup.sh](https://cs.opensource.google/bazel/bazel/+/master:tools/test/test-setup.sh;l=115), we should use the actual bash runfiles library which supports repo mapping. Working towards: https://github.com/bazelbuild/bazel/issues/18957 RELNOTES: None PiperOrigin-RevId: 552778831 Change-Id: Ie747faf58a5c3fc33d38653130d9657224909430 --- src/test/shell/BUILD | 5 ++++- src/test/shell/bazel/BUILD | 13 +------------ src/test/shell/bazel/bazel_proto_library_test.sh | 2 -- src/test/shell/integration_test_setup.sh | 15 ++++++++++++++- 4 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/test/shell/BUILD b/src/test/shell/BUILD index 85de0e15be0842..780915b2416ff8 100644 --- a/src/test/shell/BUILD +++ b/src/test/shell/BUILD @@ -45,7 +45,10 @@ sh_library( "unittest.bash", "unittest_utils.sh", ], - data = [":testenv.sh"], + data = [ + ":testenv.sh", + "@bazel_tools//tools/bash/runfiles", + ], visibility = ["//visibility:public"], ) diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD index 670a46d64af614..b1c7b338ad0773 100644 --- a/src/test/shell/bazel/BUILD +++ b/src/test/shell/bazel/BUILD @@ -22,14 +22,6 @@ gen_workspace_stanza( ], ) -gen_workspace_stanza( - name = "rules_proto_stanza", - out = "rules_proto_stanza.txt", - repos = [ - "rules_proto", - ], -) - filegroup( name = "test-deps", testonly = 1, @@ -346,10 +338,7 @@ sh_test( name = "bazel_proto_library_test", size = "large", # Downloads and compiles protobuf for *every* *test* *case* srcs = ["bazel_proto_library_test.sh"], - data = [ - ":rules_proto_stanza.txt", - ":test-deps", - ], + data = [":test-deps"], exec_compatible_with = ["//:highcpu_machine"], tags = ["no_windows"], # Doesn't work on Windows for unknown reason ) diff --git a/src/test/shell/bazel/bazel_proto_library_test.sh b/src/test/shell/bazel/bazel_proto_library_test.sh index 015ed45e691a58..b6ab60089838e7 100755 --- a/src/test/shell/bazel/bazel_proto_library_test.sh +++ b/src/test/shell/bazel/bazel_proto_library_test.sh @@ -48,8 +48,6 @@ function write_workspace() { mkdir -p "$workspace" fi - cat $(rlocation io_bazel/src/tests/shell/bazel/rules_proto_stanza.txt) >> "$workspace"WORKSPACE - cat >> "$workspace"WORKSPACE << EOF load("@rules_proto//proto:repositories.bzl", "rules_proto_dependencies", "rules_proto_toolchains") rules_proto_dependencies() diff --git a/src/test/shell/integration_test_setup.sh b/src/test/shell/integration_test_setup.sh index bbf862e837bdea..17e3cea4bc1f5d 100755 --- a/src/test/shell/integration_test_setup.sh +++ b/src/test/shell/integration_test_setup.sh @@ -20,7 +20,20 @@ function print_message_and_exit() { } if type rlocation >&/dev/null; then - # If rlocation is defined, use it to look up data-dependencies. + # An incomplete rlocation function is defined in Bazel's test-setup.sh script, + # load the actual Bash runfiles library from @bazel_tools//tools/bash/runfiles + # to make sure repo mappings is respected. + # --- begin runfiles.bash initialization v3 --- + # Copy-pasted from the Bazel Bash runfiles library v3. + set -uo pipefail; set +e; f=bazel_tools/tools/bash/runfiles/runfiles.bash + source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \ + source "$(grep -sm1 "^$f " "${RUNFILES_MANIFEST_FILE:-/dev/null}" | cut -f2- -d' ')" 2>/dev/null || \ + source "$0.runfiles/$f" 2>/dev/null || \ + source "$(grep -sm1 "^$f " "$0.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \ + source "$(grep -sm1 "^$f " "$0.exe.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \ + { echo>&2 "ERROR: cannot find $f"; exit 1; }; f=; set -e + # --- end runfiles.bash initialization v3 --- + # Load the unit test framework source "$(rlocation io_bazel/src/test/shell/unittest.bash)" \ || print_message_and_exit "unittest.bash not found!" From 36a3d8f75b9ca512910eb55692ceaf8b076d21eb Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 1 Aug 2023 06:25:13 -0700 Subject: [PATCH 08/17] Remove `--incompatible_remove_exec_tools`. It does not appear to usefully detect projects which use `genrule.exec_tools`. Part of #19132. PiperOrigin-RevId: 552783498 Change-Id: I88806b024c58f85ae0de284dd0ff513fe4cdfe1e --- .../devtools/build/lib/bazel/BazelConfiguration.java | 10 ---------- .../devtools/build/lib/bazel/rules/genrule/BUILD | 1 - .../build/lib/bazel/rules/genrule/BazelGenRule.java | 12 ------------ 3 files changed, 23 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelConfiguration.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelConfiguration.java index 8f0a8ddac750fa..bb04d748267163 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BazelConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelConfiguration.java @@ -40,20 +40,10 @@ public static class Options extends FragmentOptions { help = "If enabled, visibility checking also applies to toolchain implementations.") public boolean checkVisibilityForToolchains; - @Option( - name = "incompatible_remove_exec_tools", - defaultValue = "false", - documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, - effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS}, - metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, - help = "If enabled, use of genrule's exec_tools attribute will cause an error..") - public boolean removeExecTools; - @Override public FragmentOptions getExec() { Options exec = (Options) getDefault(); exec.checkVisibilityForToolchains = checkVisibilityForToolchains; - exec.removeExecTools = removeExecTools; return exec; } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD index c516cdc1984b57..2b791a509d5363 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD @@ -18,7 +18,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory", "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment", - "//src/main/java/com/google/devtools/build/lib/bazel:bazel_configuration", "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/rules/genrule", "//src/main/java/com/google/devtools/build/lib/util:filetype", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java index 5deff13fd3edc3..70557d2f49eacc 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java @@ -16,8 +16,6 @@ import com.google.devtools.build.lib.analysis.CommandHelper; import com.google.devtools.build.lib.analysis.RuleContext; -import com.google.devtools.build.lib.bazel.BazelConfiguration; -import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.Type; import com.google.devtools.build.lib.rules.genrule.GenRuleBase; @@ -38,16 +36,6 @@ protected boolean isStampingEnabled(RuleContext ruleContext) { // projects are migrated. @Override protected CommandHelper.Builder commandHelperBuilder(RuleContext ruleContext) { - BazelConfiguration.Options bazelOptions = - ruleContext.getConfiguration().getOptions().get(BazelConfiguration.Options.class); - - if (bazelOptions.removeExecTools - && ruleContext.attributes().has("exec_tools", BuildType.LABEL_LIST) - && !ruleContext.attributes().get("exec_tools", BuildType.LABEL_LIST).isEmpty()) { - ruleContext.attributeError( - "exec_tools", "genrule.exec_tools has been removed, use tools instead"); - } - return CommandHelper.builder(ruleContext) .addToolDependencies("tools") .addToolDependencies("exec_tools") From a402c24b7b806e6c97f675cbd5517e65cda005b9 Mon Sep 17 00:00:00 2001 From: Maxim Matyunin Date: Tue, 1 Aug 2023 08:01:41 -0700 Subject: [PATCH 09/17] docs: fix typo in starlark variable name Closes #18953. PiperOrigin-RevId: 552803590 Change-Id: I105677f2a2a9d4d37c04a0be3c0a5ca6438d1ecd --- site/en/extending/config.md | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/site/en/extending/config.md b/site/en/extending/config.md index 1b4dbb312b2c9e..3dffb103ac528e 100644 --- a/site/en/extending/config.md +++ b/site/en/extending/config.md @@ -3,6 +3,8 @@ Book: /_book.yaml # Configurations + + {% include "_buttons.html" %} This page covers the benefits and basic usage of Starlark configurations, @@ -688,7 +690,7 @@ def _rule_impl(ctx): transitioned_dep = ctx.attr.dep[0] # Note: Access doesn't change, other_deps was already a list - for other dep in ctx.attr.other_deps: + for other_dep in ctx.attr.other_deps: # ... @@ -755,11 +757,11 @@ might create exponential growth of your build graph. **Figure 1.** Scalability graph showing a top level target and its dependencies. -This graph shows a top level target, //pkg:app, which depends on two targets, a -//pkg:1_0 and //pkg:1_1. Both these targets depend on two targets, //pkg:2_0 and -//pkg:2_1. Both these targets depend on two targets, //pkg:3_0 and //pkg:3_1. -This continues on until //pkg:n_0 and //pkg:n_1, which both depend on a single -target, //pkg:dep. +This graph shows a top level target, `//pkg:app`, which depends on two targets, a +`//pkg:1_0` and `//pkg:1_1`. Both these targets depend on two targets, `//pkg:2_0` and +`//pkg:2_1`. Both these targets depend on two targets, `//pkg:3_0` and `//pkg:3_1`. +This continues on until `//pkg:n_0` and `//pkg:n_1`, which both depend on a single +target, `//pkg:dep`. Building `//pkg:app` requires \\(2n+2\\) targets: @@ -767,7 +769,7 @@ Building `//pkg:app` requires \\(2n+2\\) targets: * `//pkg:dep` * `//pkg:i_0` and `//pkg:i_1` for \\(i\\) in \\([1..n]\\) -Imagine you [implement](#user-defined-build-settings)) a flag +Imagine you [implement](#user-defined-build-settings) a flag `--//foo:owner=` and `//pkg:i_b` applies depConfig = myConfig + depConfig.owner="$(myConfig.owner)$(b)" From a3ba48e7bf87c72bddb29b42b7825e746ec00f94 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 1 Aug 2023 08:27:08 -0700 Subject: [PATCH 10/17] Delete `RetainedHeapLimiter`. It has been superseded by `GcThrashingDetector`, which is simpler and has proven to be more effective. Rename the flags without the `experimental_` prefix, keeping the old names to aid in migration. A default value for `--gc_thrashing_limits` is applied in order to keep the existing behavior that specifying a value < 100 for the threshold activates detection. PiperOrigin-RevId: 552809809 Change-Id: Iee53eaa28632da0750ccc09e815eebc479d13890 --- .../lib/runtime/CommonCommandOptions.java | 8 +- .../lib/runtime/GcThrashingDetector.java | 4 +- .../lib/runtime/MemoryPressureListener.java | 15 +- .../lib/runtime/MemoryPressureModule.java | 13 +- .../lib/runtime/MemoryPressureOptions.java | 50 +-- .../lib/runtime/RetainedHeapLimiter.java | 183 --------- src/main/protobuf/memory_pressure.proto | 11 +- .../BazelBuildEventServiceModuleTest.java | 2 +- .../runtime/MemoryPressureListenerTest.java | 55 +-- .../lib/runtime/RetainedHeapLimiterTest.java | 346 ------------------ 10 files changed, 54 insertions(+), 633 deletions(-) delete mode 100644 src/main/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiter.java delete mode 100644 src/test/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiterTest.java diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java index 44129690677141..9657df3e263d68 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java @@ -360,18 +360,16 @@ public String getTypeDescription() { + " by 4 GCs with zero second pause") public MemoryProfileStableHeapParameters memoryProfileStableHeapParameters; - @Option( name = "heap_dump_on_oom", defaultValue = "false", documentationCategory = OptionDocumentationCategory.LOGGING, effectTags = {OptionEffectTag.BAZEL_MONITORING}, help = - "Whether to manually output a heap dump if an OOM is thrown (including OOMs due to" - + " --experimental_oom_more_eagerly_threshold). The dump will be written to" + "Whether to manually output a heap dump if an OOM is thrown (including manual OOMs due to" + + " reaching --gc_thrashing_limits). The dump will be written to" + " /.heapdump.hprof. This option effectively replaces" - + " -XX:+HeapDumpOnOutOfMemoryError, which has no effect because OOMs are caught and" - + " redirected to Runtime#halt.") + + " -XX:+HeapDumpOnOutOfMemoryError, which has no effect for manual OOMs.") public boolean heapDumpOnOom; @Option( diff --git a/src/main/java/com/google/devtools/build/lib/runtime/GcThrashingDetector.java b/src/main/java/com/google/devtools/build/lib/runtime/GcThrashingDetector.java index 5c6483922fc7f9..68d69890615462 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/GcThrashingDetector.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/GcThrashingDetector.java @@ -66,12 +66,12 @@ static Limit of(Duration period, int count) { /** If enabled in {@link MemoryPressureOptions}, creates a {@link GcThrashingDetector}. */ @Nullable static GcThrashingDetector createForCommand(MemoryPressureOptions options) { - if (options.gcThrashingLimits.isEmpty() || options.oomMoreEagerlyThreshold == 100) { + if (options.gcThrashingLimits.isEmpty() || options.gcThrashingThreshold == 100) { return null; } return new GcThrashingDetector( - options.oomMoreEagerlyThreshold, + options.gcThrashingThreshold, options.gcThrashingLimits, BlazeClock.instance(), BugReporter.defaultInstance()); diff --git a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureListener.java b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureListener.java index b85eeff7bad084..500397882b92aa 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureListener.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureListener.java @@ -42,19 +42,16 @@ final class MemoryPressureListener implements NotificationListener { private final AtomicReference eventBus = new AtomicReference<>(); - private final RetainedHeapLimiter retainedHeapLimiter; private final AtomicReference gcThrashingDetector = new AtomicReference<>(); private final Executor executor; - private MemoryPressureListener(RetainedHeapLimiter retainedHeapLimiter, Executor executor) { - this.retainedHeapLimiter = retainedHeapLimiter; + private MemoryPressureListener(Executor executor) { this.executor = executor; } - static MemoryPressureListener create(RetainedHeapLimiter retainedHeapLimiter) { + static MemoryPressureListener create() { return createFromBeans( ManagementFactory.getGarbageCollectorMXBeans(), - retainedHeapLimiter, // Use a dedicated thread to broadcast memory pressure events. The service thread that calls // handleNotification for GC events is not a typical Java thread - it doesn't work with // debugger breakpoints and may not show up in thread dumps. @@ -65,7 +62,6 @@ static MemoryPressureListener create(RetainedHeapLimiter retainedHeapLimiter) { @VisibleForTesting static MemoryPressureListener createFromBeans( List gcBeans, - RetainedHeapLimiter retainedHeapLimiter, Executor executor) { ImmutableList tenuredGcEmitters = findTenuredCollectorBeans(gcBeans); if (tenuredGcEmitters.isEmpty()) { @@ -79,8 +75,7 @@ static MemoryPressureListener createFromBeans( "Unable to find tenured collector from %s: names were %s.", gcBeans, names)); } - MemoryPressureListener memoryPressureListener = - new MemoryPressureListener(retainedHeapLimiter, executor); + MemoryPressureListener memoryPressureListener = new MemoryPressureListener(executor); tenuredGcEmitters.forEach(e -> e.addNotificationListener(memoryPressureListener, null, null)); return memoryPressureListener; } @@ -150,14 +145,10 @@ private void broadcast(MemoryPressureEvent event) { } // A null EventBus implies memory pressure event between commands with no active EventBus. - // In such cases, notify RetainedHeapLimiter but do not publish event. EventBus eventBus = this.eventBus.get(); if (eventBus != null) { eventBus.post(event); } - // Post to EventBus first so memory pressure subscribers have a chance to make things - // eligible for GC before RetainedHeapLimiter would trigger a full GC. - this.retainedHeapLimiter.handle(event); } void setEventBus(@Nullable EventBus eventBus) { diff --git a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureModule.java b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureModule.java index 0ba6a8bd0f3aa7..d1850ca6173b9f 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureModule.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureModule.java @@ -17,7 +17,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.eventbus.EventBus; import com.google.common.eventbus.Subscribe; -import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.runtime.MemoryPressure.MemoryPressureStats; import com.google.devtools.build.lib.skyframe.HighWaterMarkLimiter; import com.google.devtools.common.options.OptionsBase; @@ -28,18 +27,10 @@ * pressure events. */ public final class MemoryPressureModule extends BlazeModule { - private RetainedHeapLimiter retainedHeapLimiter; - private MemoryPressureListener memoryPressureListener; + private final MemoryPressureListener memoryPressureListener = MemoryPressureListener.create(); private HighWaterMarkLimiter highWaterMarkLimiter; private EventBus eventBus; - @Override - public void workspaceInit( - BlazeRuntime runtime, BlazeDirectories directories, WorkspaceBuilder builder) { - retainedHeapLimiter = RetainedHeapLimiter.create(runtime.getBugReporter()); - memoryPressureListener = MemoryPressureListener.create(retainedHeapLimiter); - } - @Override public ImmutableList> getCommandOptions(Command command) { return ImmutableList.of(MemoryPressureOptions.class); @@ -54,7 +45,6 @@ public void beforeCommand(CommandEnvironment env) { highWaterMarkLimiter = new HighWaterMarkLimiter(env.getSkyframeExecutor(), env.getSyscallCache(), options); memoryPressureListener.setGcThrashingDetector(GcThrashingDetector.createForCommand(options)); - retainedHeapLimiter.setOptions(options); eventBus.register(this); eventBus.register(highWaterMarkLimiter); @@ -77,7 +67,6 @@ void onCrash(@SuppressWarnings("unused") CrashEvent event) { private void postStats() { MemoryPressureStats.Builder stats = MemoryPressureStats.newBuilder(); - retainedHeapLimiter.addStatsAndReset(stats); highWaterMarkLimiter.addStatsAndReset(stats); eventBus.post(stats.build()); } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureOptions.java index aa7f138a8b9f29..d9222f746e67aa 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureOptions.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureOptions.java @@ -29,25 +29,6 @@ /** Options for responding to memory pressure. */ public final class MemoryPressureOptions extends OptionsBase { - @Option( - name = "experimental_oom_more_eagerly_threshold", - defaultValue = "100", - documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, - effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS}, - converter = PercentageConverter.class, - help = - "If this flag is set to a value less than 100, Bazel will OOM if, after two full GC's, " - + "more than this percentage of the (old gen) heap is still occupied.") - public int oomMoreEagerlyThreshold; - - @Option( - name = "min_time_between_triggered_gc", - defaultValue = "1m", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION}, - help = "The minimum amount of time between GCs triggered by RetainedHeapLimiter.") - public Duration minTimeBetweenTriggeredGc; - @Option( name = "skyframe_high_water_mark_threshold", defaultValue = "85", @@ -97,30 +78,35 @@ public final class MemoryPressureOptions extends OptionsBase { public int skyframeHighWaterMarkFullGcDropsPerInvocation; @Option( - name = "experimental_gc_thrashing_limits", - defaultValue = "", + name = "gc_thrashing_limits", + oldName = "experimental_gc_thrashing_limits", + oldNameWarning = false, + defaultValue = "1s:2,20s:3,1m:5", documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION, effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS}, converter = GcThrashingLimitsConverter.class, help = "Limits which, if reached, cause GcThrashingDetector to crash Bazel with an OOM. Each" + " limit is specified as : where period is a duration and count is a" - + " positive integer. If more than --experimental_oom_more_eagerly_threshold percent" - + " of tenured space (old gen heap) remains occupied after consecutive full" - + " GCs within , an OOM is triggered. Multiple limits can be specified" - + " separated by commas.") + + " positive integer. If more than --gc_thrashing_threshold percent of tenured space" + + " (old gen heap) remains occupied after consecutive full GCs within" + + " , an OOM is triggered. Multiple limits can be specified separated by" + + " commas.") public ImmutableList gcThrashingLimits; @Option( - name = "gc_thrashing_limits_retained_heap_limiter_mutually_exclusive", - defaultValue = "true", - documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION, + name = "gc_thrashing_threshold", + oldName = "experimental_oom_more_eagerly_threshold", + oldNameWarning = false, + defaultValue = "100", + documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS}, + converter = PercentageConverter.class, help = - "If true, specifying non-empty --experimental_gc_thrashing_limits deactivates" - + " RetainedHeapLimiter to make it mutually exclusive with GcThrashingDetector." - + " Setting to false permits both to be active for the same command.") - public boolean gcThrashingLimitsRetainedHeapLimiterMutuallyExclusive; + "The percent of tenured space occupied (0-100) above which GcThrashingDetector considers" + + " memory pressure events against its limits (--gc_thrashing_limits). If set to 100," + + " GcThrashingDetector is disabled.") + public int gcThrashingThreshold; static final class NonNegativeIntegerConverter extends RangeConverter { NonNegativeIntegerConverter() { diff --git a/src/main/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiter.java b/src/main/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiter.java deleted file mode 100644 index a59b54e5fb02ea..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiter.java +++ /dev/null @@ -1,183 +0,0 @@ -// Copyright 2016 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.runtime; - -import static com.google.common.base.Preconditions.checkNotNull; - -import com.google.common.annotations.VisibleForTesting; -import com.google.common.flogger.GoogleLogger; -import com.google.devtools.build.lib.bugreport.BugReporter; -import com.google.devtools.build.lib.bugreport.Crash; -import com.google.devtools.build.lib.bugreport.CrashContext; -import com.google.devtools.build.lib.clock.BlazeClock; -import com.google.devtools.build.lib.clock.Clock; -import com.google.devtools.build.lib.concurrent.ThreadSafety; -import com.google.devtools.build.lib.runtime.MemoryPressure.MemoryPressureStats; -import com.google.devtools.common.options.Options; -import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.atomic.AtomicLong; - -/** - * Monitors the size of the retained heap and exit promptly if it grows too large. - * - *

Specifically, checks the size of the tenured space after each major GC; if it exceeds {@link - * MemoryPressureOptions#oomMoreEagerlyThreshold}%, call {@link System#gc()} to trigger a - * stop-the-world collection; if it's still more than {@link - * MemoryPressureOptions#oomMoreEagerlyThreshold}% full, exit with an {@link OutOfMemoryError}. - */ -final class RetainedHeapLimiter implements MemoryPressureStatCollector { - - private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); - - private final BugReporter bugReporter; - private final Clock clock; - - private volatile MemoryPressureOptions options = inactiveOptions(); - - private final AtomicBoolean throwingOom = new AtomicBoolean(false); - private final AtomicBoolean heapLimiterTriggeredGc = new AtomicBoolean(false); - private final AtomicInteger consecutiveIgnoredFullGcsOverThreshold = new AtomicInteger(0); - private final AtomicBoolean loggedIgnoreWarningSinceLastGc = new AtomicBoolean(false); - private final AtomicLong lastTriggeredGcMillis = new AtomicLong(); - private final AtomicInteger gcsTriggered = new AtomicInteger(0); - private final AtomicInteger maxConsecutiveIgnoredFullGcsOverThreshold = new AtomicInteger(0); - - static RetainedHeapLimiter create(BugReporter bugReporter) { - return new RetainedHeapLimiter(bugReporter, BlazeClock.instance()); - } - - @VisibleForTesting - static RetainedHeapLimiter createForTest(BugReporter bugReporter, Clock clock) { - return new RetainedHeapLimiter(bugReporter, clock); - } - - private RetainedHeapLimiter(BugReporter bugReporter, Clock clock) { - this.bugReporter = checkNotNull(bugReporter); - this.clock = checkNotNull(clock); - } - - @ThreadSafety.ThreadCompatible // Can only be called on the logical main Bazel thread. - void setOptions(MemoryPressureOptions options) { - if (options.gcThrashingLimitsRetainedHeapLimiterMutuallyExclusive - && !options.gcThrashingLimits.isEmpty()) { - this.options = inactiveOptions(); - } else { - this.options = options; - } - } - - // Can be called concurrently, handles concurrent calls with #setThreshold gracefully. - @ThreadSafety.ThreadSafe - public void handle(MemoryPressureEvent event) { - if (throwingOom.get()) { - return; // Do nothing if a crash is already in progress. - } - - boolean wasHeapLimiterTriggeredGc = false; - boolean wasGcLockerDeferredHeapLimiterTriggeredGc = false; - if (event.wasManualGc()) { - wasHeapLimiterTriggeredGc = heapLimiterTriggeredGc.getAndSet(false); - if (!wasHeapLimiterTriggeredGc) { - // This was a manually triggered GC, but not from us earlier: short-circuit. - logger.atInfo().log("Ignoring manual GC from other source"); - return; - } - } else if (event.wasGcLockerInitiatedGc() && heapLimiterTriggeredGc.getAndSet(false)) { - // If System.gc() is called was while there are JNI thread(s) in the critical region, GCLocker - // defers the GC until those threads exit the critical region. However, all GCLocker initiated - // GCs are minor evacuation pauses, so we won't get the full GC we requested. Cancel the - // timeout so we can attempt System.gc() again if we're still over the threshold. See full - // explanation in b/263405096#comment14. - logger.atWarning().log( - "Observed a GCLocker initiated GC without observing a manual GC since the last call to" - + " System.gc(), cancelling timeout to permit a retry"); - wasGcLockerDeferredHeapLimiterTriggeredGc = true; - lastTriggeredGcMillis.set(0); - } - - // Get a local reference to guard against concurrent modifications. - MemoryPressureOptions options = this.options; - int threshold = options.oomMoreEagerlyThreshold; - - if (threshold == 100) { - return; // Inactive. - } - - int actual = event.percentTenuredSpaceUsed(); - if (actual < threshold) { - if (wasHeapLimiterTriggeredGc || wasGcLockerDeferredHeapLimiterTriggeredGc) { - logger.atInfo().log("Back under threshold (%s%% of tenured space)", actual); - } - consecutiveIgnoredFullGcsOverThreshold.set(0); - return; - } - - if (wasHeapLimiterTriggeredGc) { - if (!throwingOom.getAndSet(true)) { - // We got here from a GC initiated by the other branch. - OutOfMemoryError oom = - new OutOfMemoryError( - String.format( - "RetainedHeapLimiter forcing exit due to GC thrashing: After back-to-back full" - + " GCs, the tenured space is more than %s%% occupied (%s out of a tenured" - + " space size of %s).", - threshold, event.tenuredSpaceUsedBytes(), event.tenuredSpaceMaxBytes())); - logger.atInfo().log("Calling handleCrash"); - // Exits the runtime. - bugReporter.handleCrash(Crash.from(oom), CrashContext.halt()); - } - } else if (clock.currentTimeMillis() - lastTriggeredGcMillis.get() - > options.minTimeBetweenTriggeredGc.toMillis()) { - logger.atInfo().log( - "Triggering a full GC (%s%% of tenured space after %s GC)", - actual, event.wasFullGc() ? "full" : "minor"); - heapLimiterTriggeredGc.set(true); - gcsTriggered.incrementAndGet(); - // Force a full stop-the-world GC and see if it can get us below the threshold. - System.gc(); - lastTriggeredGcMillis.set(clock.currentTimeMillis()); - consecutiveIgnoredFullGcsOverThreshold.set(0); - loggedIgnoreWarningSinceLastGc.set(false); - } else if (event.wasFullGc()) { - int consecutiveIgnored = consecutiveIgnoredFullGcsOverThreshold.incrementAndGet(); - maxConsecutiveIgnoredFullGcsOverThreshold.accumulateAndGet(consecutiveIgnored, Math::max); - logger.atWarning().log( - "Ignoring possible GC thrashing x%s (%s%% of tenured space after full GC) because of" - + " recently triggered GC", - consecutiveIgnored, actual); - } else if (!loggedIgnoreWarningSinceLastGc.getAndSet(true)) { - logger.atWarning().log( - "Ignoring possible GC thrashing (%s%% of tenured space after minor GC) because of" - + " recently triggered GC", - actual); - } - } - - @Override - public void addStatsAndReset(MemoryPressureStats.Builder stats) { - stats - .setManuallyTriggeredGcs(gcsTriggered.getAndSet(0)) - .setMaxConsecutiveIgnoredGcsOverThreshold( - maxConsecutiveIgnoredFullGcsOverThreshold.getAndSet(0)); - consecutiveIgnoredFullGcsOverThreshold.set(0); - } - - private static MemoryPressureOptions inactiveOptions() { - var options = Options.getDefaults(MemoryPressureOptions.class); - options.oomMoreEagerlyThreshold = 100; - return options; - } -} diff --git a/src/main/protobuf/memory_pressure.proto b/src/main/protobuf/memory_pressure.proto index 97a8d20669d36b..63ec9b72433383 100644 --- a/src/main/protobuf/memory_pressure.proto +++ b/src/main/protobuf/memory_pressure.proto @@ -20,11 +20,12 @@ option java_package = "com.google.devtools.build.lib.runtime"; // Statistics about memory pressure handling. message MemoryPressureStats { - // The number of calls to System.gc() made by RetainedHeapLimiter. - int32 manually_triggered_gcs = 1; - // The maximum number of consecutive full GC events over the threshold ignored - // by RetainedHeapLimiter because it recently triggered a GC. - int32 max_consecutive_ignored_gcs_over_threshold = 2; + // Deprecated: An artifact of RetainedHeapLimiter. GcThrashingDetector (its + // replacement) does not manually trigger GC. + int32 manually_triggered_gcs = 1 [deprecated = true]; + // Deprecated: An artifact of RetainedHeapLimiter. GcThrashingDetector (its + // replacement) does not ignore any non-manual full GC. + int32 max_consecutive_ignored_gcs_over_threshold = 2 [deprecated = true]; // Number of times HighWaterMarkLimiter dropped caches after minor GCs. int32 minor_gc_drops = 3; // Number of times HighWaterMarkLimiter dropped caches after full GCs. diff --git a/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java b/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java index c34ff7469994fc..dfea5f77af83bd 100644 --- a/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java @@ -729,7 +729,7 @@ public void oom_firstReportedViaHandleCrash() throws Exception { testOom( () -> { OutOfMemoryError oom = new OutOfMemoryError(); - // Simulates an OOM coming from RetainedHeapLimiter, which reports the error by calling + // Simulates an OOM coming from GcThrashingDetector, which reports the error by calling // handleCrash. Uses keepAlive() to avoid exiting the JVM and aborting the test, then // throw the original oom to ensure control flow terminates. BugReport.handleCrash(Crash.from(oom), CrashContext.keepAlive()); diff --git a/src/test/java/com/google/devtools/build/lib/runtime/MemoryPressureListenerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/MemoryPressureListenerTest.java index 43405372996db2..eb50b463ceacb5 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/MemoryPressureListenerTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/MemoryPressureListenerTest.java @@ -28,11 +28,12 @@ import com.google.common.eventbus.EventBus; import com.google.common.eventbus.Subscribe; import com.google.devtools.build.lib.bugreport.BugReporter; -import com.google.devtools.common.options.Options; +import com.google.devtools.build.lib.clock.BlazeClock; import com.sun.management.GarbageCollectionNotificationInfo; import com.sun.management.GcInfo; import java.lang.management.GarbageCollectorMXBean; import java.lang.management.MemoryUsage; +import java.time.Duration; import java.util.ArrayList; import java.util.List; import javax.management.Notification; @@ -52,7 +53,6 @@ private interface NotificationBean extends GarbageCollectorMXBean, NotificationE private final NotificationBean mockUselessBean = mock(NotificationBean.class); private final BugReporter bugReporter = mock(BugReporter.class); private final EventBus eventBus = new EventBus(); - private final RetainedHeapLimiter retainedHeapLimiter = RetainedHeapLimiter.create(bugReporter); private final List events = new ArrayList<>(); @Before @@ -87,14 +87,14 @@ public void createFromBeans_throwsIfNoTenuredSpaceBean() { IllegalStateException.class, () -> MemoryPressureListener.createFromBeans( - ImmutableList.of(mockUselessBean), retainedHeapLimiter, directExecutor())); + ImmutableList.of(mockUselessBean), directExecutor())); } @Test public void simple() { MemoryPressureListener underTest = MemoryPressureListener.createFromBeans( - ImmutableList.of(mockUselessBean, mockBean), retainedHeapLimiter, directExecutor()); + ImmutableList.of(mockUselessBean, mockBean), directExecutor()); underTest.setEventBus(eventBus); verify(mockBean).addNotificationListener(underTest, null, null); verify(mockUselessBean, never()).addNotificationListener(any(), any(), any()); @@ -134,7 +134,7 @@ public void simple() { public void nullEventBus_doNotPublishEvent() { MemoryPressureListener underTest = MemoryPressureListener.createFromBeans( - ImmutableList.of(mockUselessBean, mockBean), retainedHeapLimiter, directExecutor()); + ImmutableList.of(mockUselessBean, mockBean), directExecutor()); verify(mockBean).addNotificationListener(underTest, null, null); verify(mockUselessBean, never()).addNotificationListener(any(), any(), any()); @@ -166,8 +166,7 @@ public void nullEventBus_doNotPublishEvent() { @Test public void manualGc() { MemoryPressureListener underTest = - MemoryPressureListener.createFromBeans( - ImmutableList.of(mockBean), retainedHeapLimiter, directExecutor()); + MemoryPressureListener.createFromBeans(ImmutableList.of(mockBean), directExecutor()); underTest.setEventBus(eventBus); verify(mockBean).addNotificationListener(underTest, null, null); @@ -198,8 +197,7 @@ public void manualGc() { @Test public void doesntInvokeHandlerWhenTenuredSpaceMaxSizeIsZero() { MemoryPressureListener underTest = - MemoryPressureListener.createFromBeans( - ImmutableList.of(mockBean), retainedHeapLimiter, directExecutor()); + MemoryPressureListener.createFromBeans(ImmutableList.of(mockBean), directExecutor()); underTest.setEventBus(eventBus); verify(mockBean).addNotificationListener(underTest, null, null); @@ -229,7 +227,7 @@ public void findsTenuredSpaceWithNonZeroMaxSize() { MemoryPressureListener underTest = MemoryPressureListener.createFromBeans( - ImmutableList.of(mockBean, anotherMockBean), retainedHeapLimiter, directExecutor()); + ImmutableList.of(mockBean, anotherMockBean), directExecutor()); underTest.setEventBus(eventBus); verify(mockBean).addNotificationListener(underTest, null, null); verify(anotherMockBean).addNotificationListener(underTest, null, null); @@ -267,18 +265,21 @@ public void findsTenuredSpaceWithNonZeroMaxSize() { } @Test - public void retainedHeapLimiter_aboveThreshold_handleCrash() { - MemoryPressureOptions options = Options.getDefaults(MemoryPressureOptions.class); - options.oomMoreEagerlyThreshold = 90; - retainedHeapLimiter.setOptions(options); - + public void gcThrashingDetector_limitMet_handleCrash() { MemoryPressureListener underTest = MemoryPressureListener.createFromBeans( - ImmutableList.of(mockUselessBean, mockBean), retainedHeapLimiter, directExecutor()); - underTest.setEventBus(eventBus); + ImmutableList.of(mockUselessBean, mockBean), directExecutor()); verify(mockBean).addNotificationListener(underTest, null, null); verify(mockUselessBean, never()).addNotificationListener(any(), any(), any()); + underTest.setEventBus(eventBus); + underTest.setGcThrashingDetector( + new GcThrashingDetector( + /* threshold= */ 90, + ImmutableList.of(GcThrashingDetector.Limit.of(Duration.ofMinutes(1), 1)), + BlazeClock.instance(), + bugReporter)); + GcInfo mockGcInfo = mock(GcInfo.class); String nonTenuredSpaceName = "nope"; MemoryUsage mockMemoryUsageForNonTenuredSpace = mock(MemoryUsage.class); @@ -296,6 +297,7 @@ public void retainedHeapLimiter_aboveThreshold_handleCrash() { MemoryPressureEvent event = MemoryPressureEvent.newBuilder() .setWasManualGc(false) + .setWasFullGc(true) .setTenuredSpaceUsedBytes(99) .setTenuredSpaceMaxBytes(100) .build(); @@ -303,27 +305,10 @@ public void retainedHeapLimiter_aboveThreshold_handleCrash() { new Notification( GarbageCollectionNotificationInfo.GARBAGE_COLLECTION_NOTIFICATION, "test", 123); notification.setUserData( - new GarbageCollectionNotificationInfo("gcName", "gcAction", "non-manual", mockGcInfo) + new GarbageCollectionNotificationInfo("gcName", "end of major GC", "non-manual", mockGcInfo) .toCompositeData(null)); underTest.handleNotification(notification, null); assertThat(events).containsExactly(event); - - MemoryPressureEvent manualGcEvent = - MemoryPressureEvent.newBuilder() - .setWasManualGc(true) - .setTenuredSpaceUsedBytes(99) - .setTenuredSpaceMaxBytes(100) - .build(); - Notification manualGCNotification = - new Notification( - GarbageCollectionNotificationInfo.GARBAGE_COLLECTION_NOTIFICATION, "test", 123); - manualGCNotification.setUserData( - new GarbageCollectionNotificationInfo("gcName", "gcAction", "System.gc()", mockGcInfo) - .toCompositeData(null)); - underTest.handleNotification(manualGCNotification, null); - - verify(bugReporter).handleCrash(any(), any()); - assertThat(events).containsExactly(event, manualGcEvent); } } diff --git a/src/test/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiterTest.java b/src/test/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiterTest.java deleted file mode 100644 index 632b309e778cc3..00000000000000 --- a/src/test/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiterTest.java +++ /dev/null @@ -1,346 +0,0 @@ -// Copyright 2019 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.runtime; - -import static com.google.common.base.Preconditions.checkArgument; -import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoInteractions; -import static org.mockito.Mockito.verifyNoMoreInteractions; - -import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.bugreport.BugReporter; -import com.google.devtools.build.lib.bugreport.Crash; -import com.google.devtools.build.lib.runtime.GcThrashingDetector.Limit; -import com.google.devtools.build.lib.runtime.MemoryPressure.MemoryPressureStats; -import com.google.devtools.build.lib.testutil.ManualClock; -import com.google.devtools.common.options.Options; -import com.google.testing.junit.testparameterinjector.TestParameterInjector; -import java.lang.ref.WeakReference; -import java.time.Duration; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.ArgumentCaptor; -import org.mockito.ArgumentMatchers; - -/** Tests for {@link RetainedHeapLimiter}. */ -@RunWith(TestParameterInjector.class) -public final class RetainedHeapLimiterTest { - - private final BugReporter bugReporter = mock(BugReporter.class); - private final ManualClock clock = new ManualClock(); - private final MemoryPressureOptions options = Options.getDefaults(MemoryPressureOptions.class); - - private final RetainedHeapLimiter underTest = - RetainedHeapLimiter.createForTest(bugReporter, clock); - - @Before - public void setClock() { - clock.advanceMillis(100000); - } - - @After - public void verifyNoMoreBugReports() { - verifyNoMoreInteractions(bugReporter); - } - - @Test - public void underThreshold_noOom() { - options.oomMoreEagerlyThreshold = 99; - underTest.setOptions(options); - - underTest.handle(percentUsedAfterOrganicFullGc(100)); - underTest.handle(percentUsedAfterManualGc(89)); - - verifyNoInteractions(bugReporter); - assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1)); - } - - @Test - public void overThreshold_oom() { - options.oomMoreEagerlyThreshold = 90; - underTest.setOptions(options); - - // Triggers GC, and tells RetainedHeapLimiter to OOM if too much memory used next time. - underTest.handle(percentUsedAfterOrganicFullGc(91)); - - underTest.handle(percentUsedAfterManualGc(91)); - - ArgumentCaptor crashArgument = ArgumentCaptor.forClass(Crash.class); - verify(bugReporter).handleCrash(crashArgument.capture(), ArgumentMatchers.any()); - OutOfMemoryError oom = (OutOfMemoryError) crashArgument.getValue().getThrowable(); - assertThat(oom).hasMessageThat().contains("forcing exit due to GC thrashing"); - assertThat(oom).hasMessageThat().contains("tenured space is more than 90% occupied"); - - assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1)); - } - - @Test - public void inactiveAfterOom() { - options.oomMoreEagerlyThreshold = 90; - options.minTimeBetweenTriggeredGc = Duration.ZERO; - underTest.setOptions(options); - - underTest.handle(percentUsedAfterOrganicFullGc(91)); - underTest.handle(percentUsedAfterManualGc(91)); - verify(bugReporter).handleCrash(any(), any()); - - // No more GC or bug reports even if notifications come in after an OOM is in progress. - WeakReference ref = new WeakReference<>(new Object()); - clock.advanceMillis(Duration.ofMinutes(1).toMillis()); - underTest.handle(percentUsedAfterOrganicFullGc(91)); - underTest.handle(percentUsedAfterManualGc(91)); - assertThat(ref.get()).isNotNull(); - verifyNoMoreBugReports(); - - assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1)); - } - - @Test - public void externalGcNoTrigger() { - options.oomMoreEagerlyThreshold = 90; - underTest.setOptions(options); - - // No trigger because cause was "System.gc()". - underTest.handle(percentUsedAfterManualGc(91)); - - // Proof: no OOM. - underTest.handle(percentUsedAfterManualGc(91)); - verifyNoInteractions(bugReporter); - - assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(0)); - } - - @Test - public void triggerReset() { - options.oomMoreEagerlyThreshold = 90; - underTest.setOptions(options); - - underTest.handle(percentUsedAfterOrganicFullGc(91)); - - // Got under the threshold, so no OOM. - underTest.handle(percentUsedAfterManualGc(89)); - - // No OOM this time since wasn't triggered. - underTest.handle(percentUsedAfterManualGc(91)); - verifyNoInteractions(bugReporter); - } - - @Test - public void triggerRaceWithOtherGc() { - options.oomMoreEagerlyThreshold = 90; - underTest.setOptions(options); - - underTest.handle(percentUsedAfterOrganicFullGc(91)); - underTest.handle(percentUsedAfterOrganicFullGc(91)); - underTest.handle(percentUsedAfterManualGc(91)); - - ArgumentCaptor crashArgument = ArgumentCaptor.forClass(Crash.class); - verify(bugReporter).handleCrash(crashArgument.capture(), ArgumentMatchers.any()); - assertThat(crashArgument.getValue().getThrowable()).isInstanceOf(OutOfMemoryError.class); - } - - @Test - public void minTimeBetweenGc_lessThan_noGc() { - options.oomMoreEagerlyThreshold = 90; - options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1); - underTest.setOptions(options); - WeakReference ref = new WeakReference<>(new Object()); - - underTest.handle(percentUsedAfterOrganicFullGc(91)); - assertThat(ref.get()).isNull(); - underTest.handle(percentUsedAfterManualGc(89)); - - ref = new WeakReference<>(new Object()); - clock.advanceMillis(Duration.ofSeconds(59).toMillis()); - underTest.handle(percentUsedAfterOrganicFullGc(91)); - assertThat(ref.get()).isNotNull(); - - assertStats( - MemoryPressureStats.newBuilder() - .setManuallyTriggeredGcs(1) - .setMaxConsecutiveIgnoredGcsOverThreshold(1)); - } - - @Test - public void minTimeBetweenGc_greaterThan_gc() { - options.oomMoreEagerlyThreshold = 90; - options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1); - underTest.setOptions(options); - WeakReference ref = new WeakReference<>(new Object()); - - underTest.handle(percentUsedAfterOrganicFullGc(91)); - assertThat(ref.get()).isNull(); - underTest.handle(percentUsedAfterManualGc(89)); - - ref = new WeakReference<>(new Object()); - clock.advanceMillis(Duration.ofSeconds(61).toMillis()); - underTest.handle(percentUsedAfterOrganicFullGc(91)); - assertThat(ref.get()).isNull(); - - assertStats( - MemoryPressureStats.newBuilder() - .setManuallyTriggeredGcs(2) - .setMaxConsecutiveIgnoredGcsOverThreshold(0)); - } - - @Test - public void gcLockerDefersManualGc_timeoutCancelled() { - options.oomMoreEagerlyThreshold = 90; - options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1); - underTest.setOptions(options); - - underTest.handle(percentUsedAfterOrganicFullGc(91)); - WeakReference ref = new WeakReference<>(new Object()); - underTest.handle(percentUsedAfterGcLockerGc(91)); - assertThat(ref.get()).isNull(); - - assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(2)); - } - - @Test - public void gcLockerAfterSuccessfulManualGc_timeoutPreserved() { - options.oomMoreEagerlyThreshold = 90; - options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1); - underTest.setOptions(options); - - underTest.handle(percentUsedAfterOrganicFullGc(91)); - underTest.handle(percentUsedAfterManualGc(89)); - WeakReference ref = new WeakReference<>(new Object()); - underTest.handle(percentUsedAfterGcLockerGc(91)); - assertThat(ref.get()).isNotNull(); - - assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1)); - } - - @Test - public void reportsMaxConsecutiveIgnored() { - options.oomMoreEagerlyThreshold = 90; - options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1); - underTest.setOptions(options); - - underTest.handle(percentUsedAfterOrganicFullGc(91)); - underTest.handle(percentUsedAfterManualGc(89)); - for (int i = 0; i < 6; i++) { - underTest.handle(percentUsedAfterOrganicFullGc(91)); - } - - clock.advanceMillis(Duration.ofMinutes(2).toMillis()); - - underTest.handle(percentUsedAfterOrganicFullGc(91)); - underTest.handle(percentUsedAfterManualGc(89)); - for (int i = 0; i < 8; i++) { - underTest.handle(percentUsedAfterOrganicFullGc(91)); - } - underTest.handle(percentUsedAfterOrganicFullGc(89)); // Breaks the streak of over threshold GCs. - underTest.handle(percentUsedAfterOrganicFullGc(91)); - - clock.advanceMillis(Duration.ofMinutes(2).toMillis()); - - underTest.handle(percentUsedAfterOrganicFullGc(91)); - underTest.handle(percentUsedAfterOrganicFullGc(89)); - for (int i = 0; i < 7; i++) { - underTest.handle(percentUsedAfterOrganicFullGc(91)); - } - - assertStats( - MemoryPressureStats.newBuilder() - .setManuallyTriggeredGcs(3) - .setMaxConsecutiveIgnoredGcsOverThreshold(8)); - } - - @Test - public void threshold100_noGcTriggeredEvenWithNonsenseStats() { - options.oomMoreEagerlyThreshold = 100; - underTest.setOptions(options); - WeakReference ref = new WeakReference<>(new Object()); - - underTest.handle(percentUsedAfterOrganicFullGc(101)); - assertThat(ref.get()).isNotNull(); - - assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(0)); - } - - @Test - public void optionsNotSet_disabled() { - underTest.handle(percentUsedAfterOrganicFullGc(99)); - assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(0)); - } - - @Test - public void gcThrashingLimitsSet_mutuallyExclusive_disabled() { - options.oomMoreEagerlyThreshold = 90; - options.gcThrashingLimits = ImmutableList.of(Limit.of(Duration.ofMinutes(1), 2)); - options.gcThrashingLimitsRetainedHeapLimiterMutuallyExclusive = true; - underTest.setOptions(options); - - underTest.handle(percentUsedAfterOrganicFullGc(99)); - - assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(0)); - } - - @Test - public void gcThrashingLimitsSet_mutuallyInclusive_enabled() { - options.oomMoreEagerlyThreshold = 90; - options.gcThrashingLimits = ImmutableList.of(Limit.of(Duration.ofMinutes(1), 2)); - options.gcThrashingLimitsRetainedHeapLimiterMutuallyExclusive = false; - underTest.setOptions(options); - - underTest.handle(percentUsedAfterOrganicFullGc(99)); - - assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1)); - } - - @Test - public void statsReset() { - options.oomMoreEagerlyThreshold = 90; - underTest.setOptions(options); - - underTest.handle(percentUsedAfterOrganicFullGc(91)); - - assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1)); - assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(0)); - } - - private static MemoryPressureEvent percentUsedAfterManualGc(int percentUsed) { - return percentUsedAfterGc(percentUsed).setWasManualGc(true).setWasFullGc(true).build(); - } - - private static MemoryPressureEvent percentUsedAfterOrganicFullGc(int percentUsed) { - return percentUsedAfterGc(percentUsed).setWasFullGc(true).build(); - } - - private static MemoryPressureEvent percentUsedAfterGcLockerGc(int percentUsed) { - return percentUsedAfterGc(percentUsed).setWasGcLockerInitiatedGc(true).build(); - } - - private static MemoryPressureEvent.Builder percentUsedAfterGc(int percentUsed) { - checkArgument(percentUsed >= 0, percentUsed); - return MemoryPressureEvent.newBuilder() - .setTenuredSpaceUsedBytes(percentUsed) - .setTenuredSpaceMaxBytes(100L); - } - - private void assertStats(MemoryPressureStats.Builder expected) { - MemoryPressureStats.Builder stats = MemoryPressureStats.newBuilder(); - underTest.addStatsAndReset(stats); - assertThat(stats.build()).isEqualTo(expected.build()); - } -} From f7b0c125099226ed846f077220dde2adc8f1e637 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 1 Aug 2023 08:28:42 -0700 Subject: [PATCH 11/17] Add path to patched file to invalid chunk error When a chunk in a patch fails to apply, it now also prints the path to the file that the patch is applied to. Closes #19131. PiperOrigin-RevId: 552810161 Change-Id: I66bac906133c3a16a3e63ab147c80e5eec6f6f11 --- .../devtools/build/lib/bazel/repository/PatchUtil.java | 8 +++++++- .../build/lib/bazel/repository/PatchUtilTest.java | 3 ++- .../starlark/StarlarkRepositoryContextTest.java | 4 ++-- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/PatchUtil.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/PatchUtil.java index 10eb28f2e55b81..956e465e531c7e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/PatchUtil.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/PatchUtil.java @@ -300,7 +300,13 @@ private static void applyPatchToFile( } } - List newContent = applyOffsetPatchTo(patch, oldContent); + List newContent; + try { + newContent = applyOffsetPatchTo(patch, oldContent); + } catch (PatchFailedException e) { + throw new PatchFailedException( + String.format("in patch applied to %s: %s", oldFile, e.getMessage())); + } // The file we should write newContent to. Path outputFile; diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/PatchUtilTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/PatchUtilTest.java index 763fe5541b6155..2d4a115df995fa 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/PatchUtilTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/PatchUtilTest.java @@ -512,7 +512,8 @@ public void testChunkDoesNotMatch() throws IOException { assertThat(expected) .hasMessageThat() .contains( - "Incorrect Chunk: the chunk content doesn't match the target\n" + "in patch applied to /root/foo.cc: Incorrect Chunk: the chunk content doesn't match " + + "the target\n" + "**Original Position**: 2\n" + "\n" + "**Original Content**:\n" 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 5de9acc764b3e1..2e57036c43036c 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 @@ -375,8 +375,8 @@ public void testPatchErrorWasThrown() throws Exception { .hasCauseThat() .hasMessageThat() .isEqualTo( - "Error applying patch /outputDir/my.patch: Incorrect Chunk: the chunk content " - + "doesn't match the target\n" + "Error applying patch /outputDir/my.patch: in patch applied to " + + "/outputDir/foo: Incorrect Chunk: the chunk content doesn't match the target\n" + "**Original Position**: 1\n" + "\n" + "**Original Content**:\n" From dd3ae79dafc298f214b7c2439562744e524e04e2 Mon Sep 17 00:00:00 2001 From: salma-samy Date: Tue, 1 Aug 2023 09:18:35 -0700 Subject: [PATCH 12/17] Re-run extension if its files change fixes https://github.com/bazelbuild/bazel/issues/19068 PiperOrigin-RevId: 552823534 Change-Id: I87256b2cf954b932e24c70e22386020599f21a6f --- .../lib/bazel/bzlmod/BazelLockFileValue.java | 17 +-- .../lib/bazel/bzlmod/GsonTypeAdapterUtil.java | 14 +++ .../bazel/bzlmod/LockFileModuleExtension.java | 6 +- .../bzlmod/SingleExtensionEvalFunction.java | 114 +++++++++++++++--- .../py/bazel/bzlmod/bazel_lockfile_test.py | 42 +++++++ 5 files changed, 170 insertions(+), 23 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java index 415c31cb62af2f..26c674fb6bc14f 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java @@ -109,25 +109,26 @@ public ImmutableList getModuleAndFlagsDiff( return moduleDiff.build(); } + /** Returns the differences between an extension and its locked data */ public ImmutableList getModuleExtensionDiff( - LockFileModuleExtension lockedExtension, - ImmutableMap lockedExtensionUsages, ModuleExtensionId extensionId, byte[] transitiveDigest, + boolean filesChanged, ImmutableMap envVariables, - ImmutableMap extensionUsages) { + ImmutableMap extensionUsages, + ImmutableMap lockedExtensionUsages) { + LockFileModuleExtension lockedExtension = getModuleExtensions().get(extensionId); + ImmutableList.Builder extDiff = new ImmutableList.Builder<>(); - if (lockedExtension == null) { - return extDiff - .add("The module extension '" + extensionId + "' does not exist in the lockfile") - .build(); - } if (!Arrays.equals(transitiveDigest, lockedExtension.getBzlTransitiveDigest())) { extDiff.add( "The implementation of the extension '" + extensionId + "' or one of its transitive .bzl files has changed"); } + if (filesChanged) { + extDiff.add("One or more files the extension '" + extensionId + "' is using have changed"); + } if (!extensionUsages.equals(lockedExtensionUsages)) { extDiff.add("The usages of the extension '" + extensionId + "' has changed"); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java index f0190f4c9fb66f..334484716f8bc2 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java @@ -91,6 +91,19 @@ public ModuleKey read(JsonReader jsonReader) throws IOException { } }; + public static final TypeAdapter

For whitespace trimming, we use the same definition of whitespace as the Starlark {@code + * string.strip} method. + * + *

Following PEP-257, we expand tabs in the doc string with tab size 8 before dedenting. + * Starlark does not use tabs for indentation, but Starlark string values may contain tabs, so we + * choose to expand them for consistency with Python. + * + *

The intent is to turn documentation strings like + * + * 

+   *     """Heading
+   *
+   *     Details paragraph
+   *     """
+   *
+ * + * and + * + * 
+   *     """
+   *     Heading
+   *
+   *     Details paragraph
+   *     """
+   *
+ * + * into the desired "Heading\n\nDetails paragraph" form, and avoid the risk of documentation + * processors interpreting indented parts of the original string as special formatting (e.g. code + * blocks in the case of Markdown). + */ + public static String trimDocString(String docString) { + ImmutableList lines = expandTabs(docString, 8).lines().collect(toImmutableList()); + if (lines.isEmpty()) { + return ""; + } + // First line is special: we fully strip it and ignore it for leading spaces calculation + String firstLineTrimmed = StringModule.INSTANCE.strip(lines.get(0), NONE); + Iterable subsequentLines = Iterables.skip(lines, 1); + int minLeadingSpaces = Integer.MAX_VALUE; + for (String line : subsequentLines) { + String strippedLeading = StringModule.INSTANCE.lstrip(line, NONE); + if (!strippedLeading.isEmpty()) { + int leadingSpaces = line.length() - strippedLeading.length(); + minLeadingSpaces = min(leadingSpaces, minLeadingSpaces); + } + } + if (minLeadingSpaces == Integer.MAX_VALUE) { + minLeadingSpaces = 0; + } + + StringBuilder result = new StringBuilder(); + result.append(firstLineTrimmed); + for (String line : subsequentLines) { + // Length check ensures we ignore leading empty lines + if (result.length() > 0) { + result.append("\n"); + } + if (line.length() > minLeadingSpaces) { + result.append(StringModule.INSTANCE.rstrip(line.substring(minLeadingSpaces), NONE)); + } + } + // Remove trailing empty lines + return StringModule.INSTANCE.rstrip(result.toString(), NONE); + } + + /** + * Expands tab characters to one or more spaces, producing the same indentation level at any given + * point on any given line as would be expected when rendering the string with a given tab size; a + * Java port of Python's {@code str.expandtabs}. + */ + static String expandTabs(String line, int tabSize) { + if (!line.contains("\t")) { + // Don't alloc in the fast case. + return line; + } + checkArgument(tabSize > 0); + StringBuilder result = new StringBuilder(); + int col = 0; + for (int i = 0; i < line.length(); i++) { + char c = line.charAt(i); + switch (c) { + case '\n': + case '\r': + result.append(c); + col = 0; + break; + case '\t': + int spaces = tabSize - col % tabSize; + for (int j = 0; j < spaces; j++) { + result.append(' '); + } + col += spaces; + break; + default: + result.append(c); + col++; + } + } + return result.toString(); + } + /** Returns a slice of a sequence as if by the Starlark operation {@code x[start:stop:step]}. */ public static Object slice( Mutability mu, Object x, Object startObj, Object stopObj, Object stepObj) @@ -972,7 +1083,10 @@ public static Object execFileProgram(Program prog, Module module, StarlarkThread int[] globalIndex = module.getIndicesOfGlobals(rfn.getGlobals()); if (module.getDocumentation() == null) { - module.setDocumentation(rfn.getDocumentation()); + String documentation = rfn.getDocumentation(); + if (documentation != null) { + module.setDocumentation(Starlark.trimDocString(documentation)); + } } StarlarkFunction toplevel = diff --git a/src/main/java/net/starlark/java/eval/StarlarkFunction.java b/src/main/java/net/starlark/java/eval/StarlarkFunction.java index 60f0af44390461..3136a3bc3e6de9 100644 --- a/src/main/java/net/starlark/java/eval/StarlarkFunction.java +++ b/src/main/java/net/starlark/java/eval/StarlarkFunction.java @@ -145,10 +145,14 @@ public String getName() { return rfn.getName(); } - /** Returns the value denoted by the function's doc string literal, or null if absent. */ + /** + * Returns the value denoted by the function's doc string literal (trimmed if necessary), or null + * if absent. + */ @Nullable public String getDocumentation() { - return rfn.getDocumentation(); + String documentation = rfn.getDocumentation(); + return documentation != null ? Starlark.trimDocString(documentation) : null; } public Module getModule() { diff --git a/src/main/java/net/starlark/java/eval/StringModule.java b/src/main/java/net/starlark/java/eval/StringModule.java index b4cdc137fd9a09..302dbc1312b50c 100644 --- a/src/main/java/net/starlark/java/eval/StringModule.java +++ b/src/main/java/net/starlark/java/eval/StringModule.java @@ -184,9 +184,11 @@ public String upper(String self) { *

Note that this differs from Python 2.7, which uses ctype.h#isspace(), and from * java.lang.Character#isWhitespace(), which does not recognize U+00A0. */ + // TODO(https://github.com/bazelbuild/starlark/issues/112): use the Unicode definition of + // whitespace, matching Python 3. private static final String LATIN1_WHITESPACE = ("\u0009" + "\n" + "\u000B" + "\u000C" + "\r" + "\u001C" + "\u001D" + "\u001E" + "\u001F" - + "\u0020" + "\u0085" + "\u00A0"); + + " " + "\u0085" + "\u00A0"); private static String stringLStrip(String self, String chars) { CharMatcher matcher = CharMatcher.anyOf(chars); diff --git a/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java b/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java index 607ee5c3db8fd0..4c54059625c77c 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java @@ -824,7 +824,10 @@ public void repositoryRule() throws Exception { "", "my_repo_rule = repository_rule(", " implementation = _impl,", - " doc = 'My repository rule',", + " doc = '''My repository rule", + "", + " With details", + " ''',", " attrs = {", " 'a': attr.string(doc = 'My doc', default = 'foo'),", " 'b': attr.string(mandatory = True),", @@ -855,7 +858,7 @@ public void repositoryRule() throws Exception { .setRuleName("foo.repo_rule") .setOriginKey( OriginKey.newBuilder().setName("my_repo_rule").setFile("//:dep.bzl").build()) - .setDocString("My repository rule") + .setDocString("My repository rule\n\nWith details") .addAllAttribute(ModuleInfoExtractor.IMPLICIT_REPOSITORY_RULE_ATTRIBUTES) .addAttribute( AttributeInfo.newBuilder() @@ -878,7 +881,9 @@ public void moduleExtension() throws Exception { scratch.file( "dep.bzl", "_install = tag_class(", - " doc = 'Install',", + " doc = '''Install", + " ", + " With details''',", " attrs = {", " 'artifacts': attr.string_list(doc = 'Artifacts'),", " '_hidden': attr.bool(),", @@ -896,7 +901,9 @@ public void moduleExtension() throws Exception { " pass", "", "my_ext = module_extension(", - " doc = 'My extension',", + " doc = '''My extension", + "", + " With details''',", " tag_classes = {", " 'install': _install,", " 'artifact': _artifact,", @@ -925,12 +932,12 @@ public void moduleExtension() throws Exception { .containsExactly( ModuleExtensionInfo.newBuilder() .setExtensionName("foo.ext") - .setDocString("My extension") + .setDocString("My extension\n\nWith details") .setOriginKey(OriginKey.newBuilder().setFile("//:dep.bzl").build()) .addTagClass( ModuleExtensionTagClassInfo.newBuilder() .setTagName("install") - .setDocString("Install") + .setDocString("Install\n\nWith details") .addAttribute( AttributeInfo.newBuilder() .setName("artifacts") diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java index 67e17504c6c1b3..dbbfc96f4b5e73 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java @@ -870,6 +870,11 @@ public void testAttrDoc( Attribute documented = buildAttribute("documented", String.format("attr.%s(doc='foo')", attrType)); assertThat(documented.getDoc()).isEqualTo("foo"); + Attribute documentedNeedingDedent = + buildAttribute( + "documented", + String.format("attr.%s(doc='''foo\n\n More details.\n ''')", attrType)); + assertThat(documentedNeedingDedent.getDoc()).isEqualTo("foo\n\nMore details."); Attribute undocumented = buildAttribute("undocumented", String.format("attr.%s()", attrType)); assertThat(undocumented.getDoc()).isNull(); } @@ -899,11 +904,21 @@ public void testRuleDoc() throws Exception { ev, "def impl(ctx):", " return None", - "documented_rule = rule(impl, doc='My doc string')", + "documented_rule = rule(impl, doc = 'My doc string')", + "long_documented_rule = rule(", + " impl,", + " doc = '''Long doc", + "", + " With details", + "''',", + ")", "undocumented_rule = rule(impl)"); StarlarkRuleFunction documentedRule = (StarlarkRuleFunction) ev.lookup("documented_rule"); + StarlarkRuleFunction longDocumentedRule = + (StarlarkRuleFunction) ev.lookup("long_documented_rule"); StarlarkRuleFunction undocumentedRule = (StarlarkRuleFunction) ev.lookup("undocumented_rule"); assertThat(documentedRule.getDocumentation()).hasValue("My doc string"); + assertThat(longDocumentedRule.getDocumentation()).hasValue("Long doc\n\nWith details"); assertThat(undocumentedRule.getDocumentation()).isEmpty(); } @@ -1720,10 +1735,13 @@ public void declaredProviderDocumentation() throws Exception { evalAndExport( ev, "UndocumentedInfo = provider()", - "DocumentedInfo = provider(doc = 'My documented provider')", + "DocumentedInfo = provider(doc = '''", + " My documented provider", + "", + " Details''')", // Note fields below are not alphabetized "SchemafulWithoutDocsInfo = provider(fields = ['b', 'a'])", - "SchemafulWithDocsInfo = provider(fields = {'b': 'Field b', 'a': 'Field a'})"); + "SchemafulWithDocsInfo = provider(fields = {'b': 'Field b', 'a': 'Field\\n a'})"); StarlarkProvider undocumentedInfo = (StarlarkProvider) ev.lookup("UndocumentedInfo"); StarlarkProvider documentedInfo = (StarlarkProvider) ev.lookup("DocumentedInfo"); @@ -1732,11 +1750,11 @@ public void declaredProviderDocumentation() throws Exception { StarlarkProvider schemafulWithDocsInfo = (StarlarkProvider) ev.lookup("SchemafulWithDocsInfo"); assertThat(undocumentedInfo.getDocumentation()).isEmpty(); - assertThat(documentedInfo.getDocumentation()).hasValue("My documented provider"); + assertThat(documentedInfo.getDocumentation()).hasValue("My documented provider\n\nDetails"); assertThat(schemafulWithoutDocsInfo.getSchema()) .containsExactly("b", Optional.empty(), "a", Optional.empty()); assertThat(schemafulWithDocsInfo.getSchema()) - .containsExactly("b", Optional.of("Field b"), "a", Optional.of("Field a")); + .containsExactly("b", Optional.of("Field b"), "a", Optional.of("Field\na")); } @Test @@ -2199,10 +2217,20 @@ public void aspectDoc() throws Exception { "def _impl(target, ctx):", // " pass", "documented_aspect = aspect(_impl, doc='My doc string')", + "long_documented_aspect = aspect(", + " _impl,", + " doc='''", + " My doc string", + " ", + " With details''',", + ")", "undocumented_aspect = aspect(_impl)"); StarlarkDefinedAspect documentedAspect = (StarlarkDefinedAspect) ev.lookup("documented_aspect"); assertThat(documentedAspect.getDocumentation()).hasValue("My doc string"); + StarlarkDefinedAspect longDocumentedAspect = + (StarlarkDefinedAspect) ev.lookup("long_documented_aspect"); + assertThat(longDocumentedAspect.getDocumentation()).hasValue("My doc string\n\nWith details"); StarlarkDefinedAspect undocumentedAspect = (StarlarkDefinedAspect) ev.lookup("undocumented_aspect"); assertThat(undocumentedAspect.getDocumentation()).isEmpty(); diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/function_wrap_multiple_lines_test/golden.textproto b/src/test/java/com/google/devtools/build/skydoc/testdata/function_wrap_multiple_lines_test/golden.textproto index 20846fe4f06857..e3255e3c534f38 100644 --- a/src/test/java/com/google/devtools/build/skydoc/testdata/function_wrap_multiple_lines_test/golden.textproto +++ b/src/test/java/com/google/devtools/build/skydoc/testdata/function_wrap_multiple_lines_test/golden.textproto @@ -123,7 +123,7 @@ rule_info { } attribute { name: "deps" - doc_string: "\nThe dependencies to use. Defaults to the most recent ANTLR 3 release,\nbut if you need to use a different version, you can specify the\ndependencies here.\n" + doc_string: "The dependencies to use. Defaults to the most recent ANTLR 3 release,\nbut if you need to use a different version, you can specify the\ndependencies here." type: LABEL_LIST default_value: "[Label(\"@antlr3_runtimes//:tool\")]" } diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/misc_apis_test/golden.textproto b/src/test/java/com/google/devtools/build/skydoc/testdata/misc_apis_test/golden.textproto index ff7dbce0f141f4..3a7f865d521330 100644 --- a/src/test/java/com/google/devtools/build/skydoc/testdata/misc_apis_test/golden.textproto +++ b/src/test/java/com/google/devtools/build/skydoc/testdata/misc_apis_test/golden.textproto @@ -9,7 +9,7 @@ rule_info { } attribute { name: "deps" - doc_string: "\nA list of dependencies.\n" + doc_string: "A list of dependencies." type: LABEL_LIST provider_name_group { provider_name: "MyInfo" diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/provider_basic_test/golden.textproto b/src/test/java/com/google/devtools/build/skydoc/testdata/provider_basic_test/golden.textproto index 14c7b778a64460..0f82fdc11fb27a 100644 --- a/src/test/java/com/google/devtools/build/skydoc/testdata/provider_basic_test/golden.textproto +++ b/src/test/java/com/google/devtools/build/skydoc/testdata/provider_basic_test/golden.textproto @@ -15,7 +15,7 @@ provider_info { } provider_info { provider_name: "MyVeryDocumentedInfo" - doc_string: "\nA provider with some really neat documentation.\nLook on my works, ye mighty, and despair!\n" + doc_string: "A provider with some really neat documentation.\nLook on my works, ye mighty, and despair!" field_info { name: "favorite_food" doc_string: "A string representing my favorite food" diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/unknown_predefines_test/golden.textproto b/src/test/java/com/google/devtools/build/skydoc/testdata/unknown_predefines_test/golden.textproto index 00e84866d2bbb7..6936c857c0484a 100644 --- a/src/test/java/com/google/devtools/build/skydoc/testdata/unknown_predefines_test/golden.textproto +++ b/src/test/java/com/google/devtools/build/skydoc/testdata/unknown_predefines_test/golden.textproto @@ -38,4 +38,4 @@ func_info { mandatory: true } } -module_docstring: "\nTests that the documentation is generated even when unknown modules, calls, or providers are used.\n" +module_docstring: "Tests that the documentation is generated even when unknown modules, calls, or providers are used." diff --git a/src/test/java/net/starlark/java/eval/BUILD b/src/test/java/net/starlark/java/eval/BUILD index 82386955da80e7..3249dc9e1dea0b 100644 --- a/src/test/java/net/starlark/java/eval/BUILD +++ b/src/test/java/net/starlark/java/eval/BUILD @@ -24,6 +24,7 @@ java_test( "MutabilityTest.java", "PrinterTest.java", "StarlarkAnnotationsTest.java", + "StarlarkClassTest.java", "StarlarkEvaluationTest.java", "StarlarkFlagGuardingTest.java", "StarlarkListTest.java", diff --git a/src/test/java/net/starlark/java/eval/EvalTests.java b/src/test/java/net/starlark/java/eval/EvalTests.java index ab364c13d6e83a..63105613cf7b09 100644 --- a/src/test/java/net/starlark/java/eval/EvalTests.java +++ b/src/test/java/net/starlark/java/eval/EvalTests.java @@ -26,6 +26,7 @@ MethodLibraryTest.class, MutabilityTest.class, PrinterTest.class, + StarlarkClassTest.class, StarlarkEvaluationTest.class, StarlarkFlagGuardingTest.class, StarlarkAnnotationsTest.class, diff --git a/src/test/java/net/starlark/java/eval/EvaluationTest.java b/src/test/java/net/starlark/java/eval/EvaluationTest.java index 2f4c37fc8c8b7f..241110df864cf4 100644 --- a/src/test/java/net/starlark/java/eval/EvaluationTest.java +++ b/src/test/java/net/starlark/java/eval/EvaluationTest.java @@ -791,7 +791,8 @@ public void moduleWithDocString() throws Exception { assertThat(module.getDocumentation()).isNull(); ParserInput input = ParserInput.fromLines( - "\"\"\"Module doc header", // + "\"\"\"", + "Module doc header", // "", "Module doc details", "\"\"\"", @@ -805,7 +806,7 @@ public void moduleWithDocString() throws Exception { StarlarkThread thread = new StarlarkThread(mu, StarlarkSemantics.DEFAULT); Starlark.execFile(input, FileOptions.DEFAULT, module, thread); } - assertThat(module.getDocumentation()).isEqualTo("Module doc header\n\nModule doc details\n"); + assertThat(module.getDocumentation()).isEqualTo("Module doc header\n\nModule doc details"); } @Test diff --git a/src/test/java/net/starlark/java/eval/StarlarkClassTest.java b/src/test/java/net/starlark/java/eval/StarlarkClassTest.java new file mode 100644 index 00000000000000..6acb968a1cdeb9 --- /dev/null +++ b/src/test/java/net/starlark/java/eval/StarlarkClassTest.java @@ -0,0 +1,93 @@ +// Copyright 2023 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package net.starlark.java.eval; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for static utility methods in the {@link Starlark} class. */ +@RunWith(JUnit4.class) +public final class StarlarkClassTest { + + @Test + public void trimDocString() { + // See https://peps.python.org/pep-0257/#handling-docstring-indentation + // Single line + assertThat(Starlark.trimDocString("")).isEmpty(); + assertThat(Starlark.trimDocString(" ")).isEmpty(); + assertThat(Starlark.trimDocString("\t\t\t")).isEmpty(); + assertThat(Starlark.trimDocString("Hello world")).isEqualTo("Hello world"); + assertThat(Starlark.trimDocString(" Hello world ")).isEqualTo("Hello world"); + // First line is always fully trimmed, regardless of subsequent indentation levels. + assertThat(Starlark.trimDocString(" Hello\t\nworld")).isEqualTo("Hello\nworld"); + assertThat(Starlark.trimDocString(" Hello \n world")).isEqualTo("Hello\nworld"); + assertThat(Starlark.trimDocString(" Hello \n world")).isEqualTo("Hello\nworld"); + // Subsequent lines are dedented to their minimal indentation level and fully right-trimmed + assertThat(Starlark.trimDocString(" Hello \n world \n and \n good-bye ")) + .isEqualTo("Hello\n world\nand\n good-bye"); + // ... and the first line's indentation does not affect minimal indentation level computation. + assertThat(Starlark.trimDocString(" Hello\n world\n and\n good-bye")) + .isEqualTo("Hello\n world\n and\ngood-bye"); + // Blank lines are trimmed and do not affect indentation level computation + assertThat( + Starlark.trimDocString( + " Hello \n\n world \n\n\n \n and \n \n good-bye ")) + .isEqualTo("Hello\n\n world\n\n\n\nand\n\n good-bye"); + // Windows-style \r\n is simplified to \n + assertThat(Starlark.trimDocString("Hello\r\nworld")).isEqualTo("Hello\nworld"); + assertThat(Starlark.trimDocString("Hello\r\n\r\nworld")).isEqualTo("Hello\n\nworld"); + // Leading and trailing blank lines are removed + assertThat(Starlark.trimDocString("\n\n\n")).isEmpty(); + assertThat(Starlark.trimDocString("\r\n\r\n\r\n")).isEmpty(); + assertThat(Starlark.trimDocString("\n \n \n ")).isEmpty(); + assertThat(Starlark.trimDocString("\n \r\n \r\n ")).isEmpty(); + assertThat(Starlark.trimDocString("\n\r\nHello world\n\r\n")).isEqualTo("Hello world"); + assertThat(Starlark.trimDocString("\n\n \nHello\nworld\n\n \n\t")).isEqualTo("Hello\nworld"); + assertThat(Starlark.trimDocString("\n\n \t\nHello\n world\n \n")).isEqualTo("Hello\n world"); + // Tabs are expanded to size 8 (following Python convention) + assertThat(Starlark.trimDocString("Hello\tworld")).isEqualTo("Hello world"); + assertThat(Starlark.trimDocString("\n\tHello\n\t\tworld")).isEqualTo("Hello\n world"); + assertThat(Starlark.trimDocString("\n \tHello\n\t\tworld")).isEqualTo("Hello\n world"); + assertThat(Starlark.trimDocString("\n Hello\n\tworld")).isEqualTo("Hello\n world"); + } + + @Test + public void expandTabs() { + assertThat(Starlark.expandTabs("", 8)).isEmpty(); + assertThat(Starlark.expandTabs("Hello\nworld", 8)).isEqualTo("Hello\nworld"); + + assertThat(Starlark.expandTabs("\t", 1)).isEqualTo(" "); + assertThat(Starlark.expandTabs("\t", 2)).isEqualTo(" "); + assertThat(Starlark.expandTabs(" \t", 2)).isEqualTo(" "); + assertThat(Starlark.expandTabs("\t", 8)).isEqualTo(" "); + assertThat(Starlark.expandTabs(" \t", 8)).isEqualTo(" "); + + assertThat(Starlark.expandTabs("01\t012\t0123\t01234", 4)).isEqualTo("01 012 0123 01234"); + assertThat(Starlark.expandTabs("01\t012\t0123\t01234", 8)) + .isEqualTo("01 012 0123 01234"); + + assertThat(Starlark.expandTabs("01\t\n\t012\t0123\t01234", 4)) + .isEqualTo("01 \n 012 0123 01234"); + assertThat(Starlark.expandTabs("\r01\r\n\t012\r\n\t0123\t01234\n", 8)) + .isEqualTo("\r01\r\n 012\r\n 0123 01234\n"); + + assertThrows(IllegalArgumentException.class, () -> Starlark.expandTabs("\t", 0)); + assertThrows(IllegalArgumentException.class, () -> Starlark.expandTabs("\t", -1)); + } +} diff --git a/src/tools/starlark/java/com/google/devtools/starlark/common/DocstringUtils.java b/src/tools/starlark/java/com/google/devtools/starlark/common/DocstringUtils.java index 222276ad929945..791ad657afc789 100644 --- a/src/tools/starlark/java/com/google/devtools/starlark/common/DocstringUtils.java +++ b/src/tools/starlark/java/com/google/devtools/starlark/common/DocstringUtils.java @@ -20,7 +20,9 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.Iterator; import java.util.List; +import java.util.Locale; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -30,7 +32,7 @@ public final class DocstringUtils { private DocstringUtils() {} // uninstantiable /** - * Parses a docstring. + * Parses a trimmed docstring. * *

The format of the docstring is as follows * @@ -58,7 +60,12 @@ private DocstringUtils() {} // uninstantiable * """ * } * - * @param docstring a docstring of the format described above + *

We expect the docstring to already be trimmed and dedented to a minimal common indentation + * level by {@link Starlark#trimDocString} or an equivalent PEP-257 style trim() implementation; + * note that {@link StarlarkFunction#getDocumentation} returns a correctly trimmed and dedented + * doc string. + * + * @param doc a docstring of the format described above * @param parseErrors a list to which parsing error messages are written * @return the parsed docstring information */ @@ -166,64 +173,35 @@ public String getDescription() { } private static class DocstringParser { - private final String docstring; - /** Start offset of the current line. */ - private int startOfLineOffset = 0; - /** End offset of the current line. */ - private int endOfLineOffset = 0; - /** Current line number within the docstring. */ - private int lineNumber = 0; /** - * The indentation of the docstring literal in the source file. - * - *

Every line except the first one must be indented by at least that many spaces. + * Current line number within the docstring, 1-based; 0 indicates that parsing has not started; + * {@code lineNumber > lines.size()} indicates EOF. */ - private int baselineIndentation = 0; + private int lineNumber = 0; + /** Whether there was a blank line before the current line. */ private boolean blankLineBefore = false; + /** Whether we've seen a special section, e.g. 'Args:', already. */ private boolean specialSectionsStarted = false; - /** List of all parsed lines in the docstring so far, including all indentation. */ - private ArrayList originalLines = new ArrayList<>(); - /** - * The current line in the docstring with the baseline indentation removed. - * - *

If the indentation of a docstring line is less than the expected {@link - * #baselineIndentation}, only the existing indentation is stripped; none of the remaining - * characters are cut off. - */ + + /** List of all parsed lines in the docstring. */ + private final ImmutableList lines; + + /** Iterator over lines. */ + private final Iterator linesIter; + + /** The current line in the docstring. */ private String line = ""; + /** Errors that occurred so far. */ private final List errors = new ArrayList<>(); private DocstringParser(String docstring) { - this.docstring = docstring; - - // Infer the indentation level: - // the smallest amount of leading whitespace - // common to all non-blank lines except the first. - int indentation = Integer.MAX_VALUE; - boolean first = true; - for (String line : Splitter.on("\n").split(docstring)) { - // ignore first line - if (first) { - first = false; - continue; - } - // count leading spaces - int i; - for (i = 0; i < line.length() && line.charAt(i) == ' '; i++) {} - if (i != line.length()) { - indentation = Math.min(indentation, i); - } - } - if (indentation == Integer.MAX_VALUE) { - indentation = 0; - } - + this.lines = ImmutableList.copyOf(Splitter.on("\n").split(docstring)); + this.linesIter = lines.iterator(); + // Load the summary line nextLine(); - // the indentation is only relevant for the following lines, not the first one: - this.baselineIndentation = indentation; } /** @@ -232,69 +210,20 @@ private DocstringParser(String docstring) { * @return whether there are lines remaining to be parsed */ private boolean nextLine() { - if (startOfLineOffset >= docstring.length()) { - return false; - } - blankLineBefore = line.trim().isEmpty(); - startOfLineOffset = endOfLineOffset; - if (startOfLineOffset >= docstring.length()) { - // Previous line was the last; previous line had no trailing newline character. + if (linesIter.hasNext()) { + blankLineBefore = line.trim().isEmpty(); + line = linesIter.next(); + lineNumber++; + return true; + } else { line = ""; + lineNumber = lines.size() + 1; return false; } - // If not the first line, advance start past the newline character. In the case where there is - // no more content, then the previous line was the second-to-last line and this last line is - // empty. - if (docstring.charAt(startOfLineOffset) == '\n') { - startOfLineOffset += 1; - } - lineNumber++; - endOfLineOffset = docstring.indexOf('\n', startOfLineOffset); - if (endOfLineOffset < 0) { - endOfLineOffset = docstring.length(); - } - String originalLine = docstring.substring(startOfLineOffset, endOfLineOffset); - originalLines.add(originalLine); - int indentation = getIndentation(originalLine); - if (endOfLineOffset == docstring.length() && startOfLineOffset != 0) { - if (!originalLine.trim().isEmpty()) { - error("closing docstring quote should be on its own line, indented the same as the " - + "opening quote"); - } else if (indentation != baselineIndentation) { - error("closing docstring quote should be indented the same as the opening quote"); - } - } - if (originalLine.trim().isEmpty()) { - line = ""; - } else { - if (indentation < baselineIndentation) { - error( - "line indented too little (here: " - + indentation - + " spaces; expected: " - + baselineIndentation - + " spaces)"); - startOfLineOffset += indentation; - } else { - startOfLineOffset += baselineIndentation; - } - line = docstring.substring(startOfLineOffset, endOfLineOffset); - } - return true; - } - - /** - * Returns whether the current line is the last one in the docstring. - * - *

It is possible for both this function and {@link #eof} to return true if all content has - * been exhausted, or if the last line is empty. - */ - private boolean onLastLine() { - return endOfLineOffset >= docstring.length(); } private boolean eof() { - return startOfLineOffset >= docstring.length(); + return lineNumber > lines.size(); } private static int getIndentation(String line) { @@ -310,7 +239,7 @@ private void error(String message) { } private void error(int lineNumber, String message) { - errors.add(new DocstringParseError(message, lineNumber, originalLines.get(lineNumber - 1))); + errors.add(new DocstringParseError(message, lineNumber, lines.get(lineNumber - 1))); } private void parseArgumentSection( @@ -375,9 +304,7 @@ DocstringInfo parse() { + " \n\n" + "For more details, please have a look at the documentation."); } - if (!(onLastLine() && line.trim().isEmpty())) { - longDescriptionLines.add(line); - } + longDescriptionLines.add(line); nextLine(); } } @@ -399,7 +326,7 @@ private void checkSectionStart(boolean duplicateSection) { } private String checkForNonStandardDeprecation(String line) { - if (line.toLowerCase().startsWith("deprecated:") || line.contains("DEPRECATED")) { + if (line.toLowerCase(Locale.ROOT).startsWith("deprecated:") || line.contains("DEPRECATED")) { error( "use a 'Deprecated:' section for deprecations, similar to a 'Returns:' section:\n\n" + "Deprecated:\n" @@ -562,7 +489,10 @@ public String getMessage() { return message; } - /** Returns the line number in the containing Starlark file which contains this error. */ + /** + * Returns the line number (skipping leading blank lines, if any) in the original doc string + * which contains this error. + */ public int getLineNumber() { return lineNumber; } From 32df89144bb119476ad69a306290482a899e3217 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 1 Aug 2023 11:28:04 -0700 Subject: [PATCH 16/17] Graveyard --use_top_level_targets_for_symlinks. Followup to https://github.com/bazelbuild/bazel/commit/ceb9955cc36e65e48923dbe51437f589ce277eef . This change removes all flag logic: its functionality is now unconditionally on. PiperOrigin-RevId: 552865325 Change-Id: I4dc9f23d1a72c300e3cf3aa30e9052d20aafb923 --- .../lib/bazel/rules/BazelRulesModule.java | 8 +++++ .../lib/buildtool/BuildRequestOptions.java | 30 ------------------- .../build/lib/buildtool/ExecutionTool.java | 25 +++++++++------- .../lib/buildtool/ConvenienceSymlinkTest.java | 5 +--- 4 files changed, 23 insertions(+), 45 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java index dbf390fe874966..db59d1b460f34e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java @@ -44,6 +44,14 @@ public final class BazelRulesModule extends BlazeModule { */ @SuppressWarnings("deprecation") // These fields have no JavaDoc by design public static class BuildGraveyardOptions extends OptionsBase { + @Option( + name = "use_top_level_targets_for_symlinks", + defaultValue = "true", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.AFFECTS_OUTPUTS}, + help = "Deprecated. No-op.") + public boolean useTopLevelTargetsForSymlinks; + @Option( name = "experimental_skyframe_prepare_analysis", deprecationWarning = "This flag is a no-op and will be deleted in a future release.", diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java index 48842f71178282..197d944d1b183e 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java @@ -310,36 +310,6 @@ public String getSymlinkPrefix(String productName) { return symlinkPrefix == null ? productName + "-" : symlinkPrefix; } - // Transitional flag for safely rolling out new convenience symlink behavior. - // To be made a no-op and deleted once new symlink behavior is battle-tested. - @Option( - name = "use_top_level_targets_for_symlinks", - defaultValue = "true", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - effectTags = {OptionEffectTag.AFFECTS_OUTPUTS}, - help = - "If enabled, the symlinks are based on the configurations of the top-level targets " - + " rather than the top-level target configuration. If this would be ambiguous, " - + " the symlinks will be deleted to avoid confusion.") - public boolean useTopLevelTargetsForSymlinks; - - /** - * Returns whether to use the output directories used by the top-level targets for convenience - * symlinks. - * - *

If true, then symlinks use the actual output directories of the top-level targets. The - * symlinks will be created iff all top-level targets share the same output directory. Otherwise, - * any stale symlinks from previous invocations will be deleted to avoid ambiguity. - * - *

If false, then symlinks use the output directory implied by command-line flags, regardless - * of whether top-level targets have transitions which change them (or even have any output - * directories at all, as in the case of a build with no targets or one which only builds source - * files). - */ - public boolean useTopLevelTargetsForSymlinks() { - return useTopLevelTargetsForSymlinks; - } - @Option( name = "experimental_create_py_symlinks", defaultValue = "false", diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java index 5d1771e6ee453d..91c74921e94ad2 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java @@ -743,15 +743,18 @@ public void handleConvenienceSymlinks( /** * Creates convenience symlinks based on the target configurations. * - *

Exactly what target configurations we consider depends on the value of {@code - * --use_top_level_targets_for_symlinks}. If this flag is false, we use the top-level target - * configuration as represented by the command line prior to processing any target. If the flag is - * true, we instead use the configurations OF the top-level targets -- meaning that we account for - * the effects of any rule transitions these targets may have. + *

Top-level targets may have different configurations than the top-level configuration. This + * is because targets may apply configuration transitions. * - *

For each type of convenience symlink, if all the considered configurations agree on what - * path the symlink should point to, it gets created; otherwise, the symlink is not created, and - * in fact gets removed if it was already present from a previous invocation. + *

If all top-level targets have the same configuration - even if that isn't the top-level + * configuration - symlinks point to that configuration. + * + *

If top-level targets have mixed configurations and at least one of them has the top-level + * configuration, symliks point to the top-level configuration. + * + *

If top-level targets have mixed configurations and none has the top-level configuration, + * symlinks aren't created. Furthermore, lingering symlinks from the last build are deleted. This + * is to prevent confusion by pointing to an outdated directory the current build never used. */ private ImmutableList createConvenienceSymlinks( BuildRequestOptions buildRequestOptions, @@ -762,7 +765,9 @@ private ImmutableList createConvenienceSymlinks( // Gather configurations to consider. ImmutableSet targetConfigs; - if (buildRequestOptions.useTopLevelTargetsForSymlinks() && !targetsToBuild.isEmpty()) { + if (targetsToBuild.isEmpty()) { + targetConfigs = ImmutableSet.of(configuration); + } else { // Collect the configuration of each top-level requested target. These may be different than // the build's top-level configuration because of self-transitions. ImmutableSet requestedTargetConfigs = @@ -791,8 +796,6 @@ private ImmutableList createConvenienceSymlinks( // createOutputDirectorySymlinks call below. targetConfigs = requestedTargetConfigs; } - } else { - targetConfigs = ImmutableSet.of(configuration); } String productName = runtime.getProductName(); diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java index 3090658802b5cb..aa8c9a2271c8b3 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java @@ -238,10 +238,7 @@ public void initializeRuleClasses(ConfiguredRuleClassProvider.Builder builder) { protected void setupOptions() throws Exception { super.setupOptions(); - addOptions( - // Turn on the new symlink behavior - "--use_top_level_targets_for_symlinks", - "--experimental_merged_skyframe_analysis_execution=" + mergedAnalysisExecution); + addOptions("--experimental_merged_skyframe_analysis_execution=" + mergedAnalysisExecution); } @Override From cf14039d0394570ba69157e1d519b9edb76049b9 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 1 Aug 2023 12:34:14 -0700 Subject: [PATCH 17/17] Introduce more dependencies in Bazel Bzlmod build Working towards: https://github.com/bazelbuild/bazel/issues/18957 RELNOTES: PiperOrigin-RevId: 552886589 Change-Id: I2d170deae5bdaf05dbece4779ffb5b1137ee7b81 --- MODULE.bazel | 89 ++++++++++++++++++++++++++++++++++++------------ WORKSPACE.bzlmod | 52 ++++++++++++++++++++++++++++ extensions.bzl | 21 ++++++++++++ 3 files changed, 141 insertions(+), 21 deletions(-) diff --git a/MODULE.bazel b/MODULE.bazel index 77b720dd36049b..9d70633d92beba 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -1,5 +1,3 @@ -# To build Bazel with Bzlmod: `bazel build --enable_bzlmod //src:bazel_nojdk`. - module( name = "bazel", version = "7.0.0-pre", @@ -20,15 +18,33 @@ bazel_dep(name = "rules_cc", version = "0.0.8") bazel_dep(name = "rules_java", version = "6.3.0") bazel_dep(name = "rules_proto", version = "5.3.0-21.7") bazel_dep(name = "rules_jvm_external", version = "5.2") -bazel_dep(name = "rules_python", version = "0.19.0") +bazel_dep(name = "rules_python", version = "0.24.0") bazel_dep(name = "rules_testing", version = "0.0.4") +# TODO(pcloudy): Add remoteapis and googleapis as Bazel modules in the BCR. +bazel_dep(name = "remoteapis", version = "") +bazel_dep(name = "googleapis", version = "") + single_version_override( module_name = "rules_jvm_external", patch_strip = 1, patches = ["//third_party:rules_jvm_external_5.2.patch"], ) +local_path_override( + module_name = "remoteapis", + path = "./third_party/remoteapis", +) + +local_path_override( + module_name = "googleapis", + path = "./third_party/googleapis", +) + +# ========================================= +# Bazel Java dependencies +# ========================================= + maven = use_extension("@rules_jvm_external//:extensions.bzl", "maven") maven.install( # We don't yet specify the maven coordinates in the MODULE.bazel to avoid duplicating information. @@ -41,35 +57,66 @@ maven.install( ) use_repo(maven, "maven") -# TODO(pcloudy): Add remoteapis and googleapis as Bazel modules in the BCR. -bazel_dep(name = "remoteapis", version = "") -bazel_dep(name = "googleapis", version = "") - -local_path_override( - module_name = "remoteapis", - path = "./third_party/remoteapis", -) - -local_path_override( - module_name = "googleapis", - path = "./third_party/googleapis", -) +# ========================================= +# Other Bazel internal dependencies +# - embedded JDKs +# - repos for Debian build +# ========================================= bazel_internal_deps = use_extension("//:extensions.bzl", "bazel_internal_deps") use_repo( bazel_internal_deps, - "openjdk_linux_vanilla", + "debian_cc_deps", "openjdk_linux_aarch64_vanilla", "openjdk_linux_ppc64le_vanilla", "openjdk_linux_s390x_vanilla", - "openjdk_macos_x86_64_vanilla", + "openjdk_linux_vanilla", "openjdk_macos_aarch64_vanilla", - "openjdk_win_vanilla", + "openjdk_macos_x86_64_vanilla", "openjdk_win_arm64_vanilla", + "openjdk_win_vanilla", ) +# ========================================= +# Register platforms & toolchains +# ========================================= + +register_execution_platforms("//:default_host_platform") + register_toolchains("@bazel_tools//tools/python:autodetecting_toolchain") -# Dev dependencies +register_toolchains("//src/main/res:empty_rc_toolchain") + +# ========================================= +# Android tools dependencies +# ========================================= + +maven_android = use_extension("@rules_jvm_external//:extensions.bzl", "maven") +maven_android.install( + name = "maven_android", + # We don't yet specify the maven coordinates in the MODULE.bazel to avoid duplicating information. + # Always respect the maven_install.json file generated by rules_jvm_external from the WORKSPACE file. + # TODO(pcloudy): We should add maven coordinates here when we enable Bzlmod in the default build for Bazel. + lock_file = "//src/tools/android:maven_android_install.json", + repositories = [ + "https://dl.google.com/android/maven2", + "https://repo1.maven.org/maven2", + ], +) +use_repo(maven_android, "maven_android") + +bazel_android_deps = use_extension("//:extensions.bzl", "bazel_android_deps") +use_repo( + bazel_android_deps, + "android_gmaven_r8", + "desugar_jdk_libs", +) + +# ========================================= +# Bazel dev dependencies +# ========================================= + +bazel_dep(name = "googletest", version = "1.12.1", repo_name = "com_google_googletest") -bazel_dep(name = "googletest", version = "1.12.1", repo_name = "com_google_googletest", dev_dependency = True) +bazel_dev_deps = use_extension("//:extensions.bzl", "bazel_dev_deps") +use_repo(bazel_dev_deps, "local_bazel_source_list") diff --git a/WORKSPACE.bzlmod b/WORKSPACE.bzlmod index 81e315557a362f..639b5f16465973 100644 --- a/WORKSPACE.bzlmod +++ b/WORKSPACE.bzlmod @@ -1,2 +1,54 @@ # This file will replace the content of the WORKSPACE file when Bzlmod is enabled # No WORKSPACE prefix or suffix are added. + +bind( + name = "android/sdk", + actual = "@bazel_tools//tools/android:poison_pill_android_sdk", +) + +bind( + name = "android/dx_jar_import", + actual = "@bazel_tools//tools/android:no_android_sdk_repository_error", +) + +bind( + name = "android/d8_jar_import", + actual = "@bazel_tools//tools/android:no_android_sdk_repository_error", +) + +bind( + name = "android/crosstool", + actual = "@bazel_tools//tools/cpp:toolchain", +) + +bind( + name = "android_sdk_for_testing", + actual = "@bazel_tools//tools/android:empty", +) + +bind( + name = "android_ndk_for_testing", + actual = "@bazel_tools//tools/android:empty", +) + +bind( + name = "databinding_annotation_processor", + actual = "@bazel_tools//tools/android:empty", +) + +# This value is overridden by android_sdk_repository function to allow targets +# to select on whether or not android_sdk_repository has run. +bind( + name = "has_androidsdk", + actual = "@bazel_tools//tools/android:always_false", +) + +# To run the Android integration tests in //src/test/shell/bazel/android:all or +# build the Android sample app in //examples/android/java/bazel:hello_world +# +# 1. Install an Android SDK and NDK from https://developer.android.com +# 2. Set the $ANDROID_HOME and $ANDROID_NDK_HOME environment variables +# 3. Uncomment the two lines below +# +# android_sdk_repository(name = "androidsdk") +# android_ndk_repository(name = "androidndk") diff --git a/extensions.bzl b/extensions.bzl index 800ee5f8dc9d23..1e38f0a4bcae03 100644 --- a/extensions.bzl +++ b/extensions.bzl @@ -17,8 +17,29 @@ """ load("//:repositories.bzl", "embedded_jdk_repositories") +load("//:distdir.bzl", "dist_http_archive", "dist_http_jar") +load("//tools/distributions/debian:deps.bzl", "debian_deps") +load("//src/test/shell/bazel:list_source_repository.bzl", "list_source_repository") + +### Extra dependencies for building Bazel def _bazel_internal_deps(_ctx): embedded_jdk_repositories() + debian_deps() bazel_internal_deps = module_extension(implementation = _bazel_internal_deps) + +### Extra dependencies for testing Bazel + +def _bazel_dev_deps(_ctx): + list_source_repository(name = "local_bazel_source_list") + +bazel_dev_deps = module_extension(implementation = _bazel_dev_deps) + +### Extra dependencies for Bazel Android tools + +def _bazel_android_deps(_ctx): + dist_http_jar(name = "android_gmaven_r8") + dist_http_archive(name = "desugar_jdk_libs") + +bazel_android_deps = module_extension(implementation = _bazel_android_deps)