Symbol graph support for swiftbuild build system #8923
Conversation
Refactor the build system protocol to make symbol graphs and build plans optional build outputs
…ager into symbol_graph_extract
|
@swift-ci please test |
| outputRedirection: .collect(redirectStderr: true), | ||
| outputDirectory: symbolGraphDirectory, | ||
| verboseOutput: swiftCommandState.logLevel <= .info | ||
| if buildResult.symbolGraph { |
There was a problem hiding this comment.
It might be nice if the BuildResult could include the symbol graph paths themselves so we can compute those at the build system layer instead of in the dump command
|
@swift-ci please test |
|
@swift-ci test Windows |
1 similar comment
|
@swift-ci test Windows |
|
@swift-ci please test |
|
@swift-ci test Windows |
cmcgee1024
requested review from
AnthonyLatsis,
bnbarham,
xedin,
jakepetroules,
dschaefer2,
shawnhyam,
bripeticca,
plemarquand,
bkhouri and
daveyc123
as code owners
|
@swift-ci test Windows |
cmcgee1024
changed the title
| /// result for indication that the output was produced. | ||
| public enum BuildOutput { | ||
| case symbolGraph | ||
| // TODO associated values for the following symbol graph options: |
There was a problem hiding this comment.
I wonder if these extra flags should be captured by the requested outputs or the build parameters. Not sure I have a strong opinion right now but we should come up with a guideline to apply to future output types
|
@swift-ci please test |
|
@swift-ci test Windows |
|
@swift-ci please test |
|
@swift-ci test Windows |
| @@ -608,7 +608,7 @@ struct TraitTests { | |||
| #expect(symbolGraph.contains("TypeGatedByPackage10Trait1")) | |||
| #expect(symbolGraph.contains("TypeGatedByPackage10Trait2")) | |||
| } when: { | |||
| buildSystem == .swiftbuild | |||
| buildSystem == .swiftbuild && ProcessInfo.hostOperatingSystem == .windows | |||
There was a problem hiding this comment.
issue: does means this test currently fails for all platform with SwiftBuild. This seems like a regression from current behaviour.
There was a problem hiding this comment.
suggestion: consider migrating the test suite to Swift Testing so we can get feedback and test re-enablement once functionality is available.
| @@ -571,6 +571,7 @@ class PackageCommandTestCase: CommandsBuildProviderTestCase { | |||
| func testDumpSymbolGraphPrettyFormatting() async throws { | |||
| // Depending on how the test is running, the `swift-symbolgraph-extract` tool might be unavailable. | |||
| try XCTSkipIf((try? UserToolchain.default.getSymbolGraphExtract()) == nil, "skipping test because the `swift-symbolgraph-extract` tools isn't available") | |||
| try XCTSkipIf(buildSystemProvider == .swiftbuild, "skipping test because pretty printing isn't yet supported with swiftbuild build system via swift build and the swift compiler") | |||
There was a problem hiding this comment.
question: Can we reference a GitHub issue in the comment that will address the Swift Build pretty printing integration?
| @@ -123,14 +123,17 @@ struct APIDiff: AsyncSwiftCommand { | |||
| let apiDigesterTool = SwiftAPIDigester(fileSystem: swiftCommandState.fileSystem, tool: apiDigesterPath) | |||
|
|
|||
| // Build the current package. | |||
| try await buildSystem.build() | |||
| let buildResult = try await buildSystem.build(subset: .allExcludingTests, buildOutputs: [.buildPlan]) | |||
| guard let buildPlan = buildResult.buildPlan else { | |||
There was a problem hiding this comment.
question: From my understanding, a build plan is a native build system concept, but I believe the API Diff supports SwiftBuild Build System. Should we be aborting here? Also, should the destinationBuildParameter and toolsBuildParameters always be available as "build outputs"?
| // We are enabling all traits for dumping the symbol graph. | ||
| traitConfiguration: .init(enableAllTraits: true), | ||
| cacheBuildManifest: false | ||
| ) | ||
| try await buildSystem.build() | ||
| // TODO pass along the various flags as associated values to the symbol graph build output (e.g. includeSPISymbols) |
There was a problem hiding this comment.
question: do we have a GitHub issue tracking this TODO?
| continue | ||
| } | ||
|
|
||
| print("-- Emitting symbol graph for", description.module.name) |
There was a problem hiding this comment.
question: should we be using ObservabilityScope for emitting message to the console?
| try buildSystem.buildPlan.buildModules.first { | ||
| $0.module.name == moduleName && $0.buildParameters.destination == destination | ||
| // Build the target, if needed. We are interested in symbol graph (ideally) or a build plan. | ||
| // TODO pass along the options as associated values to the symbol graph build output (e.g. includeSPI) |
There was a problem hiding this comment.
question: Do we have a GitHub issue tracking this TODO?
| .split(whereSeparator: \.isNewline) | ||
| .first { String($0).hasPrefix("Files written to ") }? | ||
| .dropFirst(17) | ||
| try await withKnownIssue(isIntermittent: true, { |
There was a problem hiding this comment.
question: The test was previously passing on Windows, but we now marked it as intermittent. do we understand why?
Refactor the build system protocol to make symbol graphs and build plans optional build outputs.
Rewrite the dump-symbol-graph and the plugin delegate to use this new protocol and get support
from the swiftbuild build system.