Skip to content

GH-50449: [C++] Invalid version script argument supplied to MSVC linker #50450

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

Open
johnwparent wants to merge 2 commits into
base: main
Choose a base branch
from
+4 −0
Open

GH-50449: [C++] Invalid version script argument supplied to MSVC linker #50450

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3b3d0dd
GH-50449: Invalid version script argument supplied to MSVC linker
johnwparent Jul 9, 2026
4271176
Apply suggestions from code review
johnwparent Jul 9, 2026
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
4 changes: 4 additions & 0 deletions cpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,10 @@ if(CMAKE_VERSION VERSION_LESS 3.18)
else()
set(CXX_LINKER_SUPPORTS_VERSION_SCRIPT TRUE)
endif()
Comment on lines 359 to 364

@kou kou Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you remove this because we require CMake 3.25 or later now?

elseif(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")

@kou kou Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
elseif(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
elseif(MSVC)

@johnwparent johnwparent Jul 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@kou kou Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All MSVC like compilers behave like #50450 (comment) , right? (All symbols are hidden by default.)

If so, can we use elseif(MSVC) here?

# msvc does not support version scripts, but check_linker_flag won't return false
Comment thread
johnwparent marked this conversation as resolved.
Outdated
# due to how msvc's linker reports unrecognized arguments
Comment thread
johnwparent marked this conversation as resolved.
Outdated
set(CXX_LINKER_SUPPORTS_VERSION_SCRIPT FALSE)
else()
include(CheckLinkerFlag)
check_linker_flag(CXX
Expand Down
Loading