Add swiftRegexBuilder dependency on Windows #1264
Conversation
Following swiftlang#1198 and swiftlang#1262, this is needed to build on Windows.
| @@ -37,6 +37,7 @@ target_compile_options(FoundationInternationalization PRIVATE ${_SwiftFoundation | |||
| target_compile_options(FoundationInternationalization PRIVATE -package-name "SwiftFoundation") | |||
|
|
|||
| target_link_libraries(FoundationInternationalization PUBLIC | |||
| $<$<PLATFORM_ID:Windows>:swiftRegexBuilder> | |||
There was a problem hiding this comment.
I wonder if this should be universal.
There was a problem hiding this comment.
I'm not sure if I fully understand why this is required. We don't actually have any direct dependencies on any other swift targets so this one shouldn't be any different - we just get them all from the toolchain, we don't actually depend on cmake targets for any of the stdlib modules. If this is related to the current CI failure where plutil fails to link, I have a different fix in-flight that I'm about to push that doesn't require adding this dependency
There was a problem hiding this comment.
I was not certain and I am assuming this was tested on macOS. Since I don't know how this was tested on macOS, I decided to limit it to Windows out of an abundance of caution.
There was a problem hiding this comment.
Is there a build failure somewhere that you saw that prompted the need to add this?
There was a problem hiding this comment.
The plutil linking failure related to an implicitly imported symbol from RegexBuilder is being resolved at swiftlang/swift-corelibs-foundation#5204 if that's the failure that prompted this
There was a problem hiding this comment.
Following #1265, there is still one missing symbol:
ICUDateFormatter.swift.obj : error LNK2019: unresolved external symbol __imp_$sSS17_StringProcessing14RegexComponent0C7BuilderMc referenced in function $sS2S17_StringProcessing14RegexComponent0C7BuilderWl
Duration+UnitsFormatStyle.swift.obj : error LNK2001: unresolved external symbol __imp_$sSS17_StringProcessing14RegexComponent0C7BuilderMc
TimeZone_ICU.swift.obj : error LNK2001: unresolved external symbol __imp_$sSS17_StringProcessing14RegexComponent0C7BuilderMc
bin\FoundationInternationalization.dll : fatal error LNK1120: 1 unresolved externals
There was a problem hiding this comment.
Are there any other linking errors / do you have a full build log I can help look at? It looks like those are just type metadata symbols and I'm not certain why the binary would still be referencing the conformance symbols when it no longer calls the function that requires the conformance (unless there's still another call somewhere in there)
There was a problem hiding this comment.
Just merged #1268 as well, which should resolve this problem for now. We'll continue to investigate how to land the original change without requiring client modules to link the RegexBuilder library
There was a problem hiding this comment.
Here is a build on swift CI that hits the issue:
https://ci-external.swift.org/job/swift-corelibs-libdispatch-PR-windows/132/console
There was a problem hiding this comment.
Just merged #1268 as well, which should resolve this problem for now. We'll continue to investigate how to land the original change without requiring client modules to link the
RegexBuilderlibrary
Thank you, there are other issues on swift-ci that prevent checking for the fix but I can confirm that the issue is fixed with our CI with these changes.
There was a problem hiding this comment.
I also just merged #1265 which avoids the link to regex builder on all platforms since the usage isn't Windows specific. So I think any issues should be resolved here now, but let me know if you continue to see any fallout of the change. We shouldn't need to link regex builder from this module since it shouldn't be using it (currently) and if we do I don't think depending on the cmake target will work to make that happen.
Just saw your comment. I am going to start a new build with your changes and update you. |
Following #1198 and #1262, this is needed to build on Windows.