Skip to content

Conversation

@zhjwpku
Copy link
Collaborator

@zhjwpku zhjwpku commented May 1, 2025

FetchContent_MakeAvailable generate a _SOURCE_DIR[1], for arrow it's arrow_SOURCE_DIR. arrow use a internal arrow_SOURCE_DIR to set its THIRDPARTY_DIR[2].

The generated arrow_SOURCE_DIR overwrite the arrow's interal arrow_SOURCE_DIR, causing the following build error:

CMake Error at build/_deps/arrow-src/cpp/cmake_modules/ThirdpartyToolchain.cmake:449 (file):
file STRINGS file
"/Users/zhjwpku/zhjwpku/iceberg-cpp/build/_deps/arrow-src/thirdparty/versions.txt"
cannot be read.

Fix this issue by renaming fetchcontent_declare name from Arrow to VendoredArrow.

[1] https://cmake.org/cmake/help/latest/module/FetchContent.html#command:fetchcontent_makeavailable
[2] https://github.com/apache/arrow/blob/main/cpp/cmake_modules/ThirdpartyToolchain.cmake#L368

FetchContent_MakeAvailable generate a <lowercaseName>_SOURCE_DIR[1],
for arrow it's arrow_SOURCE_DIR. arrow use a internal arrow_SOURCE_DIR
to set its THIRDPARTY_DIR[2].

The generated arrow_SOURCE_DIR overwrite the arrow's interal
arrow_SOURCE_DIR, causing the following build error:

CMake Error at build/_deps/arrow-src/cpp/cmake_modules/ThirdpartyToolchain.cmake:449 (file):
  file STRINGS file
  "/Users/zhjwpku/zhjwpku/iceberg-cpp/build/_deps/arrow-src/thirdparty/versions.txt"
  cannot be read.

Fix this issue by renaming fetchcontent_declare name from Arrow
to VendoredArrow.

[1] https://cmake.org/cmake/help/latest/module/FetchContent.html#command:fetchcontent_makeavailable
[2] https://github.com/apache/arrow/blob/main/cpp/cmake_modules/ThirdpartyToolchain.cmake#L368

Signed-off-by: Junwang Zhao <[email protected]>
Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

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

+1

Where did you hit this issue?

@zhjwpku
Copy link
Collaborator Author

zhjwpku commented May 4, 2025

+1

Where did you hit this issue?

I just rebased the main branch and removed the build dir then started a new config/build, using the following:

cmake -S . -B build -DCMAKE_INSTALL_PREFIX=/tmp/iceberg/install -DICEBERG_BUILD_STATIC=ON -DICEBERG_BUILD_SHARED=ON

I'm wondering why our CI doesn't hit this 😵‍💫

@Fokko Fokko merged commit b839b3b into apache:main May 4, 2025
5 checks passed
@Fokko
Copy link
Contributor

Fokko commented May 4, 2025

Thanks @zhjwpku for fixing this right away, and thanks @lidavidm and @wgtmac for the quick review

@zhjwpku zhjwpku deleted the vendor_arrow_avoid_variable_conflict branch May 4, 2025 08:29
@wgtmac
Copy link
Member

wgtmac commented May 6, 2025

I tried to reproduce it but unfortunately it built successfully.

@zhjwpku
Copy link
Collaborator Author

zhjwpku commented May 6, 2025

I tried to reproduce it but unfortunately it built successfully.

I think this issue may be caused by a bug in CMake, which appears to have been fixed in version 3.30. However, since we need to support CMake versions back to 3.25, this change seems reasonable.

I checked GH runner image[1], the CMake version is 3.31.6, which explains why our CI doesn't trigger the build error.

I bet your CMake version is greater than 3.30 ;) (not 100% sure since I'm not familiar their release process, they might have backport the fix to some earlier minor versions).

Merge: 6323fdc9cf 86ad7cc886
Author: Brad King <[email protected]>
Date:   Mon Sep 23 18:13:34 2024 +0000

    Merge topic 'project-vars' into release-3.30

    86ad7cc886 project: Only define non-cache vars if already defined
    4c152752da Help: State valid scopes for using proj_SOURCE_DIR and proj_BINARY_DIR

    Acked-by: Kitware Robot <[email protected]>
    Acked-by: buildbot <[email protected]>
    Merge-request: !9820

[1] https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md#tools

@wgtmac
Copy link
Member

wgtmac commented May 6, 2025

Ah, that makes sense. Perhaps we can add a comment to your fix so we can restore the Arrow project name once bumping the minimum CMake version?

@zhjwpku
Copy link
Collaborator Author

zhjwpku commented May 6, 2025

Ah, that makes sense. Perhaps we can add a comment to your fix so we can restore the Arrow project name once bumping the minimum CMake version?

Will add a TODO later, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants