Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve external paths to their workspaces #4063

Merged
merged 2 commits into from
Feb 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -31,22 +31,26 @@
* Converts execution-root-relative paths to absolute files with a minimum of file system calls
* (typically none).
*
* <p>Files which exist both underneath the execution root and within the workspace will be resolved
* to workspace paths.
* <p>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 {

private final ImmutableList<String> 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;
}

Expand All @@ -61,6 +65,7 @@ public static ExecutionRootPathResolver fromProject(Project project) {
Blaze.getBuildSystemProvider(project),
WorkspaceRoot.fromProject(project),
projectData.getBlazeInfo().getExecutionRoot(),
projectData.getBlazeInfo().getOutputBase(),
projectData.getWorkspacePathResolver());
}

Expand All @@ -73,49 +78,55 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I'm not sure if I get it correctly, why do we need to modify resolveExecutionRootPath method? Isn't it enough just to modify resolveToIncludeDirectories? And is it possible that ExecutionRootPath starts from "external"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey hey! Thanks for taking another look, @tpasternak.

I'd think we'd need to modify execution root paths--and not just directories--because one might try to browse to an external file path. More generally, those execroot paths are unstable for all files, not just directories, so I'd think we'd always want to point to the stable paths, lest they disappear under the editor when a build gets run that uses different external workspaces. This ended up being important for Xcode/VSCode, anyway. I think this is a case we want to defend against. (Unless I'm misunderstanding your question; you all are the experts, immersed in this day-to-day.)

// 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 path.getFileRootedAt(executionRoot);
// Else, in main workspace
return workspacePathResolver.resolveToFile(path.getAbsoluteOrRelativeFile().getPath());
}

/**
* 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<File> 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(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");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -44,14 +45,15 @@ 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<File> 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
Expand All @@ -64,7 +66,7 @@ public void testGenfilesPathRelativeToExecRoot() {
}

@Test
public void testNonOutputPathsRelativeToWorkspaceRoot() {
public void testMainWorkspacePathsRelativeToWorkspaceRoot() {
ImmutableList<File> files =
pathResolver.resolveToIncludeDirectories(new ExecutionRootPath("tools/fast"));
assertThat(files).containsExactly(WORKSPACE_ROOT.fileForPath(new WorkspacePath("tools/fast")));
Expand Down
1 change: 1 addition & 0 deletions cpp/src/com/google/idea/blaze/cpp/BlazeCWorkspace.java
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ private OCWorkspaceImpl.ModifiableModel calculateConfigurations(
Blaze.getBuildSystemProvider(project),
workspaceRoot,
blazeProjectData.getBlazeInfo().getExecutionRoot(),
blazeProjectData.getBlazeInfo().getOutputBase(),
blazeProjectData.getWorkspacePathResolver());

int progress = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ public BlazeConfigurationResolverResult update(
Blaze.getBuildSystemProvider(project),
WorkspaceRoot.fromProject(project),
blazeProjectData.getBlazeInfo().getExecutionRoot(),
blazeProjectData.getBlazeInfo().getOutputBase(),
blazeProjectData.getWorkspacePathResolver());
ImmutableMap<TargetKey, CToolchainIdeInfo> toolchainLookupMap =
BlazeConfigurationToolchainResolver.buildToolchainLookupMap(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public Optional<BlazeCompilerFlagsProcessor> getProcessor(Project project) {
Blaze.getBuildSystemProvider(project),
workspaceRoot,
projectData.getBlazeInfo().getExecutionRoot(),
projectData.getBlazeInfo().getOutputBase(),
projectData.getWorkspacePathResolver());
return Optional.of(new IncludeRootFlagsProcessor(executionRootPathResolver));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ private ExecutionRootPathResolver executionRootPathResolver(BlazeProjectData pro
new BazelBuildSystemProvider(),
workspaceRoot,
projectData.getBlazeInfo().getExecutionRoot(),
projectData.getBlazeInfo().getOutputBase(),
projectData.getWorkspacePathResolver());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public void testCompilerSwitchesSimple() {
}

@Test
public void relativeSysroot_makesAbsolutePathInWorkspace() {
public void relativeSysroot_makesAbsolutePathInMainWorkspace() {
ImmutableList<String> cFlags = ImmutableList.of("--sysroot=third_party/toolchain/");
BlazeCompilerSettings settings =
new BlazeCompilerSettings(
Expand Down Expand Up @@ -118,7 +118,7 @@ public void absoluteSysroot_doesNotChange() {
}

@Test
public void relativeIsystem_makesAbsolutePathInExecRoot() {
public void relativeIsystem_makesAbsolutePathInWorkspaces() {
ImmutableList<String> cFlags =
ImmutableList.of("-isystem", "external/arm_gcc/include", "-DFOO=1", "-Ithird_party/stl");
BlazeCompilerSettings settings =
Expand All @@ -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<String> 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<String> cFlags = ImmutableList.of("-isystem", "/usr/include");
Expand Down