Don’t cause any file system file effects when trying to find an implicit workspace for a file #1350
Conversation
|
@swift-ci Please test |
ahoppen
force-pushed
the
no-side-effects-when-searching-for-workspace
branch
from
d31440c to
e66a30a
Compare
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
1 similar comment
|
@swift-ci Please test Windows |
| let buildSystem: BuildSystem | ||
| if let defaultBuildSystem { | ||
| buildSystem = defaultBuildSystem | ||
| } else if let buildServer = await createBuildServerBuildSystem(rootPath: rootPath) { | ||
| buildSystem = buildServer | ||
| } else if let swiftpm = await createSwiftPMBuildSystem(rootUrl: rootUrl) { | ||
| buildSystem = swiftpm | ||
| } else if let compdb = createCompilationDatabaseBuildSystem(rootPath: rootPath) { | ||
| buildSystem = compdb | ||
| } else { | ||
| logger.error("Could not set up a build system at '\(rootUri.forLogging)'") | ||
| return nil | ||
| } | ||
| return buildSystem |
There was a problem hiding this comment.
Could turn these into early returns now
| let projectRoot = await buildSystem.projectRoot | ||
| return await buildSystem.fileHandlingCapability(for: uri) == .handled && !projectRoots.contains(projectRoot) |
There was a problem hiding this comment.
Could we reject !projectRoots.contains(projectRoot) cases sooner, even before we attempt to create the workspace? Could the build system root ever differ from the workspace root?
There was a problem hiding this comment.
We do reject !projectRoots.contains(projectRoot) before we create the workspace because it’s part of the satisfying closure passed to createWorkspace.
We cannot perform this check before generating the build graph because we projectRoot is a property of buildSystem. And when you create a SwiftPMBuildSystem, for the Sources folder, I think it creates a build system with project root set to the folder that contains the Package.swift.
But we can hoist this check to be before the build graph generation, which makes sense.
There was a problem hiding this comment.
Just to clarify, I meant rejecting before the call to createWorkspace, based on the workspace URL. But yeah looks like the build system root can differ from the workspace. Doing it before the build graph generation makes sense 👍
…cit workspace for a file When looking for a workspace that can handle a file, we were creating full-fledged workspaces along the way, which we would then discard if they couldn’t handle the file being opened. This had multiple problems: 1. When background indexing is enabled, it caused semantic indexing of the workspace, which wrote files to a `.index-build` directory and was a waste of work 2. When background indexing is enabled, it caused package resolution, which also created a `.index-build` folder to be created 3. It caused a syntactic test index of the workspace, which was a waste of work. To fix this, do multiple things: 1. When creating a workspace, add a check right after build system creation. This allows us to early exit if the build system can’t handle the file and prevents us from generating the `Workspace`, fixing (1) and (3) 2. Don’t call `reloadPackage` when creating a `SwiftPMWorkspace`. Instead, explicitly call `generateBuildGraph` once we committed to creating the workspace.
ahoppen
force-pushed
the
no-side-effects-when-searching-for-workspace
branch
from
e66a30a to
f85d821
Compare
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
1 similar comment
|
@swift-ci Please test Windows |
When looking for a workspace that can handle a file, we were creating full-fledged workspaces along the way, which we would then discard if they couldn’t handle the file being opened. This had multiple problems:
.index-builddirectory and was a waste of work.index-buildfolder to be createdTo fix this, do multiple things:
Workspace, fixing (1) and (3)reloadPackagewhen creating aSwiftPMWorkspace. Instead, explicitly callgenerateBuildGraphonce we committed to creating the workspace.rdar://128682978