-
Notifications
You must be signed in to change notification settings - Fork 43
Update Windows/Android installer scripts for swift-foundation re-core #312
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is fine for now. It is unfortunate that we need these headers to be distributed. Is the installed set audited in the CMake rules? I wonder if we should clean this up in the future with some heat rules to crawl the directory.
Yeah unfortunately the headers need to be distributed since we don't build Foundation with library evolution enabled so the modules need to be present in the SDK even though they're always
We'd love to not need to come update this list every time we add/remove/rename a header so if there's anything we can do going forward to have it copy the whole directory rather than list files explicitly we'd definitely be on board with that. |
I believe that we might need some tweaks to the compiler as these headers are not part of the default header search path, so this might cause a regression for use of Foundation.
If the files are being audited in CMake and are isolated per module, we might be able to do something. I need to play around with that some, so for now, let's leave this. I'm not sure I like the structure here though - we have avoid any headers in |
Sounds good
I believe with the current setup, these modules are installed alongside the existing FWIW this is where we're currently looking at putting these modules on Linux, since |
On Windows the layout is slightly different than Linux. However, my understanding from various conversations about dispatch, the C API is not intended to be part of the user surface, and so requiring the additional header search path to be added is not considered a problem. The headers being required will mean that the user will have to change the flags to make any code using Foundation continue to build. There is no |
I don't have a toolchain yet to confirm (working out some SwiftPM issues and then once that's resolved hopefully I can verify this with a toolchain in practice) but I don't think that |
@compnerd I know you approved this before but given the above, any additional concerns about this? We were able to successfully produce a functional toolchain and swift-ci testing passes with these changes, so I don't think we need any compiler changes to support this. Any other issues with merging this once the PR to swift-corelibs-foundation has passed tests to confirm it all works? |
I think that the android changes are not entirely correct - we do want the renaming to |
* Update Android installer scripts for swift-foundation re-core * _FoundationCollections is a combined module not files in an arch-specific dir * _FoundationCollections is named by ArchArchDir not ArchTriple (cherry picked from commit 29fadd0)
This updates the Windows/Android scripts to account for the Foundation re-core which involves:
_foundation_unicode
and_FoundationCShims
added to the SDK (with.lib
files on Windows)_FoundationCollections.swiftmodule
,FoundationEssentials.swiftmodule
, andFoundationInternationalization.swiftmodule
added to the SDK (with.lib
files on Windows)_FoundationICU
,FoundationEssentials
, andFoundationInternationalization
Additionally, this updates the paths for
Foundation
/FoundationXML
/FoundationNetworking
to account for the new structure of the swift module (a directory with per-arch files rather than files put into per-arch directories)