Skip to content
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.

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

[C++][Gandiva] MSVC_VERSION not handled correctly #36594

Closed
h-vetinari opened this issue Jul 10, 2023 · 0 comments · Fixed by #36595
Closed

[C++][Gandiva] MSVC_VERSION not handled correctly #36594

h-vetinari opened this issue Jul 10, 2023 · 0 comments · Fixed by #36595

Comments

@h-vetinari
Copy link
Contributor

h-vetinari commented Jul 10, 2023

Describe the bug, including details regarding any error messages, version, and platform.

The following logic is deficient:

if(MSVC)
# clang pretends to be a particular version of MSVC. Thestandard
# library uses C++14 features, so we have to use that -std version
# to get the IR compilation to work.
# See https://cmake.org/cmake/help/latest/variable/MSVC_VERSION.html
# for MSVC_VERSION and Visual Studio version.
if(MSVC_VERSION LESS 1930)
set(FMS_COMPATIBILITY 19.20)
elseif(MSVC_VERSION LESS 1920)
set(FMS_COMPATIBILITY 19.10)
else()
message(FATAL_ERROR "Unsupported MSVC_VERSION=${MSVC_VERSION}")
endif()
set(PLATFORM_CLANG_OPTIONS -std=c++17 -fms-compatibility
-fms-compatibility-version=${FMS_COMPATIBILITY})

It will never reach the elseif or the else (as it just decides between <1930, yes or no), and will pass an empty FMS_COMPATIBILITY for VS2022.

Also worth noting, elsewhere MSVC_VERSION uses 19, and not 19xx. This is also wrong according to the CMake docs.

Component(s)

C++ - Gandiva

@kou kou changed the title [gandiva] MSVC_VERSION not handled correctly [C++][Gandiva] MSVC_VERSION not handled correctly Jul 10, 2023
kou added a commit to kou/arrow that referenced this issue Jul 10, 2023
kou added a commit to kou/arrow that referenced this issue Aug 11, 2023
kou added a commit to kou/arrow that referenced this issue Oct 11, 2023
kou added a commit that referenced this issue Oct 19, 2023
…-version (#36595)

### Rationale for this change

`MSVC_VERSION` isn't used correctly. 

### What changes are included in this PR?

* Remove MSVC 2013 or earlier check because it reached EOL
* Don't change `-fms-compatibility-version` depending on `MSVC_VERSION`
   * Always use the lowest supported Visual Studio version for it

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* Closes: #36594

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@kou kou added this to the 15.0.0 milestone Oct 19, 2023
JerAguilon pushed a commit to JerAguilon/arrow that referenced this issue Oct 23, 2023
…bility-version (apache#36595)

### Rationale for this change

`MSVC_VERSION` isn't used correctly. 

### What changes are included in this PR?

* Remove MSVC 2013 or earlier check because it reached EOL
* Don't change `-fms-compatibility-version` depending on `MSVC_VERSION`
   * Always use the lowest supported Visual Studio version for it

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* Closes: apache#36594

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
…bility-version (apache#36595)

### Rationale for this change

`MSVC_VERSION` isn't used correctly. 

### What changes are included in this PR?

* Remove MSVC 2013 or earlier check because it reached EOL
* Don't change `-fms-compatibility-version` depending on `MSVC_VERSION`
   * Always use the lowest supported Visual Studio version for it

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* Closes: apache#36594

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
…bility-version (apache#36595)

### Rationale for this change

`MSVC_VERSION` isn't used correctly. 

### What changes are included in this PR?

* Remove MSVC 2013 or earlier check because it reached EOL
* Don't change `-fms-compatibility-version` depending on `MSVC_VERSION`
   * Always use the lowest supported Visual Studio version for it

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* Closes: apache#36594

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants