swiftlang / swift-installer-scripts

Apache License 2.0
73 stars 38 forks source link

Update Windows/Android installer scripts for swift-foundation re-core #312

Closed jmschonfeld closed 4 months ago

jmschonfeld commented 5 months ago

This updates the Windows/Android scripts to account for the Foundation re-core which involves:

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)

jmschonfeld commented 5 months ago

It is unfortunate that we need these headers to be distributed. Is the installed set audited in the CMake rules?

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 internal imports and shouldn't be used directly by clients. Could you elaborate on a second piece? Our CMake files do list these headers explicitly if that's what you mean.

I wonder if we should clean this up in the future with some heat rules to crawl the directory.

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.

compnerd commented 5 months ago

It is unfortunate that we need these headers to be distributed. Is the installed set audited in the CMake rules?

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 internal imports and shouldn't be used directly by clients. Could you elaborate on a second piece? Our CMake files do list these headers explicitly if that's what you mean.

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.

I wonder if we should clean this up in the future with some heat rules to crawl the directory.

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.

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 /usr/lib/swift with the Windows toolchain, and it is a shame to add them here.

jmschonfeld commented 5 months ago

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.

Sounds good

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.

I'm not sure I like the structure here though - we have avoid any headers in /usr/lib/swift with the Windows toolchain, and it is a shame to add them here.

I believe with the current setup, these modules are installed alongside the existing dispatch, Block, and os modules. AFAICT I don't think we need any changes to the compiler since those modules already exist in the search path and in my toolchain builds in CI clients of Foundation are able to build fine. I might be missing something but since from what I can tell we already do have headers in /usr/lib/swift/dispatch, etc. is there something that's customized for those modules elsewhere that we'd need to replicate for /usr/lib/swift/_foundation_unicode and /usr/lib/swift/_FoundationCShims? I'm open to suggestions on better places to put these modules if we deem that's better, but it'd be unfortunate to have to block the re-core on a compiler change if that's what's necessary so perhaps we could revisit this once we land the change if that's a requirement?

FWIW this is where we're currently looking at putting these modules on Linux, since /usr/lib/swift/CoreFoundation is already where the CoreFoundation module exists on Linux today in Swift 5.10 (but I do see that the CoreFoundation module doesn't get installed on Windows so I know that's not exactly the same situation here).

compnerd commented 5 months ago

I believe with the current setup, these modules are installed alongside the existing dispatch, Block, and os modules. AFAICT I don't think we need any changes to the compiler since those modules already exist in the search path and in my toolchain builds in CI clients of Foundation are able to build fine. I might be missing something but since from what I can tell we already do have headers in /usr/lib/swift/dispatch, etc. is there something that's customized for those modules elsewhere that we'd need to replicate for /usr/lib/swift/_foundation_unicode and /usr/lib/swift/_FoundationCShims? I'm open to suggestions on better places to put these modules if we deem that's better, but it'd be unfortunate to have to block the re-core on a compiler change if that's what's necessary so perhaps we could revisit this once we land the change if that's a requirement?

FWIW this is where we're currently looking at putting these modules on Linux, since /usr/lib/swift/CoreFoundation is already where the CoreFoundation module exists on Linux today in Swift 5.10 (but I do see that the CoreFoundation module doesn't get installed on Windows so I know that's not exactly the same situation here).

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 CoreFoundation module on Windows as it was late to the release and ended up sealing that exposure as well. Linux was unable to get away with it as it had shipped for quite a bit. This is why the headers are not present, but also means that the header search path does not need to be adjusted.

jmschonfeld commented 5 months ago

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 CoreFoundation module on Windows as it was late to the release and ended up sealing that exposure as well. Linux was unable to get away with it as it had shipped for quite a bit. This is why the headers are not present, but also means that the header search path does not need to be adjusted.

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 _foundation_unicode and _FoundationCShims are any different than dispatch in this case. The Dispatch swift module (which is public API) has a public import of the CDispatch clang module (which is the module installed into the usr/include/dispatch folder). If clients of Dispatch's swift module can find the CDispatch module in the dispatch folder under usr/include, is there a reason why clients of FoundationInternationalization wouldn't be able to find the _FoundationICU module in usr/include/_foundation_unicode? I agree it's unfortunate we have to install these modules but until (if) we enable library evolution for all the Foundation modules I think that's a requirement we'll need to live with. To me it doesn't sound like we'd truly need compiler changes to support this since by nature of how dispatch is setup I believe these paths already need to be in the header search path but hopefully once I have a toolchain built with these changes I can validate that's true

jmschonfeld commented 4 months ago

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

compnerd commented 4 months ago

I think that the android changes are not entirely correct - we do want the renaming to $(ArchTriple).swift* as that is how the compiler looks up the modules (using the ....swift{doc,interface,module}/$(ArchTriple).swift{doc,interface,module}). I think that re-core is more important though. We can work to fix the android builds subsequently, but lets get the re-coring completed, if that is what it takes to fix the output.