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

[Release][C++] Release verification tasks fail with libxsimd-dev installed on ubuntu 22.04 #33786

Closed
raulcd opened this issue Jan 19, 2023 · 12 comments · Fixed by #33811
Closed

Comments

@raulcd
Copy link
Member

raulcd commented Jan 19, 2023

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

As pointed during the Release verification for 11.0.0 RC 0 the build failed on Ubuntu 22.04 with:

-- Building xsimd from source
CMake Error at cmake_modules/ThirdpartyToolchain.cmake:2295 (add_library):
  add_library cannot create imported target "xsimd" because another target
  with the same name already exists.
Call Stack (most recent call first):
  CMakeLists.txt:498 (include)

Full log shared by @pitrou here: https://gist.github.com/pitrou/3fdca2460fa71bba731b0706703b70b2

I have been able to reproduce when installing: $ sudo apt install libxsimd-dev on my Ubuntu 22.04.

Mail thread where the issue was raised: https://lists.apache.org/thread/bxkd8xb90pf83mp17xjv3gms46yzyz2q

Component(s)

C++, Release

@assignUser
Copy link
Member

resolve_dependency clearly does not correctly detect the system install (maybe discards it due to version?) and falls back on source build. So it is ossible that Findxsimd.cmkae was run, targets were created but resolve_dependecy discarded it causing the conflict?

I will investigate

@assignUser assignUser self-assigned this Jan 19, 2023
@assignUser
Copy link
Member

Ah yes this seems to be the case as 22.04 has xsimd 7.6.0 and we require 8.1.0:
Unpacking libxsimd-dev:amd64 (7.6.0-2)

@assignUser
Copy link
Member

Yes this is exactly what happens:

