Remove setting ICUCORE for macOS in System.Globalization.Native CMake config #39900
Conversation
… config Follow-up to dotnet#39833. We don't link against ICU on macOS but load it via `dlopen()` at runtime and compilation uses headers from homebrew. That means `HAVE_SET_MAX_VARIABLE` and `HAVE_UDAT_STANDALONE_SHORTER_WEEKDAYS` will always be defined and we don't need the ICUCORE variable.
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
| if(ICUCORE STREQUAL ICUCORE-NOTFOUND) | ||
| message(FATAL_ERROR "Cannot find libicucore, skipping build for System.Globalization.Native. .NET globalization is not expected to function.") | ||
| return() | ||
| endif() | ||
| add_definitions(-DOSX_ICU_LIBRARY_PATH=\"/usr/lib/libicucore.dylib\") |
There was a problem hiding this comment.
Thank you for the cleanup!
a micro-nit, since we don't have surrounding quotes, looks like we do not need to the escape the single pair:
| add_definitions(-DOSX_ICU_LIBRARY_PATH=\"/usr/lib/libicucore.dylib\") | |
| add_definitions(-DOSX_ICU_LIBRARY_PATH="/usr/lib/libicucore.dylib") |
There was a problem hiding this comment.
nit: Is it possible the build can produce error with nice message when /usr/lib/libicucore.dylib is missing for any reason? I know it should exist but I don't know if anyone can customize on their machines.
There was a problem hiding this comment.
@tarekgh actually the file won't exist on disk in the next macOS release:
from https://developer.apple.com/documentation/macos-release-notes/macos-big-sur-11-beta-release-notes
New in macOS Big Sur 11 beta, the system ships with a built-in dynamic linker cache of all system-provided libraries. As part of this change, copies of dynamic libraries are no longer present on the filesystem. Code that attempts to check for dynamic library presence by looking for a file at a path or enumerating a directory will fail. Instead, check for library presence by attempting to dlopen() the path, which will correctly check for the library in the cache. (62986286)
I verified that you can still dlopen() the library on a macOS 11 system so our usage should be fine.
There was a problem hiding this comment.
@akoeplinger thanks for the info.
This make me wonder now if the app local ICU will work there. @safern could you please track testing this?
There was a problem hiding this comment.
This make me wonder now if the app local ICU will work there. @safern could you please track testing this?
app local ICU also does a dlopen on the full path of the library using the NATIVE_DLL_SEARCH_DIRECTORIES.
There was a problem hiding this comment.
No, because the way that macOS ships ICU is with a single binary which is libicucore.dylib -- for AppLocal we try and load libicuuc.dylib.<version>.dylib and libicui18n.<version>.dylib we never probe for libicucore without a version.
Do you still think we have that problem? If so I can test it but I don't think it would clash because of the way macOS distributes ICU.
There was a problem hiding this comment.
Do you still think we have that problem?
If you are confident enough there will not be any issue, then I am fine with that.
There was a problem hiding this comment.
I think it would be an issue only if the user manually added libicuuc and libicui18n with the same version in the system search paths.
There was a problem hiding this comment.
I think at that time these will not be in the system cache and shouldn't get loaded. anyway, if you get chance later, give it a try and look how things work on Big Sur
There was a problem hiding this comment.
Yeah, I will. Thanks for raising concern.
Co-authored-by: Adeel Mujahid <[email protected]>
|
It seems like this fixes: #39583 ? |
|
@safern yeah I think it should fix that one too. |
Thanks for including that on your cleanup 😄 -- updated the description to close the issue. |
|
GitHub checks didn't update, build is green on AzDO. |
… config (dotnet#39900) Follow-up to dotnet#39833. We don't link against ICU on macOS but load it via `dlopen()` at runtime and compilation uses headers from homebrew. That means `HAVE_SET_MAX_VARIABLE` and `HAVE_UDAT_STANDALONE_SHORTER_WEEKDAYS` will always be defined and we don't need the ICUCORE variable. * PR feedback Co-authored-by: Adeel Mujahid <[email protected]>
ghost
locked as resolved and limited conversation to collaborators
Follow-up to #39833.
Fixes: #39583
We don't link against ICU on macOS but load it via
dlopen()at runtime and compilation uses headers from homebrew.That means
HAVE_SET_MAX_VARIABLEandHAVE_UDAT_STANDALONE_SHORTER_WEEKDAYSwill always be defined and we don't need the ICUCORE variable./cc @am11