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

[R][C++] Cmake error Unknown compiler: IntelLLVM 2023.2.0 #37750

Closed
thisisnic opened this issue Sep 15, 2023 · 7 comments · Fixed by #37781
Closed

[R][C++] Cmake error Unknown compiler: IntelLLVM 2023.2.0 #37750

thisisnic opened this issue Sep 15, 2023 · 7 comments · Fixed by #37781
Assignees
Milestone

Comments

@thisisnic
Copy link
Member

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

When trying to build the C++ library from source on one of the CRAN machines, we get the following error:

-- Building using CMake version: 3.26.3
-- The C compiler identification is IntelLLVM 2023.2.0
-- The CXX compiler identification is IntelLLVM 2023.2.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler:
/data/gannet/ripley/intel/oneapi/compiler/2023.2.1/linux/bin/icx - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler:
/data/gannet/ripley/intel/oneapi/compiler/2023.2.1/linux/bin/icpx - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Arrow version: 13.0.0 (full: '13.0.0')
-- Arrow SO version: 1300 (full: 1300.0.0)
-- clang-tidy found at /usr/bin/clang-tidy
-- clang-format found at /usr/bin/clang-format
-- Found ClangTools: /usr/bin/clang-format
-- infer not found
-- Found Python3: /usr/bin/python3.10 (found version "3.10.11") found
components: Interpreter
fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
-- Found cpplint executable at
/tmp/RtmpAEH6nP/R.INSTALL27b65d4fc79fd5/arrow/tools/cpp/build-support/cpplint.py
-- System processor: x86_64
-- Performing Test CXX_SUPPORTS_SSE4_2
-- Performing Test CXX_SUPPORTS_SSE4_2 - Success
-- Performing Test CXX_SUPPORTS_AVX2
-- Performing Test CXX_SUPPORTS_AVX2 - Success
-- Performing Test CXX_SUPPORTS_AVX512
-- Performing Test CXX_SUPPORTS_AVX512 - Success
-- Arrow build warning level: PRODUCTION
CMake Error at cmake_modules/SetupCxxFlags.cmake:384 (message):
   Unknown compiler: IntelLLVM 2023.2.0
Call Stack (most recent call first):
   CMakeLists.txt:419 (include)

Component(s)

C++

@thisisnic thisisnic added the Priority: Blocker Marks a blocker for the release label Sep 15, 2023
@thisisnic
Copy link
Member Author

I think we just need to add it to the various bits where we check the value of CMAKE_CXX_COMPILER_ID in apache/arrow/cpp/cmake_modules/SetupCxxFlags.cmake, though I've no idea what flags would be appropriate to set,

@thisisnic
Copy link
Member Author

Or, as @jonkeane mentioned on Zulip, if this is something we don't want to or can't support, reintroduce the "arrow without arrow" approach that worked for solaris.

@thisisnic thisisnic changed the title [C++] Cmake error Unknown compiler: IntelLLVM 2023.2.0 [R][C++] Cmake error Unknown compiler: IntelLLVM 2023.2.0 Sep 15, 2023
@kou
Copy link
Member

kou commented Sep 16, 2023

Can we use the following (and similar changes)?

diff --git a/cpp/cmake_modules/SetupCxxFlags.cmake b/cpp/cmake_modules/SetupCxxFlags.cmake
index a5f565972..a65daf03f 100644
--- a/cpp/cmake_modules/SetupCxxFlags.cmake
+++ b/cpp/cmake_modules/SetupCxxFlags.cmake
@@ -329,7 +329,7 @@ if("${BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN")
     set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-sign-conversion")
     set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wunused-result")
     set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wdate-time")
-  elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Intel")
+  elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Intel" OR CMAKE_CXX_COMPILER_ID STREQUAL "IntelLLVM")
     if(WIN32)
       set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /Wall")
       set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /Wno-deprecated")

Or can we use the following (and similar changes)?

diff --git a/cpp/cmake_modules/SetupCxxFlags.cmake b/cpp/cmake_modules/SetupCxxFlags.cmake
index a5f565972..b595e6372 100644
--- a/cpp/cmake_modules/SetupCxxFlags.cmake
+++ b/cpp/cmake_modules/SetupCxxFlags.cmake
@@ -313,7 +313,7 @@ if("${BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN")
     set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /wd4267")
     set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /wd4838")
   elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_CXX_COMPILER_ID STREQUAL
-                                                        "Clang")
+                                                        "Clang" OR CMAKE_CXX_COMPILER_ID STREQUAL "IntelLLVM")
     set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wall")
     set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wextra")
     set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wdocumentation")

Or do we need one more new branch for the compiler?

@thisisnic
Copy link
Member Author