/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(2288):  resolve_dependency(xsimd REQUIRED_VERSION 8.1.0 FORCE_ANY_NEWER_VERSION TRUE )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(225):  set(options )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(226):  set(one_value_args FORCE_ANY_NEWER_VERSION HAVE_ALT IS_RUNTIME_DEPENDENCY REQUIRED_VERSION USE_CONFIG )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(232):  set(multi_value_args COMPONENTS PC_PACKAGE_NAMES )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(233):  cmake_parse_arguments(ARG ${options} ${one_value_args} ${multi_value_args} REQUIRED_VERSION;8.1.0;FORCE_ANY_NEWER_VERSION;TRUE )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(238):  if(ARG_UNPARSED_ARGUMENTS )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(241):  if(${ARG_IS_RUNTIME_DEPENDENCY} STREQUAL  )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(242):  set(ARG_IS_RUNTIME_DEPENDENCY TRUE )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(245):  if(ARG_HAVE_ALT )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(247):  else()
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(248):  set(PACKAGE_NAME xsimd )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(250):  set(FIND_PACKAGE_ARGUMENTS ${PACKAGE_NAME} )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(251):  if(ARG_REQUIRED_VERSION AND NOT ARG_FORCE_ANY_NEWER_VERSION )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(254):  if(ARG_USE_CONFIG )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(257):  if(ARG_COMPONENTS )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(260):  if(xsimd_SOURCE STREQUAL AUTO )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(261):  find_package(${FIND_PACKAGE_ARGUMENTS} )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(262):  set(COMPATIBLE ${${PACKAGE_NAME}_FOUND} )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(263):  if(COMPATIBLE AND ARG_FORCE_ANY_NEWER_VERSION AND ARG_REQUIRED_VERSION )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(266):  if(${${PACKAGE_NAME}_VERSION} VERSION_LESS ${ARG_REQUIRED_VERSION} )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(267):  message(DEBUG Couldn't find xsimd >= ${ARG_REQUIRED_VERSION} )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(268):  set(COMPATIBLE FALSE )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(271):  if(COMPATIBLE )
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(273):  else()
/arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake(274):  build_dependency(xsimd )

@assignUser
Copy link
Member

The problem is caused here:

if(ARG_REQUIRED_VERSION AND NOT ARG_FORCE_ANY_NEWER_VERSION)
list(APPEND FIND_PACKAGE_ARGUMENTS ${ARG_REQUIRED_VERSION})

Removing AND NOT ARG_FORCE_ANY_NEWER_VERSION fixes this case. Though I don't quite understand the purpose of the arg FORCE_ANY_NEWER_VERSION as in the lines above it prevents any version check from happening and a bit further down it is used to double check cmakes default behaviour for version checks (the given version is treated as a minimum version unless EXACT is added):

if(COMPATIBLE
AND ARG_FORCE_ANY_NEWER_VERSION
AND ARG_REQUIRED_VERSION)
if(${${PACKAGE_NAME}_VERSION} VERSION_LESS ${ARG_REQUIRED_VERSION})
message(DEBUG "Couldn't find ${DEPENDENCY_NAME} >= ${ARG_REQUIRED_VERSION}")

I think FORCE_ANY_NEWER_VERSION can just be removed completely? I will check when it was added.

@assignUser
Copy link
Member

assignUser commented Jan 19, 2023

It was introduced in #13958 to handle the SameMajor version compatibility required by xsimd but the xsimdConfigVersion.xmake provided even by xsimd 7.6.0 checks for this and will correctly abort find_package if the installed version is 9.0.0 and required is 8.1.0.

I have tested this by rewriting the ubuntu 22.04 xsimdConfigVersion.cmake to 9.0.0:

CMake Warning at cmake_modules/ThirdpartyToolchain.cmake:261 (find_package):
  Could not find a configuration file for package "xsimd" that is compatible
  with requested version "8.1.0".

  The following configuration files were considered but not accepted:

    /usr/lib/x86_64-linux-gnu/cmake/xsimd/xsimdConfig.cmake, version: 9.0.0
    /lib/x86_64-linux-gnu/cmake/xsimd/xsimdConfig.cmake, version: 9.0.0

Call Stack (most recent call first):
  cmake_modules/ThirdpartyToolchain.cmake:2288 (resolve_dependency)
  CMakeLists.txt:498 (include)

@kou I will open a PR soon.

@assignUser
Copy link
Member

assignUser commented Jan 19, 2023

to handle the SameMajor version compatibility required by xsimd

Oh I misunderstood the intention, FORCE_ANY_NEWER_VERSION is meant to circumvent the SameMajor checks that xsimd does to also make 9.0.0 work for us even if (as seen above) the default check makes it fail.

It might be possible to override the system provided xsimdConfigVersion.cmake by adding a custom one earlier on the search path. I will test.

Edit: No because the version is hardcoded into the config and we would need to know the version that is installed on the system.

@kou
Copy link
Member

kou commented Jan 20, 2023

We can just reuse the existing xsimd target:

diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 3eda538fb2..b03ebbbf11 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -2292,7 +2292,10 @@ if(ARROW_USE_XSIMD)
                      TRUE)
 
   if(xsimd_SOURCE STREQUAL "BUNDLED")
-    add_library(xsimd INTERFACE IMPORTED)
+    # Reuse xsimd target defined by find_package(xsimd)
+    if(NOT TARGET xsimd)
+      add_library(xsimd INTERFACE IMPORTED)
+    endif()
     if(CMAKE_VERSION VERSION_LESS 3.11)
       set_target_properties(xsimd PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
                                              "${XSIMD_INCLUDE_DIR}")

Or we can use other target name like we did for ProtoBuf:

diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt
index 8398a394e7..b193ea80d4 100644
--- a/cpp/CMakeLists.txt
+++ b/cpp/CMakeLists.txt
@@ -798,8 +798,8 @@ if(ARROW_WITH_RAPIDJSON)
 endif()
 
 if(ARROW_USE_XSIMD)
