Skip to content
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

[SR-14782] swift package describe is omitting product dependencies from per-target output #3556

Conversation

abertelrud
Copy link
Contributor

While swift package describe had an entry for target_dependencies in the output of each target, it omitted product_dependencies.

Motivation:

Currently (while we still have the distinction between products and targets in the SwiftPM package model) we should include both kinds of dependencies in the package description output format.

Modifications:

  • add product dependencies to the per-target outputs (because of how this is done, the same code automatically works for both JSON and plain text output)

…from per-target output

While `swift package describe` had an entry for `target_dependencies` in the output of each target, it omitted `product_dependencies`.

rdar://79358923
@abertelrud
Copy link
Contributor Author

@swift-ci please smoke test

@@ -254,6 +254,21 @@ final class PackageToolTests: XCTestCase {
XCTAssert(textChunk6.contains("Path: Sources/CExec"), textChunk6)
XCTAssert(textChunk6.contains("Sources:\n main.c"), textChunk6)
}

fixture(name: "DependencyResolution/External/Simple/Bar") { prefix in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This uses an existing test fixture so we don't need to add more of them.

let targetDependencies = target.dependencies.compactMap{ $0.target }
self.targetDependencies = targetDependencies.isEmpty ? nil : targetDependencies.map{ $0.name }
let productDependencies = target.dependencies.compactMap{ $0.product }
self.productDependencies = productDependencies.isEmpty ? nil : productDependencies.map{ $0.name }
Copy link
Member

Choose a reason for hiding this comment

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

cc @yim-lee for potential impact on collection generator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory this should have no impact since it's additive, but that's a good point. I guess this would have the effect of omitting the target_dependencies empty array for a target that had only product dependencies, in addition to adding the product_dependencies array. I don't know if product dependencies is something that you might want to start including in the collections at some point — I don't think that's in the data structures now (are target dependencies there now?).

Copy link
Member

Choose a reason for hiding this comment

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

👍 thanks.

The generator is not using describe yet (but will), so this change has no impact. I don't think we will include *_dependencies in the package collection data, but IIRC the intention is that it will help determine which targets to include (i.e., only targets included in a product).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. That's in the separate product_memberships array, and unaffected by this.

@abertelrud abertelrud requested a review from yim-lee June 16, 2021 00:07
@abertelrud abertelrud added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready Author believes the PR is ready to be merged & any feedback has been addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants