GH-50449: [C++] Invalid version script argument supplied to MSVC linker#50450
GH-50449: [C++] Invalid version script argument supplied to MSVC linker#50450johnwparent wants to merge 2 commits into
Conversation
Signed-off-by: John Parent <john.parent@kitware.com>
kou
changed the title
kou
left a comment
kou
left a comment
There was a problem hiding this comment.
Interesting.
If we use LINKER:-version-script=... instead of -Wl,--version-script=..., does check_linker_flag() with MSVC report FALSE?
If check_linker_flag() with MSVC doesn't report FALSE for any flags, I feel that it's a problem of CMake. Could you report it to CMake?
| else() | ||
| set(CXX_LINKER_SUPPORTS_VERSION_SCRIPT TRUE) | ||
| endif() | ||
| elseif(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC") |
There was a problem hiding this comment.
| elseif(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC") | |
| elseif(MSVC) |
There was a problem hiding this comment.
I let this be the more restrictive condition as I wasn't sure if any "MSVC like" compilers would support the version file, I'm not aware of any that do, so I think this suggestion is probably fine, but I'm not aware of the scope of Arrow's deployment, and just wanted to point out there are side effects from changing this check beyond terseness, just to be safe.
There was a problem hiding this comment.
All MSVC like compilers behave like #50450 (comment) , right? (All symbols are hidden by default.)
If so, can we use elseif(MSVC) here?
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
It's an inherent behavior of MSVC's linker, but it does look like that module attempts to catch diagnostic output. I'll see if we're interested in a patch. Another question, that I didn't both to ask for Spack, but is relevant to this patch, is do we want to control symbol visibility with MSVC the same way the version script is doing here? In which case we can write a .def or similar file instead of skipping this entirely. |
github-actions
Bot
added
awaiting committer review
Yes. It's better. We don't want to export symbols for bundled dependencies because they may be conflicted with other products that use Apache Arrow C++. |
Unless you're re-exporting this symbols deliberately, for DLLs with MSVC, you don't need to worry, the transitive symbols are hidden by default (all symbols are hidden by default with MSVC) |
|
OK. Then we don't need this feature for MSVC. |
| if(CMAKE_VERSION VERSION_LESS 3.18) | ||
| if(APPLE OR WIN32) | ||
| set(CXX_LINKER_SUPPORTS_VERSION_SCRIPT FALSE) | ||
| else() | ||
| set(CXX_LINKER_SUPPORTS_VERSION_SCRIPT TRUE) | ||
| endif() |
There was a problem hiding this comment.
Could you remove this because we require CMake 3.25 or later now?
2 participants
Rationale for this change
Preventing an invalid argument to be supplied to the MSVC linker, which can potentially break arrow builds on paths with spaces on Windows when using MSVC.
Example of failing build without this patch: https://gitlab.spack.io/spack/spack-packages/-/jobs/22969196
What changes are included in this PR?
cpp/CMakeLists.txthas been updated to add a specific check for the MSVC toolchain, and if true, setsCXX_LINKER_SUPPORTS_VERSION_SCRIPTtoFALSE.Are these changes tested?
Yes, see the build of
arrowin this CI run in Spack's built with this patch and a path with a space: https://gitlab.spack.io/spack/spack-packages/-/jobs/22970695Are there any user-facing changes?
No
Alternatively, the
check_linker_flagcould be turned into atry_compilewith a-wallflag (and the MSVC equivalent) turned on, or the path to the version script could be properly quoted on all platforms, but this feels like the most robust solutions with the smallest impact.