Thanks @kou!

I guess it comes down to how common this compiler is and if we want to support it given the effort it'd take to do so.

Unlike with the old Solaris build, where we used to build the R package without Arrow C++, this is a modern compiler, so it seems reasonable to try to support it.

Perhaps we start off by trying what @kou has suggested above, and seeing if that works, and if so, that's great, and if not then we re-evaluate?

@assignUser; given the extra output above, do you think we could replicate that environment and build failure now?

@assignUser
Copy link
Member

I suspected that this is not the default identification, when replicating it with one of the official oneAPI docker containers it reported as Intel not IntelLLVM even thought it should have been the new llvm backed compiler... but checking cmake docs https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_COMPILER_ID.html IntelLLVM is correct... maybe the cmake version in the container was to old and there is a fallback to 'Intel'? I will test this.

If 'supporting' the compiler is as simple as adding the additonal condition I am fine with it. I don't think anything more substantial isn't really warranted at this point (there has not been any user requests for it).

@thisisnic
Copy link
Member Author

I suspected that this is not the default identification, when replicating it with one of the official oneAPI docker containers it reported as Intel not IntelLLVM even thought it should have been the new llvm backed compiler... but checking cmake docs https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_COMPILER_ID.html IntelLLVM is correct... maybe the cmake version in the container was to old and there is a fallback to 'Intel'? I will test this.

If 'supporting' the compiler is as simple as adding the additonal condition I am fine with it. I don't think anything more substantial isn't really warranted at this point (there has not been any user requests for it).

Thanks @assignUser! If you're happy to take a look at this, then once you've got a PR up and tested I'll cherry-pick it into the CRAN release branch and try to submit it again.

@assignUser
Copy link
Member

Can confirm it worked because cmake 3.16 does not know about intelLLVM and falls back to intel. While it looks like we have to bump mimalloc version to make it IntelLLVM compatible... let's hope that's it.

thisisnic pushed a commit that referenced this issue Sep 20, 2023
### Rationale for this change

The IntelLLVM compiler has a different compiler ID that we currently can't handle and causes the build to fail. 

### What changes are included in this PR?

This adds the missing flags/IDs. Mimalloc in the current bundled version is not compatible with IntelLLVM this can be worked around by using a system version, a simple version bump is not sufficient and I don't think it's worth the effort to fix it properly for now.

For the R package build I have disabled mimalloc with IntelLLVM to avoid issues on CRAN.

* Closes: #37750

Authored-by: Jacob Wujciak-Jens <jacob@wujciak.de>
Signed-off-by: Nic Crane <thisisnic@gmail.com>
@thisisnic thisisnic added this to the 14.0.0 milestone Sep 20, 2023
thisisnic pushed a commit to thisisnic/arrow that referenced this issue Sep 20, 2023
### Rationale for this change

The IntelLLVM compiler has a different compiler ID that we currently can't handle and causes the build to fail. 

### What changes are included in this PR?

This adds the missing flags/IDs. Mimalloc in the current bundled version is not compatible with IntelLLVM this can be worked around by using a system version, a simple version bump is not sufficient and I don't think it's worth the effort to fix it properly for now.

For the R package build I have disabled mimalloc with IntelLLVM to avoid issues on CRAN.

* Closes: apache#37750

Authored-by: Jacob Wujciak-Jens <jacob@wujciak.de>
Signed-off-by: Nic Crane <thisisnic@gmail.com>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
### Rationale for this change

The IntelLLVM compiler has a different compiler ID that we currently can't handle and causes the build to fail. 

### What changes are included in this PR?

This adds the missing flags/IDs. Mimalloc in the current bundled version is not compatible with IntelLLVM this can be worked around by using a system version, a simple version bump is not sufficient and I don't think it's worth the effort to fix it properly for now.

For the R package build I have disabled mimalloc with IntelLLVM to avoid issues on CRAN.

* Closes: apache#37750

Authored-by: Jacob Wujciak-Jens <jacob@wujciak.de>
Signed-off-by: Nic Crane <thisisnic@gmail.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
### Rationale for this change

The IntelLLVM compiler has a different compiler ID that we currently can't handle and causes the build to fail. 

### What changes are included in this PR?

This adds the missing flags/IDs. Mimalloc in the current bundled version is not compatible with IntelLLVM this can be worked around by using a system version, a simple version bump is not sufficient and I don't think it's worth the effort to fix it properly for now.

For the R package build I have disabled mimalloc with IntelLLVM to avoid issues on CRAN.

* Closes: apache#37750

Authored-by: Jacob Wujciak-Jens <jacob@wujciak.de>
Signed-off-by: Nic Crane <thisisnic@gmail.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.

3 participants