Hardcode path to ICU on macOS #39833
Conversation
| @@ -28,7 +28,7 @@ if(CLR_CMAKE_TARGET_UNIX) | |||
| 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=\"${ICUCORE}\") | |||
| add_definitions(-DOSX_ICU_LIBRARY_PATH=\"/usr/lib/libicucore.dylib\") | |||
There was a problem hiding this comment.
nit: does setting CMAKE_FIND_LIBRARY_SUFFIXES=.dylib before the introspection line above (find_library) and unsetting it after the introspection works? That will save us using the hard-coded path.
There was a problem hiding this comment.
e.g.
There was a problem hiding this comment.
let's do that in a follow-up PR to unblock CI.
There was a problem hiding this comment.
it might, but I see no reason why we should not use the hardcoded path since that is where ICU will always be on macOS.
There was a problem hiding this comment.
That defeats the purpose of using find_library (the call above). :)
There was a problem hiding this comment.
Is CMAKE_REQUIRED_LIBRARIES setting to libicucore.tbd considered ok? maybe we need to change that as well; .tbd is a metadata (YAML) file.
Even better, if we could avoid introducing more hardcoded paths using set(CMAKE_FIND_LIBRARY_SUFFIXES .dylib), it will at least keep it consistent with other usages of find_* macros (under platform conditions or platform specific cmake files), IMHO. e.g. in theory, someone can build libicu locally on macOS (for experiments) and have it installed in other place present in PATH.
There was a problem hiding this comment.
doesn't seem to be needed though, will send a PR to remove it ;)
There was a problem hiding this comment.
Perhaps this is what you're discussing here, but from https://gitlab.kitware.com/cmake/cmake/-/issues/21007
The .tbd files in the SDK are the preferred way to link to system libraries on macOS. This is in preparation for macOS 11 where the /usr/lib/*.dylib files do not even exist on disk.
Is it correct that /usr/lib/libicucore.dylib does not exist on Big Sur? If so will this change break there?
cc @ViktorHofer
There was a problem hiding this comment.
If this doesn't exist in Big Sur I will hit it soon as I will be fixing: #39583
So if that's the case I will also fix this to work on Big Sur.
There was a problem hiding this comment.
@danmosemsft @safern I commented on the cmake gitlab issue, but copying here:
I checked on a macOS 11 system and the file is indeed not on disk but you can apparently still load it via dlopen() so we should still be able to use it.
They probably do some tricks in the dynamic loader to allow loading libicucore.dylib even though it doesn't exist at that location on disk. We don't know whether Apple will keep that or remove at some later point though.
|
To sum up the findings, we're 99% sure this is because of the recent cmake upgrade to 3.18 that got pushed to brew with Homebrew/homebrew-core@7927380. A build that ran 8hours ago (presumably with cmake 3.17.2) still got the expected |
Co-authored-by: Alexander Köplinger <[email protected]>
|
Filed an issue with CMake: https://gitlab.kitware.com/cmake/cmake/-/issues/21007 |
… 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.
… config (#39900) Follow-up to #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]>
… 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
For some reason the path ends up being
/Applications/Xcode_11.3.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk/usr/lib/libicucore.tbdinstead of the expected/usr/lib/libicucore.dylibrecently on the build machines.Fixes https://github.com/dotnet/core-eng/issues/10304
EDIT:
To sum up the findings, this is because of the recent cmake upgrade to 3.18 that got pushed to brew with Homebrew/homebrew-core@7927380.
A build that ran 8hours ago (presumably with cmake 3.17.2) still got the expected
/usr/lib/libicucore.dylib, a build from 4hours ago got the Xcode path.