-  list(APPEND ARROW_SHARED_LINK_LIBS xsimd)
-  list(APPEND ARROW_STATIC_LINK_LIBS xsimd)
+  list(APPEND ARROW_SHARED_LINK_LIBS ${ARROW_XSIMD})
+  list(APPEND ARROW_STATIC_LINK_LIBS ${ARROW_XSIMD})
 endif()
 
 add_custom_target(arrow_dependencies)
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 3eda538fb2..3480baae2d 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -2292,15 +2292,17 @@ if(ARROW_USE_XSIMD)
                      TRUE)
 
   if(xsimd_SOURCE STREQUAL "BUNDLED")
-    add_library(xsimd INTERFACE IMPORTED)
+    add_library(arrow::xsimd INTERFACE IMPORTED)
     if(CMAKE_VERSION VERSION_LESS 3.11)
-      set_target_properties(xsimd PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
-                                             "${XSIMD_INCLUDE_DIR}")
+      set_target_properties(arrow::xsimd PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
+                                                    "${XSIMD_INCLUDE_DIR}")
     else()
-      target_include_directories(xsimd INTERFACE "${XSIMD_INCLUDE_DIR}")
+      target_include_directories(arrow::xsimd INTERFACE "${XSIMD_INCLUDE_DIR}")
     endif()
+    set(ARROW_XSIMD arrow::xsimd)
   else()
     message(STATUS "xsimd found. Headers: ${xsimd_INCLUDE_DIRS}")
+    set(ARROW_XSIMD xsimd)
   endif()
 endif()

@kou
Copy link
Member

kou commented Jan 20, 2023

BTW, I think that this is not a blocker because we have a -Dxsimd_SOURCE=BUNDLED workaround.

@kou
Copy link
Member

kou commented Jan 20, 2023

If we need 11.0.0 RC1, we can include a fix of this.

@assignUser
Copy link
Member

assignUser commented Jan 20, 2023

BTW, I think that this is not a blocker because we have a -Dxsimd_SOURCE=BUNDLED workaround.

I agree

@raulcd
Copy link
Member Author

raulcd commented Jan 20, 2023

For completeness I've run the verification scripts locally on Ubuntu 22.04 with libxsimd-dev installed and was able to build using the proposed workaround:

$ git diff
diff --git a/dev/release/verify-release-candidate.sh b/dev/release/verify-release-candidate.sh
index 9e044d2..a659d00 100755
--- a/dev/release/verify-release-candidate.sh
+++ b/dev/release/verify-release-candidate.sh
@@ -643,6 +643,7 @@ test_and_install_cpp() {
     -DPARQUET_BUILD_EXAMPLES=ON \
     -DPARQUET_BUILD_EXECUTABLES=ON \
     -DPARQUET_REQUIRE_ENCRYPTION=ON \
+    -Dxsimd_SOURCE=BUNDLED \
     ${ARROW_CMAKE_OPTIONS:-} \
     ${ARROW_SOURCE_DIR}/cpp
   export CMAKE_BUILD_PARALLEL_LEVEL=${CMAKE_BUILD_PARALLEL_LEVEL:-${NPROC}}

@kou
Copy link
Member

kou commented Jan 21, 2023

We can use ... ARROW_CMAKE_OPTIONS="-Dxsimd_SOURCE=BUNDLED" dev/release/verify-release-candidate.sh ... instead of changing verify-release-candidate.sh.

kou added a commit to kou/arrow that referenced this issue Jan 21, 2023
kou added a commit that referenced this issue Jan 21, 2023
### Rationale for this change

If old xsimd is installed, CMake target for bundled xsimd is conflicted.

### What changes are included in this PR?

Use `arrow::xsimd` for bundled xsimd's target name to avoid conflict.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* Closes: #33786

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@kou kou added this to the 12.0.0 milestone Jan 21, 2023
sjperkins pushed a commit to sjperkins/arrow that referenced this issue Feb 10, 2023
### Rationale for this change

If old xsimd is installed, CMake target for bundled xsimd is conflicted.

### What changes are included in this PR?

Use `arrow::xsimd` for bundled xsimd's target name to avoid conflict.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* Closes: apache#33786

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.

3 participants