[Plugins] Include Plugins in `Manifest.targetsRequired` #3623

fourplusone · 2021-07-19T21:13:28Z

Include Plugins in Manifest.targetsRequired

Motivation:

Fixes SR-14343. As Plugins are modeled as a dependency they also must be included the list of required targets for a product.

Modifications:

Changed the implementation of Manifest.targetsRequired to include plugins

Result:

SR-14343 is fixed

Fixes SR-14343. As Plugins are modeled as a dependency they also must be included the list of required targets for a product.

neonichu · 2021-07-20T17:17:18Z

@swift-ci please smoke test

tomerd · 2021-07-20T18:10:58Z

Sources/PackageModel/Manifest.swift

+
+                let plugins: [String] = target.pluginUsages?.compactMap { pluginUsage in
+                    switch pluginUsage {
+                    case .plugin(name: let name, package: nil):


should this be .plugin(name: let name, _):?

My understanding is, that if package is not nil .plugin(...) declares a dependency to a target in another package. Therefore it's not relevant in this context

neonichu · 2021-07-20T22:31:28Z

10:23:08 Test Suite 'PackageCollectionsTests' started at 2021-07-20 10:21:53.935
10:23:08 Test Case '-[PackageCollectionsTests.PackageCollectionsTests testCollectionPendingTrustConfirmIsKeptOnAdd]' started.
10:23:08 Test Case '-[PackageCollectionsTests.PackageCollectionsTests testCollectionPendingTrustConfirmIsKeptOnAdd]' passed (0.105 seconds).
10:23:08 Exited with signal code 4

Maybe interesting for @yim-lee

neonichu · 2021-07-20T22:31:41Z

@swift-ci please smoke test macOS

yim-lee · 2021-07-21T01:24:33Z

https://ci.swift.org/job/swift-package-manager-with-xcode-self-hosted-PR-osx/2239/console

Hard to tell what happened with PackageCollectionsTests. Will keep an eye on it in case it happens again.

This should help prevent it: #3629

Motivation: Some of the package collection tests don't require search and finish quickly, so when background thread tries to `populateTargetTrie` it results in signal 4. For [example](swiftlang#3623 (comment)): ``` 10:23:08 Test Suite 'PackageCollectionsTests' started at 2021-07-20 10:21:53.935 10:23:08 Test Case '-[PackageCollectionsTests.PackageCollectionsTests testCollectionPendingTrustConfirmIsKeptOnAdd]' started. 10:23:08 Test Case '-[PackageCollectionsTests.PackageCollectionsTests testCollectionPendingTrustConfirmIsKeptOnAdd]' passed (0.105 seconds). 10:23:08 Exited with signal code 4 ``` Modifications: Set `initializeTargetTrie` to `false` for these tests such that `populateTargetTrie` is not called.

Motivation: Some of the package collection tests don't require search and finish quickly, so when background thread tries to `populateTargetTrie` it results in signal 4. For [example](#3623 (comment)): ``` 10:23:08 Test Suite 'PackageCollectionsTests' started at 2021-07-20 10:21:53.935 10:23:08 Test Case '-[PackageCollectionsTests.PackageCollectionsTests testCollectionPendingTrustConfirmIsKeptOnAdd]' started. 10:23:08 Test Case '-[PackageCollectionsTests.PackageCollectionsTests testCollectionPendingTrustConfirmIsKeptOnAdd]' passed (0.105 seconds). 10:23:08 Exited with signal code 4 ``` Modifications: Set `initializeTargetTrie` to `false` for these tests such that `populateTargetTrie` is not called.

Motivation: Some of the package collection tests don't require search and finish quickly, so when background thread tries to `populateTargetTrie` it results in signal 4. For [example](swiftlang#3623 (comment)): ``` 10:23:08 Test Suite 'PackageCollectionsTests' started at 2021-07-20 10:21:53.935 10:23:08 Test Case '-[PackageCollectionsTests.PackageCollectionsTests testCollectionPendingTrustConfirmIsKeptOnAdd]' started. 10:23:08 Test Case '-[PackageCollectionsTests.PackageCollectionsTests testCollectionPendingTrustConfirmIsKeptOnAdd]' passed (0.105 seconds). 10:23:08 Exited with signal code 4 ``` Modifications: Set `initializeTargetTrie` to `false` for these tests such that `populateTargetTrie` is not called.

* [Collections] Disable target trie in some of the tests Motivation: Some of the package collection tests don't require search and finish quickly, so when background thread tries to `populateTargetTrie` it results in signal 4. For [example](#3623 (comment)): ``` 10:23:08 Test Suite 'PackageCollectionsTests' started at 2021-07-20 10:21:53.935 10:23:08 Test Case '-[PackageCollectionsTests.PackageCollectionsTests testCollectionPendingTrustConfirmIsKeptOnAdd]' started. 10:23:08 Test Case '-[PackageCollectionsTests.PackageCollectionsTests testCollectionPendingTrustConfirmIsKeptOnAdd]' passed (0.105 seconds). 10:23:08 Exited with signal code 4 ``` Modifications: Set `initializeTargetTrie` to `false` for these tests such that `populateTargetTrie` is not called. * [Collections] Don't queue populateTargetTrie if there is no data Motivation: `populateTargetTrie`, potentially because it's done in a background queue, causes a lot of random crashes in tests (not just in SwiftPM but other projects as well). The exception happens inside SQLite C library and it's not yet clear what the real cause is. However, since most tests do not use the package collections feature at all, `populateTargetTrie` is basically a no-op and therefore doesn't need to be queued. Modification: Queue `populateTargetTrie` only if there is package collection data. This code change doesn't actually fix anything but may perhaps reduce the chance of crashing. rdar://80840989

abertelrud · 2021-08-02T19:57:35Z

Thanks for the fix, @fourplusone! The fix looks good... it would be great to also have a unit test for it. There are some test fixtures for plugins in the Fixtures/Miscellaneous/Plugins subdirectory that could be useful for this purpose.

elsh · 2021-08-16T21:34:36Z

Thanks for the fix, @fourplusone! The fix looks good... it would be great to also have a unit test for it. There are some test fixtures for plugins in the Fixtures/Miscellaneous/Plugins subdirectory that could be useful for this purpose.

@fourplusone do you need any additional info on adding unit tests?

abertelrud · 2021-09-08T04:44:07Z

Extended a unit test to cover this. It's at abertelrud@c7fd9ec. @fourplusone, do you mind if I add that to this branch?

fourplusone · 2021-09-08T07:02:19Z

@abertelrud Feel free to add it 🙂

…lved # Conflicts: # Tests/PackageModelTests/ManifestTests.swift

abertelrud · 2021-09-08T16:46:16Z

@abertelrud Feel free to add it 🙂

Great, thanks! I pushed a commit extending an existing test to cover this case. Thank you for the fix!

abertelrud · 2021-09-08T16:46:22Z

@swift-ci please smoke test

abertelrud · 2021-09-09T23:09:27Z

@swift-ci please smoke test linux

abertelrud · 2021-09-13T16:53:13Z

Looks like all the tests pass and the reviews are in place, so merging this. Thank you for the fix!

[Plugins] Include Plugins in Manifest.targetsRequired

6fe1e3f

Fixes SR-14343. As Plugins are modeled as a dependency they also must be included the list of required targets for a product.

fourplusone requested review from abertelrud, friedbunny, neonichu and tomerd as code owners July 19, 2021 21:13

tomerd assigned abertelrud Jul 19, 2021

neonichu approved these changes Jul 20, 2021

View reviewed changes

tomerd reviewed Jul 20, 2021

View reviewed changes

yim-lee mentioned this pull request Jul 21, 2021

[Collections] Disable target trie in some of the tests #3629

Merged

abertelrud approved these changes Aug 2, 2021

View reviewed changes

Extend a unit test to cover target requirements when plugins are invo…

1a6721f

…lved # Conflicts: # Tests/PackageModelTests/ManifestTests.swift

abertelrud merged commit d5d3b92 into swiftlang:main Sep 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Plugins] Include Plugins in `Manifest.targetsRequired` #3623

[Plugins] Include Plugins in `Manifest.targetsRequired` #3623

Uh oh!

fourplusone commented Jul 19, 2021

Uh oh!

neonichu commented Jul 20, 2021

Uh oh!

tomerd Jul 20, 2021

Uh oh!

fourplusone Jul 20, 2021 •

edited

Loading

Uh oh!

neonichu commented Jul 20, 2021

Uh oh!

neonichu commented Jul 20, 2021

Uh oh!

yim-lee commented Jul 21, 2021 •

edited

Loading

Uh oh!

abertelrud commented Aug 2, 2021

Uh oh!

elsh commented Aug 16, 2021

Uh oh!

abertelrud commented Sep 8, 2021

Uh oh!

fourplusone commented Sep 8, 2021

Uh oh!

abertelrud commented Sep 8, 2021

Uh oh!

abertelrud commented Sep 8, 2021

Uh oh!

abertelrud commented Sep 9, 2021

Uh oh!

abertelrud commented Sep 13, 2021

Uh oh!

Uh oh!

[Plugins] Include Plugins in Manifest.targetsRequired #3623

[Plugins] Include Plugins in Manifest.targetsRequired #3623

Uh oh!

Conversation

fourplusone commented Jul 19, 2021

Motivation:

Modifications:

Result:

Uh oh!

neonichu commented Jul 20, 2021

Uh oh!

tomerd Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

fourplusone Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neonichu commented Jul 20, 2021

Uh oh!

neonichu commented Jul 20, 2021

Uh oh!

yim-lee commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abertelrud commented Aug 2, 2021

Uh oh!

elsh commented Aug 16, 2021

Uh oh!

abertelrud commented Sep 8, 2021

Uh oh!

fourplusone commented Sep 8, 2021

Uh oh!

abertelrud commented Sep 8, 2021

Uh oh!

abertelrud commented Sep 8, 2021

Uh oh!

abertelrud commented Sep 9, 2021

Uh oh!

abertelrud commented Sep 13, 2021

Uh oh!

Uh oh!

[Plugins] Include Plugins in `Manifest.targetsRequired` #3623

[Plugins] Include Plugins in `Manifest.targetsRequired` #3623

fourplusone Jul 20, 2021 •

edited

Loading

yim-lee commented Jul 21, 2021 •

edited

Loading