-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix builtArtifacts for products from dependencies #9121
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
base: main
Are you sure you want to change the base?
fix builtArtifacts for products from dependencies #9121
Conversation
@@ -184,7 +184,7 @@ final class PluginDelegate: PluginInvocationDelegate { | |||
|
|||
var builtArtifacts: [PluginInvocationBuildResult.BuiltArtifact] = [] | |||
|
|||
for rootPkg in packageGraph.rootPackages { | |||
for rootPkg in packageGraph.packages { |
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.
After looking at a38e1d6?w=1#diff-9fb228cf9656e1f8d7cd26746626bc4d0952ca068ecfdbc544347ba3f1dc1894 I think you've correctly identified the root cause, but I'm not sure this is the right fix. Previously, this loop iterated over all the products in the build plan, and the build plan would include products from packages other than the roots. With this change, we iterate over all the products in the package graph, but I believe that will include products of dependency packages which were not built, and won't exist on disk. Maybe we should delegate this computation to the build system layer instead by adding a requirement to the BuildSystem
protocol similar to the existing var builtTestProducts: [BuiltTestProduct]
?
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.
Thanks for the feedback! I've managed to implement the the suggestion, but there's a problem for Xcode
and SwiftBuild
build systems. They don't offer a build plan so the builtTestProducts
and builtProducts
(which I added) cannot be computed the same way as we do for the native build system (which offers a build plan and build description). For example, builtTestProducts
are implemented in this way for Xcode build system
and SwiftBuild system
:
public var builtTestProducts: [BuiltTestProduct] {
get async {
do {
let graph = try await getPackageGraph()
var builtProducts: [BuiltTestProduct] = []
for package in graph.rootPackages {
for product in package.products where product.type == .test {
let binaryPath = try buildParameters.binaryPath(for: product)
builtProducts.append(
BuiltTestProduct(
productName: product.name,
binaryPath: binaryPath,
packagePath: package.path,
testEntryPointPath: product.underlying.testEntryPointPath
)
)
}
}
return builtProducts
} catch {
self.observabilityScope.emit(error)
return []
}
}
}
Which again leads us to the same issue of traversing the package graph.
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.
That's a good point. The "Xcode" build system backend has never supported plugins, and is being phased out in favor of "SwiftBuild", so that implementation can just return an empty list.
SwiftBuild doesn't have a build plan, but does expose similar information. SWBBuildService exposes a computeTargetDependencyGraph API which can tell us which underlying targets will be built for a given build request. It might take some additional bookkeeping to map those targets back to the appropriate SwiftPM artifacts though. I'll take a closer look sometime in the next few days and see if I can recommend an approach/see if there's new API we need to expose.
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.
Thanks!
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.
Need to run it through some more testing but I think swiftlang/swift-build#801 and #9143 should let us compute this correctly in the new build system while restoring the previous behavior of the old one - would be interested to get your thoughts
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.
looks good! I will updated this PR when those land.
Traverse all
packages
in package graph.Motivation:
As described in this issue this is an attempt to fix the regression.
Modifications:
Traverse all
packages
instead of onlyroot packages
to be able to populatebuiltArfifacts
from products from dependencies.Result:
Building products from dependencies inside command plugin works again.