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.

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

Handle generated resources from plugins #4306

Merged
merged 1 commit into from Apr 22, 2022
Merged

Handle generated resources from plugins #4306

merged 1 commit into from Apr 22, 2022

Conversation

neonichu
Copy link
Member

rdar://89693335

@neonichu neonichu added the WIP Work in progress label Apr 19, 2022
@neonichu
Copy link
Member Author

Biggest question I have: since we don't support generated sources for Clang targets, yet, I am assuming we do the same for resources for now and handle support for Clang targets separately?

@neonichu
Copy link
Member Author

Oh, there's one major limitation right now: because we do not retain information from the manifest about non-existent files during the initial pass of TargetSourcesBuilder, this will only work for built-in resource types.

Maybe we should treat any unknown resources as to be processed right now? The main reason we don't include anything but certain known types automatically was that we wanted to prevent people from accidentally shipping files with their products, but since any files here are basically already opt-in through depending on a given plugin, that might not be a concern?

@abertelrud
Copy link
Contributor

Oh, there's one major limitation right now: because we do not retain information from the manifest about non-existent files during the initial pass of TargetSourcesBuilder, this will only work for built-in resource types.

Because unknown files are vended to the plugin, I did actually extend TargetSourcesBuilder to carry these through and pass them to the plugin rather than warn about them correctly — so we should be seeing those files now.

Maybe we should treat any unknown resources as to be processed right now? The main reason we don't include anything but certain known types automatically was that we wanted to prevent people from accidentally shipping files with their products, but since any files here are basically already opt-in through depending on a given plugin, that might not be a concern?

To do this completely correctly I think would involve reworking TargetSourcesBuilder to be able to work piecemeal on sets of files, and to also keep more information from the manifest (or pass it in each time). But I do think there is great value in even supporting less flexible situations, so always processing the resource files makes sense for now.

@abertelrud
Copy link
Contributor

Biggest question I have: since we don't support generated sources for Clang targets, yet, I am assuming we do the same for resources for now and handle support for Clang targets separately?

Yes, I think that makes sense. It's unfortunate that there's such a split between Clang targets and Swift targets in SwiftPM, causing overlap for the things that aren't difference. But I think it's a good idea to tackle this piecemeal, as you're doing.

@neonichu neonichu force-pushed the plugin-generated-resources branch 3 times, most recently from 98499b4 to 4650e37 Compare April 19, 2022 20:41
@neonichu
Copy link
Member Author

OK, I think this is basically ready for review now, just need to add some tests.

@neonichu
Copy link
Member Author

@swift-ci please smoke test

abertelrud
abertelrud approved these changes Apr 19, 2022
View reviewed changes
Copy link
Contributor

@abertelrud abertelrud left a comment

Choose a reason for hiding this comment

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

LGTM (modulo the need for tests as you mentioned). Thank you for taking this on!

@neonichu
Copy link
Member Author

I moved the big switch statement into TargetSourcesBuilder now, so that clients just call a single API and automatically get the correct handling, also taking into account the tools version. This seems preferable to burying this logic in the build system.

@neonichu
Copy link
Member Author 

/private/var/folders/pf/p5g2vb2j0yx1x6vlmxdppc700000gn/T/Miscellaneous_PluginGeneratedResources.qrxDtx/Miscellaneous_PluginGeneratedResources/.build/plugins/cache/Generator>, <output:
'dyld: lazy symbol binding failed: can'\''t resolve symbol _$ss13_runAsyncMainyyyyYaKcF in /private/var/folders/pf/p5g2vb2j0yx1x6vlmxdppc700000gn/T/Miscellaneous_PluginGeneratedResources.qrxDtx/Miscellaneous_PluginGeneratedResources/.build/plugins/cache/Generator because dependent dylib /usr/lib/swift/libswift_Concurrency.dylib could not be loaded
dyld: can'\''t resolve symbol _$ss13_runAsyncMainyyyyYaKcF in /private/var/folders/pf/p5g2vb2j0yx1x6vlmxdppc700000gn/T/Miscellaneous_PluginGeneratedResources.qrxDtx/Miscellaneous_PluginGeneratedResources/.build/plugins/cache/Generator because dependent dylib /usr/lib/swift/libswift_Concurrency.dylib could not be loaded
'>

@neonichu
Copy link
Member Author

Self-hosted is running macOS 11.5.2, so we shouldn't be trying to find the concurrency libs in the OS, I think. Also odd that the smoke test passes just fine. Maybe an issue with the superior toolchain?

@neonichu
Copy link
Member Author

It is using

swift --version
swift-driver version: 1.26.7 Apple Swift version 5.5 (swiftlang-1300.0.27.6 clang-1300.0.27.2)
Target: x86_64-apple-macosx11.0

So maybe we'll just disable the test for < 5.6

@neonichu
Copy link
Member Author

@swift-ci please smoke test

@neonichu
Copy link
Member Author

@swift-ci please smoke test

@abertelrud
Copy link
Contributor

It is using

swift --version
swift-driver version: 1.26.7 Apple Swift version 5.5 (swiftlang-1300.0.27.6 clang-1300.0.27.2)
Target: x86_64-apple-macosx11.0

So maybe we'll just disable the test for < 5.6

Oh I see! Yes, this needs at least 5.6.

tomerd
tomerd reviewed Apr 22, 2022
View reviewed changes
Copy link
Member

@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.

looks good, some style nits

@neonichu
Handle generated resources from plugins
dfefce8 
Up until now, we were treating all generated files as Swift source files. With this, we are involving `TargetSourcesBuilder` in the decision of how to treat a certain file, provided the tools version is 5.7 or newer.

Remaining limitations:
- generated files aren't treated the same way as non-generated ones, e.g. we will treat anything unknown as a resource to be processed and we won't respect declarations from the manifest
- files in Clang targets aren't treated at all at the moment

rdar://89693335
@neonichu
Copy link
Member Author

@swift-ci please smoke test

@neonichu
Copy link
Member Author

@swift-ci please smoke test linux

@neonichu neonichu merged commit 0147f71 into main Apr 22, 2022
@neonichu neonichu deleted the plugin-generated-resources branch April 22, 2022 20:52
@neonichu neonichu mentioned this pull request Apr 22, 2022
Merged
@MaxDesiatov MaxDesiatov mentioned this pull request May 12, 2022
Merged
MaxDesiatov added a commit to swiftwasm/JavaScriptKit that referenced this pull request May 12, 2022
@MaxDesiatov @kateinoigakukun
Supply JSKit runtime in SwiftPM resources (#193)
e022311 
Due to the fact that apple/swift-package-manager#4306 will only be available starting with Swift 5.7, we can't generate these resources with a SwiftPM plugin. I propose storing the generated code in the repository directly for now. When 5.7 is released, we can switch to a SwiftPM plugin approach.

Can be tested end-to-end with swiftwasm/carton#335.

Co-authored-by: Yuta Saito <kateinoigakukun@gmail.com>
@MaxDesiatov MaxDesiatov mentioned this pull request May 12, 2022
Open
@neonichu neonichu mentioned this pull request Feb 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomerd tomerd tomerd left review comments

@abertelrud abertelrud abertelrud approved these changes

@elsh elsh Awaiting requested review from elsh

Assignees

@neonichu neonichu

Labels
ready Author believes the PR is ready to be merged & any feedback has been addressed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@neonichu @abertelrud @tomerd