Skip to content

Don't link plugin dependencies to products #6695

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

Jump to bottom
Merged
merged 1 commit into from
Jul 18, 2023
Merged

Don't link plugin dependencies to products #6695

merged 1 commit into from
Jul 18, 2023

Conversation

neonichu
Copy link
Contributor

@neonichu neonichu commented Jul 11, 2023 •
edited
Loading

This fixes two issues with plugins and their dependencies:
- we were traversing plugin products and including their targets as part of any product
- since we are including plugin targets as dependencies as well, we could traverse plugin targets in a similarly incorrect way

@neonichu neonichu self-assigned this Jul 11, 2023
@neonichu
Copy link
Contributor Author

@swift-ci please smoke test

@neonichu
Copy link
Contributor Author

neonichu commented Jul 11, 2023 •
edited
Loading

Seems like the tests are passing, so maybe this was really just a mistake.

@rauhul
Copy link
Member

rauhul commented Jul 11, 2023

Locally confirmed fixes #6673 (which I initially mis-diagnosed)

@neonichu neonichu force-pushed the dont-link-plugin-dependencies branch from 2024a70 to 27c9f48 Compare July 11, 2023 23:35
@neonichu neonichu marked this pull request as ready for review July 11, 2023 23:36
@neonichu
Copy link
Contributor Author

@swift-ci please smoke test

@neonichu neonichu changed the title WIP: Don't link plugin dependencies to products Don't link plugin dependencies to products Jul 11, 2023
@neonichu
Copy link
Contributor Author

@swift-ci please smoke test windows

@neonichu
Copy link
Contributor Author

I'd like to add some test coverage for this and we probably also want this fix in 5.9. I am not 100% sure how plugins ever worked with these issues in place 😬

abertelrud
abertelrud reviewed Jul 12, 2023
View reviewed changes
@neonichu neonichu force-pushed the dont-link-plugin-dependencies branch from 27c9f48 to c5c6777 Compare July 13, 2023 04:51
@neonichu
Copy link
Contributor Author

I do wonder whether we need to put this fix behind the 5.9 tools-version? At least in theory, it seems like the bug could be load bearing if dependencies ended up being unexpressed because of this. By definition, any package that currently ships couldn't have suffered major consequences from this bug, otherwise it would have been possible to ship said package.

@neonichu
Copy link
Contributor Author

@swift-ci please smoke test

@neonichu
Copy link
Contributor Author

/private/var/folders/7n/r31lzfjx6556jgc6vg55_cg80000gn/T/Miscellaneous_Plugins.tJQMsU/IncorrectDependencies/Sources/MyPluginExecutable/MyPluginExecutable.swift:3:24: error: concurrency is only available in macOS 10.15.0 or newer
static func main() async throws {

Hm, this is legitimate, but I wonder why this issue didn't show up locally?

@neonichu
Copy link
Contributor Author

@swift-ci please smoke test

@neonichu
Copy link
Contributor Author

@swift-ci please smoke test windows

tomerd
tomerd approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@tomerd tomerd left a comment

Choose a reason for hiding this comment

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

small suggestion to improve code readability

@neonichu
Don't link plugin dependencies to products
96b7594 
This fixes two issues with plugins and their dependencies:
- we were traversing plugin products and including their targets as part of any product
- since we are including plugin targets as dependencies as well, we could traverse plugin targets in a similarly incorrect way
@neonichu neonichu force-pushed the dont-link-plugin-dependencies branch from 2d793cd to 96b7594 Compare July 17, 2023 19:51
@neonichu
Copy link
Contributor Author

@swift-ci please smoke test

@neonichu
Copy link
Contributor Author

@swift-ci please smoke test windows

@neonichu
Copy link
Contributor Author

Windows build failed with

0.13 sec
          Start 1325: TestFoundation.TestProcess-test_exit1
Build timed out (after 60 minutes). Marking the build as aborted.

Seems like this isn't entirely a fluke...

@neonichu
Copy link
Contributor Author

@swift-ci please smoke test windows

@neonichu neonichu merged commit 1e72ef8 into main Jul 18, 2023
@neonichu neonichu deleted the dont-link-plugin-dependencies branch July 18, 2023 02:40
@neonichu neonichu mentioned this pull request Jul 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abertelrud abertelrud abertelrud left review comments

@tomerd tomerd tomerd approved these changes

@rauhul rauhul rauhul approved these changes

@MaxDesiatov MaxDesiatov Awaiting requested review from MaxDesiatov

Assignees

@neonichu neonichu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@neonichu @rauhul @tomerd @abertelrud