Skip to content

Add swiftRegexBuilder dependency on Windows #1264

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

Closed
wants to merge 1 commit into from
Closed

Add swiftRegexBuilder dependency on Windows #1264

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Sources/FoundationInternationalization/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should be universal.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a build failure somewhere that you saw that prompted the need to add this?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Here is a build on swift CI that hits the issue:
https://ci-external.swift.org/job/swift-corelibs-libdispatch-PR-windows/132/console

Copy link
Author

Choose a reason for hiding this comment

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

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

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.

FoundationEssentials
_FoundationCShims
_FoundationICU)
Expand Down