From 65a9411f572a7c0c4b208038d58bd5250f97c7e3 Mon Sep 17 00:00:00 2001 From: Christopher Peterson Sauer Date: Mon, 31 Oct 2022 23:43:47 -0700 Subject: [PATCH 1/2] Resolve external paths to their workspaces Please see https://github.com/bazelbuild/intellij/issues/2766 for context --- .../workspace/ExecutionRootPathResolver.java | 34 +++++++++++++++---- .../ExecutionRootPathResolverTest.java | 5 +-- 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/base/src/com/google/idea/blaze/base/sync/workspace/ExecutionRootPathResolver.java b/base/src/com/google/idea/blaze/base/sync/workspace/ExecutionRootPathResolver.java index c35fd0a2e9e..d4518f72a92 100644 --- a/base/src/com/google/idea/blaze/base/sync/workspace/ExecutionRootPathResolver.java +++ b/base/src/com/google/idea/blaze/base/sync/workspace/ExecutionRootPathResolver.java @@ -25,14 +25,16 @@ import com.google.idea.blaze.base.sync.data.BlazeProjectDataManager; import com.intellij.openapi.project.Project; import java.io.File; +import java.util.regex.Pattern; import javax.annotation.Nullable; /** * Converts execution-root-relative paths to absolute files with a minimum of file system calls * (typically none). * - *

Files which exist both underneath the execution root and within the workspace will be resolved - * to workspace paths. + *

