Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix builtArtifacts for products from dependencies #9121

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Sources/Commands/Utilities/PluginDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ final class PluginDelegate: PluginInvocationDelegate {

var builtArtifacts: [PluginInvocationBuildResult.BuiltArtifact] = []

for rootPkg in packageGraph.rootPackages {
for rootPkg in packageGraph.packages {
Copy link
Contributor

@owenv owenv Sep 15, 2025 •
edited
Loading

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]?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@owenv just tested the your changes that got merged and everything works! thanks a lot! I think we can close this PR and also the corresponding issue.

let builtProducts = rootPkg.products.filter {
switch subset {
case .all(let includingTests):
Expand Down