Support swift package migrate with --build-system swiftbuild #8888
Conversation
owenv
requested review from
AnthonyLatsis,
bnbarham,
xedin,
jakepetroules,
dschaefer2,
shawnhyam,
bripeticca,
plemarquand,
bkhouri,
cmcgee1024 and
daveyc123
as code owners
|
@swift-ci test |
owenv
force-pushed
the
owenv/swift-build-migrate
branch
from
f82b6dc to
a396f96
Compare
| let fixItDuration: Duration | ||
| let targetsToUpdateInManifest: [String] | ||
| if buildSystem.supportsSerializedDaignosticsCollectionViaDelegate { | ||
| let diagnosticPaths = delegate.serializedDiagnosticsPathsByTarget |
There was a problem hiding this comment.
When I was originally thinking about this future when we have to support two implementations I hoped that it would be possible to expose target -> diagnostics as part of the BuildSystem protocol and abstract the implementation so we don't have to do these kind of checks the command itself...
There was a problem hiding this comment.
I reworked this a bit so that the serialized diagnostic paths are in a BuildResult returned by BuildSystem.build instead, it gets rid of the divergence here in Migrate.swift
There was a problem hiding this comment.
Nice, I like that approach very much! I left a comment about "name" but otherwise LGTM!
owenv
force-pushed
the
owenv/swift-build-migrate
branch
from
a396f96 to
d661ade
Compare
|
@swift-ci please test |
| self.serializedDiagnosticPathsByTargetName = serializedDiagnosticPathsByTargetName | ||
| } | ||
|
|
||
| public var serializedDiagnosticPathsByTargetName: Result<[String: [AbsolutePath]], Error> |
There was a problem hiding this comment.
I think there is a problem with using a "name" here as a string unless we can 100% make sure that it would distinguish between host and target builds of the same module (i.e. in BuildOperation we use module.name which won't make a distinction)...
There was a problem hiding this comment.
I think we need a test that uses a target as a plugin dependency to make sure that we don't override the diagnostics.
There was a problem hiding this comment.
I added another test here to ensure only one fix-it is applied even if the sources need to be built twice as a common dependency of host tools and target content. I think using the name here is in line with the fact that the list of targets may be user provided - names uniquely identify targets in the manifest model, just not once they've been lowered to targets building for a specific platform
There was a problem hiding this comment.
My concern is that we do allow passing flags like -Xswiftc-host or whatever it's spelled so there could be a difference in fix-its produced when building for host and destination, if we use simply a name instead of an identifier that includes target information we'd just override diagnostic files in that map.
There was a problem hiding this comment.
Right now they'd be concatenated instead of one overriding the other, so the entry corresponding to "Foo" includes the diagnostic paths for Foo when built for the host and those when built for the target, and we rely on SwiftFixIt to deduplicate them. I'm not sure we want to drop the host diagnostics entirely because that feels arbitrary imo.
There was a problem hiding this comment.
Oh I see, yeah, that sounds okay, SwiftFixIt should be able to handle that!
owenv
force-pushed
the
owenv/swift-build-migrate
branch
from
d661ade to
c60a191
Compare
|
@swift-ci test |
|
@swift-ci test Windows |
|
LGTM, thanks! |
… building target for both host and destination This is a limited cherry-pick of swiftlang#8888 For example if a target is a dependency of a plugin we build it for host and destination and produce the same fix-it twice and attempt to update manifest twice because uniquing is done on the ModuleBuildDescriptions. The changes do the following: - Coalesce all the diagnostic files for the same target regardless of host or destination build to make it possible for `SwiftFixIt` to filter duplicate fix-its; - Unique based on module identity to prevent duplicate setting insertion. Resolves: rdar://155569285
… building target for both host and destination This is a limited cherry-pick of swiftlang#8888 For example if a target is a dependency of a plugin we build it for host and destination and produce the same fix-it twice and attempt to update manifest twice because uniquing is done on the ModuleBuildDescriptions. The changes do the following: - Coalesce all the diagnostic files for the same target regardless of host or destination build to make it possible for `SwiftFixIt` to filter duplicate fix-its; - Unique based on module identity to prevent duplicate setting insertion. Resolves: rdar://155569285
… when… (#8929) … building target for both host and destination - Explanation: This is a limited cherry-pick of #8888 For example if a target is a dependency of a plugin we build it for host and destination and produce the same fix-it twice and attempt to update manifest twice because uniquing is done on the ModuleBuildDescriptions. The changes do the following: - Coalesce all the diagnostic files for the same target regardless of host or destination build to make it possible for`SwiftFixIt` to filter duplicate fix-its; - Unique based on module identity to prevent duplicate setting insertion. - Resolves: rdar://155569285 - Main Branch PR: #8888 - Risk: Very Low. This is a narrow fix for `swift package migrate` command. - Reviewed By: Me and @AnthonyLatsis reviewed the work in the main PR. - Testing: Added new test-cases to the suite.
Similar to the API digester integration, pass along a build system delegate to capture serialized diagnostic paths from the build and pass them to the fix-it application infra.
Builds on the changes in #8857, only the second commit is new