Files which exist both underneath the execution root and within a workspace will be resolved + * to paths within their workspace. This prevents those paths from being broken when a different + * target is built. */ public class ExecutionRootPathResolver { @@ -76,13 +78,13 @@ public File resolveExecutionRootPath(ExecutionRootPath path) { if (isInWorkspace(path)) { return workspacePathResolver.resolveToFile(path.getAbsoluteOrRelativeFile().getPath()); } - return path.getFileRootedAt(executionRoot); + return convertExternalToStable(path.getFileRootedAt(executionRoot)); } /** * This method should be used for directories. Returns all workspace files corresponding to the - * given execution-root-relative path. If the file does not exist inside the workspace (e.g. for - * blaze output files or external workspace files), returns the path rooted in the execution root. + * given execution-root-relative path. If the file does not exist inside a workspace (e.g. for + * blaze output files), returns the path rooted in the execution root. */ public ImmutableList resolveToIncludeDirectories(ExecutionRootPath path) { if (path.isAbsolute()) { @@ -97,7 +99,7 @@ public ImmutableList resolveToIncludeDirectories(ExecutionRootPath path) { return ImmutableList.of(); } } - return ImmutableList.of(path.getFileRootedAt(executionRoot)); + return ImmutableList.of(convertExternalToStable(path.getFileRootedAt(executionRoot))); } public File getExecutionRoot() { @@ -118,4 +120,24 @@ private static String getFirstPathComponent(String path) { private static boolean isExternalWorkspacePath(String firstPathComponent) { return firstPathComponent.equals("external"); } + + /** + * Converts external paths in unstable locations under execroot that may change on the next build + * to stable ones under their external workspace. + * That is, converts paths under /execroot//external/ to paths under + * /external/. + * Safe to call on all paths; non-external paths are left as they are. + * See https://github.com/bazelbuild/intellij/issues/2766 for more details. + */ + private static File convertExternalToStable(File f) { + String regexSep = Pattern.quote(f.separator); + // Workspace name defaults to __main__ per DEFAULT_REPOSITORY_DIRECTORY in + // https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/cmdline/LabelConstants.java + // Valid workspace name regex copied from LEGAL_WORKSPACE_NAME in + // https://github.com/bazelbuild/bazel/blob/9302ebd906a2f5e9678f994efb2fbc8abab544c0/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java + String regexUnstableExecrootSubpath = // ↓ matches workspace name. + regexSep + "execroot" + regexSep + "(__main__|\\p{Alpha}[-.\\w]*)" + regexSep + "external" + regexSep; + String stableExternalSubpath = f.separator + "external" + f.separator; + return new File(f.getAbsolutePath().replaceFirst(regexUnstableExecrootSubpath, stableExternalSubpath)); + } } diff --git a/base/tests/unittests/com/google/idea/blaze/base/sync/workspace/ExecutionRootPathResolverTest.java b/base/tests/unittests/com/google/idea/blaze/base/sync/workspace/ExecutionRootPathResolverTest.java index c0c788be419..0c6cead3c94 100644 --- a/base/tests/unittests/com/google/idea/blaze/base/sync/workspace/ExecutionRootPathResolverTest.java +++ b/base/tests/unittests/com/google/idea/blaze/base/sync/workspace/ExecutionRootPathResolverTest.java @@ -33,7 +33,8 @@ public class ExecutionRootPathResolverTest extends BlazeTestCase { private static final WorkspaceRoot WORKSPACE_ROOT = new WorkspaceRoot(new File("/path/to/root")); - private static final String EXECUTION_ROOT = "/path/to/_bazel_user/1234bf129e/root"; + private static final String EXECUTION_ROOT = "/path/to/_bazel_user/1234bf129e/execroot/__main__"; + private static final String OUTPUT_BASE = "/path/to/_bazel_user/1234bf129e"; private ExecutionRootPathResolver pathResolver; @@ -51,7 +52,7 @@ protected void initTest(Container applicationServices, Container projectServices public void testExternalWorkspacePathRelativeToExecRoot() { ImmutableList files = pathResolver.resolveToIncludeDirectories(new ExecutionRootPath("external/guava/src")); - assertThat(files).containsExactly(new File(EXECUTION_ROOT, "external/guava/src")); + assertThat(files).containsExactly(new File(OUTPUT_BASE, "external/guava/src")); } @Test From 78ce07e30f539ee0bc070e6be3b958f0fb2ef27b Mon Sep 17 00:00:00 2001 From: Christopher Peterson Sauer Date: Tue, 31 Jan 2023 15:47:09 -0800 Subject: [PATCH 2/2] Switch external path resolution implementation to getFileRootedAt(outputBase) Based on tpasternak's good feedback. Please see https://github.com/bazelbuild/intellij/issues/2766 and https://github.com/bazelbuild/intellij/pull/4063 for context. --- .../workspace/ExecutionRootPathResolver.java | 75 ++++++++----------- .../ExecutionRootPathResolverTest.java | 5 +- .../idea/blaze/cpp/BlazeCWorkspace.java | 1 + .../blaze/cpp/BlazeConfigurationResolver.java | 1 + .../blaze/cpp/IncludeRootFlagsProcessor.java | 1 + .../blaze/cpp/BlazeCppResolvingTestCase.java | 1 + .../blaze/cpp/BlazeCompilerSettingsTest.java | 28 ++++++- 7 files changed, 63 insertions(+), 49 deletions(-) diff --git a/base/src/com/google/idea/blaze/base/sync/workspace/ExecutionRootPathResolver.java b/base/src/com/google/idea/blaze/base/sync/workspace/ExecutionRootPathResolver.java index d4518f72a92..c7706730dc9 100644 --- a/base/src/com/google/idea/blaze/base/sync/workspace/ExecutionRootPathResolver.java +++ b/base/src/com/google/idea/blaze/base/sync/workspace/ExecutionRootPathResolver.java @@ -25,7 +25,6 @@ import com.google.idea.blaze.base.sync.data.BlazeProjectDataManager; import com.intellij.openapi.project.Project; import java.io.File; -import java.util.regex.Pattern; import javax.annotation.Nullable; /** @@ -40,15 +39,18 @@ public class ExecutionRootPathResolver { private final ImmutableList buildArtifactDirectories; private final File executionRoot; + private final File outputBase; private final WorkspacePathResolver workspacePathResolver; public ExecutionRootPathResolver( BuildSystemProvider buildSystemProvider, WorkspaceRoot workspaceRoot, File executionRoot, + File outputBase, WorkspacePathResolver workspacePathResolver) { this.buildArtifactDirectories = buildArtifactDirectories(buildSystemProvider, workspaceRoot); this.executionRoot = executionRoot; + this.outputBase = outputBase; this.workspacePathResolver = workspacePathResolver; } @@ -63,6 +65,7 @@ public static ExecutionRootPathResolver fromProject(Project project) { Blaze.getBuildSystemProvider(project), WorkspaceRoot.fromProject(project), projectData.getBlazeInfo().getExecutionRoot(), + projectData.getBlazeInfo().getOutputBase(), projectData.getWorkspacePathResolver()); } @@ -75,10 +78,18 @@ public File resolveExecutionRootPath(ExecutionRootPath path) { if (path.isAbsolute()) { return path.getAbsoluteOrRelativeFile(); } - if (isInWorkspace(path)) { - return workspacePathResolver.resolveToFile(path.getAbsoluteOrRelativeFile().getPath()); + String firstPathComponent = getFirstPathComponent(path.getAbsoluteOrRelativeFile().getPath()); + if (buildArtifactDirectories.contains(firstPathComponent)) { + // Build artifacts accumulate under the execution root, independent of symlink settings + return path.getFileRootedAt(executionRoot); + } + if (firstPathComponent.equals("external")) { // In external workspace + // External workspaces accumulate under the output base. + // The symlinks to them under the execution root are unstable, and only linked per build. + return path.getFileRootedAt(outputBase); } - return convertExternalToStable(path.getFileRootedAt(executionRoot)); + // Else, in main workspace + return workspacePathResolver.resolveToFile(path.getAbsoluteOrRelativeFile().getPath()); } /** @@ -90,54 +101,32 @@ public ImmutableList resolveToIncludeDirectories(ExecutionRootPath path) { if (path.isAbsolute()) { return ImmutableList.of(path.getAbsoluteOrRelativeFile()); } - if (isInWorkspace(path)) { - WorkspacePath workspacePath = - WorkspacePath.createIfValid(path.getAbsoluteOrRelativeFile().getPath()); - if (workspacePath != null) { - return workspacePathResolver.resolveToIncludeDirectories(workspacePath); - } else { - return ImmutableList.of(); - } + String firstPathComponent = getFirstPathComponent(path.getAbsoluteOrRelativeFile().getPath()); + if (buildArtifactDirectories.contains(firstPathComponent)) { + // Build artifacts accumulate under the execution root, independent of symlink settings + return ImmutableList.of(path.getFileRootedAt(executionRoot)); + } + if (firstPathComponent.equals("external")) { // In external workspace + // External workspaces accumulate under the output base. + // The symlinks to them under the execution root are unstable, and only linked per build. + return ImmutableList.of(path.getFileRootedAt(outputBase)); + } + // Else, in main workspace + WorkspacePath workspacePath = + WorkspacePath.createIfValid(path.getAbsoluteOrRelativeFile().getPath()); + if (workspacePath != null) { + return workspacePathResolver.resolveToIncludeDirectories(workspacePath); + } else { + return ImmutableList.of(); } - return ImmutableList.of(convertExternalToStable(path.getFileRootedAt(executionRoot))); } public File getExecutionRoot() { return executionRoot; } - private boolean isInWorkspace(ExecutionRootPath path) { - String firstPathComponent = getFirstPathComponent(path.getAbsoluteOrRelativeFile().getPath()); - return !buildArtifactDirectories.contains(firstPathComponent) - && !isExternalWorkspacePath(firstPathComponent); - } - private static String getFirstPathComponent(String path) { int index = path.indexOf(File.separatorChar); return index == -1 ? path : path.substring(0, index); } - - private static boolean isExternalWorkspacePath(String firstPathComponent) { - return firstPathComponent.equals("external"); - } - - /** - * Converts external paths in unstable locations under execroot that may change on the next build - * to stable ones under their external workspace. - * That is, converts paths under /execroot//external/ to paths under - * /external/. - * Safe to call on all paths; non-external paths are left as they are. - * See https://github.com/bazelbuild/intellij/issues/2766 for more details. - */ - private static File convertExternalToStable(File f) { - String regexSep = Pattern.quote(f.separator); - // Workspace name defaults to __main__ per DEFAULT_REPOSITORY_DIRECTORY in - // https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/cmdline/LabelConstants.java - // Valid workspace name regex copied from LEGAL_WORKSPACE_NAME in - // https://github.com/bazelbuild/bazel/blob/9302ebd906a2f5e9678f994efb2fbc8abab544c0/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java - String regexUnstableExecrootSubpath = // ↓ matches workspace name. - regexSep + "execroot" + regexSep + "(__main__|\\p{Alpha}[-.\\w]*)" + regexSep + "external" + regexSep; - String stableExternalSubpath = f.separator + "external" + f.separator; - return new File(f.getAbsolutePath().replaceFirst(regexUnstableExecrootSubpath, stableExternalSubpath)); - } } diff --git a/base/tests/unittests/com/google/idea/blaze/base/sync/workspace/ExecutionRootPathResolverTest.java b/base/tests/unittests/com/google/idea/blaze/base/sync/workspace/ExecutionRootPathResolverTest.java index 0c6cead3c94..ee2478bbe30 100644 --- a/base/tests/unittests/com/google/idea/blaze/base/sync/workspace/ExecutionRootPathResolverTest.java +++ b/base/tests/unittests/com/google/idea/blaze/base/sync/workspace/ExecutionRootPathResolverTest.java @@ -45,11 +45,12 @@ protected void initTest(Container applicationServices, Container projectServices new BazelBuildSystemProvider(), WORKSPACE_ROOT, new File(EXECUTION_ROOT), + new File(OUTPUT_BASE), new WorkspacePathResolverImpl(WORKSPACE_ROOT)); } @Test - public void testExternalWorkspacePathRelativeToExecRoot() { + public void testExternalWorkspacePathRelativeToOutputBase() { ImmutableList files = pathResolver.resolveToIncludeDirectories(new ExecutionRootPath("external/guava/src")); assertThat(files).containsExactly(new File(OUTPUT_BASE, "external/guava/src")); @@ -65,7 +66,7 @@ public void testGenfilesPathRelativeToExecRoot() { } @Test - public void testNonOutputPathsRelativeToWorkspaceRoot() { + public void testMainWorkspacePathsRelativeToWorkspaceRoot() { ImmutableList files = pathResolver.resolveToIncludeDirectories(new ExecutionRootPath("tools/fast")); assertThat(files).containsExactly(WORKSPACE_ROOT.fileForPath(new WorkspacePath("tools/fast"))); diff --git a/cpp/src/com/google/idea/blaze/cpp/BlazeCWorkspace.java b/cpp/src/com/google/idea/blaze/cpp/BlazeCWorkspace.java index 6f3271a3a41..8260416431f 100644 --- a/cpp/src/com/google/idea/blaze/cpp/BlazeCWorkspace.java +++ b/cpp/src/com/google/idea/blaze/cpp/BlazeCWorkspace.java @@ -173,6 +173,7 @@ private OCWorkspaceImpl.ModifiableModel calculateConfigurations( Blaze.getBuildSystemProvider(project), workspaceRoot, blazeProjectData.getBlazeInfo().getExecutionRoot(), + blazeProjectData.getBlazeInfo().getOutputBase(), blazeProjectData.getWorkspacePathResolver()); int progress = 0; diff --git a/cpp/src/com/google/idea/blaze/cpp/BlazeConfigurationResolver.java b/cpp/src/com/google/idea/blaze/cpp/BlazeConfigurationResolver.java index e3968e4d08c..1098913aff6 100644 --- a/cpp/src/com/google/idea/blaze/cpp/BlazeConfigurationResolver.java +++ b/cpp/src/com/google/idea/blaze/cpp/BlazeConfigurationResolver.java @@ -75,6 +75,7 @@ public BlazeConfigurationResolverResult update( Blaze.getBuildSystemProvider(project), WorkspaceRoot.fromProject(project), blazeProjectData.getBlazeInfo().getExecutionRoot(), + blazeProjectData.getBlazeInfo().getOutputBase(), blazeProjectData.getWorkspacePathResolver()); ImmutableMap toolchainLookupMap = BlazeConfigurationToolchainResolver.buildToolchainLookupMap( diff --git a/cpp/src/com/google/idea/blaze/cpp/IncludeRootFlagsProcessor.java b/cpp/src/com/google/idea/blaze/cpp/IncludeRootFlagsProcessor.java index fc9a71acc08..7362da39602 100644 --- a/cpp/src/com/google/idea/blaze/cpp/IncludeRootFlagsProcessor.java +++ b/cpp/src/com/google/idea/blaze/cpp/IncludeRootFlagsProcessor.java @@ -47,6 +47,7 @@ public Optional getProcessor(Project project) { Blaze.getBuildSystemProvider(project), workspaceRoot, projectData.getBlazeInfo().getExecutionRoot(), + projectData.getBlazeInfo().getOutputBase(), projectData.getWorkspacePathResolver()); return Optional.of(new IncludeRootFlagsProcessor(executionRootPathResolver)); } diff --git a/cpp/tests/integrationtests/com/google/idea/blaze/cpp/BlazeCppResolvingTestCase.java b/cpp/tests/integrationtests/com/google/idea/blaze/cpp/BlazeCppResolvingTestCase.java index 6c57ceb2f6d..8890b587946 100644 --- a/cpp/tests/integrationtests/com/google/idea/blaze/cpp/BlazeCppResolvingTestCase.java +++ b/cpp/tests/integrationtests/com/google/idea/blaze/cpp/BlazeCppResolvingTestCase.java @@ -94,6 +94,7 @@ private ExecutionRootPathResolver executionRootPathResolver(BlazeProjectData pro new BazelBuildSystemProvider(), workspaceRoot, projectData.getBlazeInfo().getExecutionRoot(), + projectData.getBlazeInfo().getOutputBase(), projectData.getWorkspacePathResolver()); } diff --git a/cpp/tests/unittests/com/google/idea/blaze/cpp/BlazeCompilerSettingsTest.java b/cpp/tests/unittests/com/google/idea/blaze/cpp/BlazeCompilerSettingsTest.java index 114bbd07e70..cad05489ab6 100644 --- a/cpp/tests/unittests/com/google/idea/blaze/cpp/BlazeCompilerSettingsTest.java +++ b/cpp/tests/unittests/com/google/idea/blaze/cpp/BlazeCompilerSettingsTest.java @@ -86,7 +86,7 @@ public void testCompilerSwitchesSimple() { } @Test - public void relativeSysroot_makesAbsolutePathInWorkspace() { + public void relativeSysroot_makesAbsolutePathInMainWorkspace() { ImmutableList cFlags = ImmutableList.of("--sysroot=third_party/toolchain/"); BlazeCompilerSettings settings = new BlazeCompilerSettings( @@ -118,7 +118,7 @@ public void absoluteSysroot_doesNotChange() { } @Test - public void relativeIsystem_makesAbsolutePathInExecRoot() { + public void relativeIsystem_makesAbsolutePathInWorkspaces() { ImmutableList cFlags = ImmutableList.of("-isystem", "external/arm_gcc/include", "-DFOO=1", "-Ithird_party/stl"); BlazeCompilerSettings settings = @@ -130,16 +130,36 @@ public void relativeIsystem_makesAbsolutePathInExecRoot() { cFlags, "cc version (trunk r123456)"); - String execRoot = blazeProjectData.getBlazeInfo().getExecutionRoot().toString(); + String outputBase = blazeProjectData.getBlazeInfo().getOutputBase().toString(); assertThat(settings.getCompilerSwitches(CLanguageKind.C, null)) .containsExactly( "-isystem", - execRoot + "/external/arm_gcc/include", + outputBase + "/external/arm_gcc/include", "-DFOO=1", "-I", workspaceRoot + "/third_party/stl"); } + @Test + public void relativeIquote_makesAbsolutePathInExecRoot() { + ImmutableList cFlags = + ImmutableList.of("-iquote", "bazel-out/android-arm64-v8a-opt/bin/external/boringssl"); + BlazeCompilerSettings settings = + new BlazeCompilerSettings( + getProject(), + new File("bin/c"), + new File("bin/c++"), + cFlags, + cFlags, + "cc version (trunk r123456)"); + + String execRoot = blazeProjectData.getBlazeInfo().getExecutionRoot().toString(); + assertThat(settings.getCompilerSwitches(CLanguageKind.C, null)) + .containsExactly( + "-iquote", + execRoot + "/bazel-out/android-arm64-v8a-opt/bin/external/boringssl"); + } + @Test public void absoluteISystem_doesNotChange() { ImmutableList cFlags = ImmutableList.of("-isystem", "/usr/include");