-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add prepare for index experimental build argument #7574
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
Changes from 9 commits
d470938
b0d7a86
0740be5
348c08d
26996d5
abd1224
b513d5a
c821697
1aa46c0
377bb69
c5e892a
64487b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,3 +18,4 @@ Package.resolved | |
.docc-build | ||
.vscode | ||
Utilities/InstalledSwiftPMConfiguration/config.json | ||
.devcontainer |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -582,6 +582,17 @@ public final class SwiftTargetBuildDescription { | |
args += ["-emit-module-interface-path", self.parseableModuleInterfaceOutputPath.pathString] | ||
} | ||
|
||
if self.defaultBuildParameters.prepareForIndexing { | ||
args += [ | ||
"-Xfrontend", "-enable-library-evolution", | ||
"-Xfrontend", "-experimental-skip-all-function-bodies", | ||
"-Xfrontend", "-experimental-lazy-typecheck", | ||
"-Xfrontend", "-experimental-skip-non-exportable-decls", | ||
"-Xfrontend", "-experimental-allow-module-with-compiler-errors", | ||
"-Xfrontend", "-empty-abi-descriptor" | ||
] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could also try out There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do. Currently just trying to get all the xplatform tests passing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow, those two chopped my previous prepare time in half for sourcekit-lsp which happened in 30 seconds versus 95 for the debug build. I had to enable-library-evolution for those to work, but that should be fine? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm, enabling library evolution would result in different diagnostics (eg. need to add
Just to be clear, 95s is the full debug build and a prepare used to be 60s without these and is now 30s? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Correct. For sourcekit-lsp. I've been bouncing around. I need to do a more complete benchmark on a few different projects to get a better picture. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, sounds like we could probably gate the library evolution requirement check on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've lifted the restriction so that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, thanks @tshortli! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am still getting
in the latest 6.0 toolchain from May 26. Not sure this change got in after that build? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would have been just after: |
||
} | ||
|
||
args += self.defaultBuildParameters.toolchain.extraFlags.swiftCompilerFlags | ||
// User arguments (from -Xswiftc) should follow generated arguments to allow user overrides | ||
args += self.defaultBuildParameters.flags.swiftCompilerFlags | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,7 +41,7 @@ extension LLBuildManifestBuilder { | |
let inputs = try self.computeSwiftCompileCmdInputs(target) | ||
|
||
// Outputs. | ||
let objectNodes = try target.objects.map(Node.file) | ||
let objectNodes = target.defaultBuildParameters.prepareForIndexing ? [] : try target.objects.map(Node.file) | ||
let moduleNode = Node.file(target.moduleOutputPath) | ||
let cmdOutputs = objectNodes + [moduleNode] | ||
|
||
|
@@ -397,7 +397,8 @@ extension LLBuildManifestBuilder { | |
fileList: target.sourcesFileListPath, | ||
isLibrary: isLibrary, | ||
wholeModuleOptimization: target.defaultBuildParameters.configuration == .release, | ||
outputFileMapPath: try target.writeOutputFileMap() // FIXME: Eliminate side effect. | ||
outputFileMapPath: try target.writeOutputFileMap(), // FIXME: Eliminate side effect. | ||
prepareForIndexing: target.defaultBuildParameters.prepareForIndexing | ||
) | ||
} | ||
|
||
|
@@ -417,6 +418,8 @@ extension LLBuildManifestBuilder { | |
inputs.append(resourcesNode) | ||
} | ||
|
||
let prepareForIndexing = target.defaultBuildParameters.prepareForIndexing | ||
|
||
func addStaticTargetInputs(_ target: ResolvedModule) throws { | ||
// Ignore C Modules. | ||
if target.underlying is SystemLibraryTarget { return } | ||
|
@@ -428,7 +431,7 @@ extension LLBuildManifestBuilder { | |
if target.underlying is ProvidedLibraryTarget { return } | ||
|
||
// Depend on the binary for executable targets. | ||
if target.type == .executable { | ||
if target.type == .executable && !prepareForIndexing { | ||
// FIXME: Optimize. | ||
let product = try plan.graph.allProducts.first { | ||
try $0.type == .executable && $0.executableTarget.id == target.id | ||
|
@@ -446,8 +449,16 @@ extension LLBuildManifestBuilder { | |
case .swift(let target)?: | ||
inputs.append(file: target.moduleOutputPath) | ||
case .clang(let target)?: | ||
for object in try target.objects { | ||
inputs.append(file: object) | ||
if prepareForIndexing { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you elaborate on why this is necessary? This is handling a clang target, supposedly preparation doesn't apply here at all and these object files are produced by other clang targets? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the normal case this adds a dependency on the object files from the clang modules to force a rebuild when they change. In preparation, there are no object files generated so this was a haphazard attempt to propagate the dependencies to their sources. This should actually be the headers. Let me give this one a bit more thought. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After giving this more thought, I need to keep the clang commands around so I can map the swiftmodule dependencies on C source files so that the modules get re-prepared when the C files change. I'll just replace them with phony commands so they don't actually get built. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After looking at the clang commands, I realized that the header dependencies are coming from the generated .d files. Since I'm not running the compiler they won't be there. So my original idea of adding the dependencies on the headers instead of the object files seems to be working. |
||
// In preparation, we're only building swiftmodules | ||
// propagate the dependency to the header files in this target | ||
for header in target.clangTarget.headers { | ||
inputs.append(file: header) | ||
} | ||
} else { | ||
for object in try target.objects { | ||
inputs.append(file: object) | ||
} | ||
} | ||
case nil: | ||
throw InternalError("unexpected: target \(target) not in target map \(self.plan.targetMap)") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -437,6 +437,9 @@ public struct BuildOptions: ParsableArguments { | |
@Flag(help: "Enable or disable indexing-while-building feature") | ||
public var indexStoreMode: StoreMode = .autoIndexStore | ||
|
||
@Flag(name: .customLong("experimental-prepare-for-indexing")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be a good idea to have this hidden (noticed in swiftlang/sourcekit-lsp#1373) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. This option isn't really useful to humans :). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
var prepareForIndexing: Bool = false | ||
dschaefer2 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/// Whether to enable generation of `.swiftinterface`s alongside `.swiftmodule`s. | ||
@Flag(name: .customLong("enable-parseable-module-interfaces")) | ||
public var shouldEnableParseableModuleInterfaces: Bool = false | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
// Copyright (C) 2024 Apple Inc. All rights reserved. | ||
// | ||
// This document is the property of Apple Inc. | ||
// It is considered confidential and proprietary. | ||
// | ||
// This document may not be reproduced or transmitted in any form, | ||
// in whole or in part, without the express written permission of | ||
// Apple Inc. | ||
dschaefer2 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
import Build | ||
import Foundation | ||
import LLBuildManifest | ||
@_spi(SwiftPMInternal) | ||
import SPMTestSupport | ||
import TSCBasic | ||
import XCTest | ||
|
||
class PrepareForIndexTests: XCTestCase { | ||
func testPrepare() throws { | ||
let (graph, fs, scope) = try macrosPackageGraph() | ||
|
||
let plan = try BuildPlan( | ||
destinationBuildParameters: mockBuildParameters(prepareForIndexing: true), | ||
toolsBuildParameters: mockBuildParameters(prepareForIndexing: false), | ||
graph: graph, | ||
fileSystem: fs, | ||
observabilityScope: scope | ||
) | ||
|
||
let builder = LLBuildManifestBuilder(plan, fileSystem: fs, observabilityScope: scope) | ||
let manifest = try builder.generatePrepareManifest(at: "/manifest") | ||
|
||
// Make sure we're building the swift modules | ||
let outputs = manifest.commands.flatMap(\.value.tool.outputs).map(\.name) | ||
XCTAssertTrue(outputs.contains(where: { $0.hasSuffix(".swiftmodule")})) | ||
|
||
// Ensure swiftmodules built with correct arguments | ||
let coreCommands = manifest.commands.values.filter({ | ||
$0.tool.outputs.contains(where: { | ||
$0.name.hasSuffix("debug/Core.build/Core.swiftmodule") | ||
}) | ||
}) | ||
XCTAssertEqual(coreCommands.count, 1) | ||
let coreSwiftc = try XCTUnwrap(coreCommands.first?.tool as? SwiftCompilerTool) | ||
XCTAssertTrue(coreSwiftc.otherArguments.contains("-experimental-skip-all-function-bodies")) | ||
|
||
// Ensure tools are built normally | ||
let toolCommands = manifest.commands.values.filter({ | ||
$0.tool.outputs.contains(where: { | ||
$0.name.hasSuffix("debug/Modules-tool/SwiftSyntax.swiftmodule") | ||
}) | ||
}) | ||
XCTAssertEqual(toolCommands.count, 1) | ||
let toolSwiftc = try XCTUnwrap(toolCommands.first?.tool as? SwiftCompilerTool) | ||
XCTAssertFalse(toolSwiftc.otherArguments.contains("-experimental-skip-all-function-bodies")) | ||
|
||
// Make sure only object files for tools are built | ||
XCTAssertTrue(outputs.filter({ $0.hasSuffix(".o") }).allSatisfy({ $0.contains("-tool.build/")}), | ||
"outputs:\n\t\(outputs.filter({ $0.hasSuffix(".o") }).joined(separator: "\n\t"))" | ||
) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
-enable-library-evolution
needed here now?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was waiting for an update toolchain. There's one there now. Should be ready to remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind updating the diff accordingly before merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. That will be the last piece. Once I can make sure it works 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the only thing blocking the PR from being merged, I’d prefer to get the PR merged with
-enable-library-evolution
still set because it could unblock me to test preparation in sourcekit-lsp. I know that I’ll get spurious warnings because of library evolution but I can ignore those for now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. As mentioned above, I'm not even sure if the change that was made even fixes it. I have one more fix Ben B mentioned about hiding the flag and then I'll kick off the tests.