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++] CMAKE_AR is not passed to bzip2 thirdparty dependency #29793

Closed
asfimport opened this issue Oct 3, 2021 · 7 comments
Closed

[C++] CMAKE_AR is not passed to bzip2 thirdparty dependency #29793

asfimport opened this issue Oct 3, 2021 · 7 comments

Comments

@asfimport
Copy link

It seems like the AR or CMAKE_AR variables aren't getting passed for the bzip2 build, which causes if to fail if we're doing a BUNDLED build and ar isn't available in the $PATH (e.g. in a conda environment).

To replicate:

  1. Download Arrow and start an interactive shell in a container
    (docker should be fine if you prefer it to podman)
git clone --depth 1 git@github.com:apache/arrow.git
podman run -it --rm -v ./arrow:/arrow:Z docker://ursalab/amd64-ubuntu-18.04-conda-python-3.6:worker bash
  1. Build Arrow by running this in in the container:
export ARROW_BUILD_TOOLCHAIN=$CONDA_PREFIX
export ARROW_HOME=$CONDA_PREFIX
export PARQUET_HOME=$CONDA_PREFIX
export ARROW_BUILD_TYPE=DEBUG

cd /arrow
mkdir -p cpp/build
pushd cpp/build

cmake \
      -DCMAKE_BUILD_TYPE=$ARROW_BUILD_TYPE \
      -DCMAKE_INSTALL_PREFIX=$ARROW_HOME \
      -DCMAKE_AR=${AR} \
      -DCMAKE_RANLIB=${RANLIB} \
      -DARROW_WITH_BZ2=ON \
      -DARROW_VERBOSE_THIRDPARTY_BUILD=ON \
      -DARROW_JEMALLOC=OFF \
      -DARROW_SIMD_LEVEL=NONE -DARROW_RUNTIME_SIMD_LEVEL=NONE \
      -DARROW_DEPENDENCY_SOURCE=BUNDLED \
      ..
make
# make[3]: ar: No such file or directory
# make[3]: *** [Makefile:48: libbz2.a] Error 127
# make[2]: *** [CMakeFiles/bzip2_ep.dir/build.make:135: bzip2_ep-prefix/src/bzip2_ep-stamp/bzip2_ep-build] Error 2
# make[1]: *** [CMakeFiles/Makefile2:726: CMakeFiles/bzip2_ep.dir/all] Error 2

In the cmake call above, ARROW_JEMALLOC and the SIMD flags are just to skip compiling irrelevant things.

I think this line in ThirdpartyToolchain.cmake needs to be changed to pass CMAKE_AR.

set(BZIP2_EXTRA_ARGS "CC=${CMAKE_C_COMPILER}" "CFLAGS=${EP_C_FLAGS}")

Other related issues have also needed to pass CMAKE_RANLIB, in addition to CMAKE_AR. I'm not sure if that applies here.

 
Related: ARROW-4471, ARROW-4831

Edit: added ARROW_BUILD_TYPE above.

Reporter: Karl Dunkle Werner / @karldw
Assignee: Karl Dunkle Werner / @karldw

PRs and other links:

Note: This issue was originally created as ARROW-14210. Please see the migration documentation for further details.

@asfimport
Copy link
Author

Neal Richardson / @nealrichardson:
Thanks @karldw! Would you like to make a PR with this fix? cc @kou

@asfimport
Copy link
Author

Karl Dunkle Werner / @karldw:
I'm happy to. Two questions:

  1. Do we need CMAKE_RANLIB here, or is that irrelevant?
  2. Do you want a test for this? If so, could you point me to the file where that test should be added?

@asfimport
Copy link
Author

Neal Richardson / @nealrichardson:

  1. Do we need CMAKE_RANLIB here, or is that irrelevant?

Not sure, I guess you'll see :)

  1. Do you want a test for this? If so, could you point me to the file where that test should be added?

I'm not sure about this either, we have lots of CI already, what's different about this setup exactly? Maybe there's an existing CI job we can modify rather than adding a new one.

@asfimport
Copy link
Author

Karl Dunkle Werner / @karldw:

what's different about this setup exactly?
The combination of:

@asfimport
Copy link
Author

Kouhei Sutou / @kou:
Could you do it for CMAKE_RANLIB too like

if(CMAKE_AR)
set(EP_COMMON_TOOLCHAIN ${EP_COMMON_TOOLCHAIN} -DCMAKE_AR=${CMAKE_AR})
endif()
if(CMAKE_RANLIB)
set(EP_COMMON_TOOLCHAIN ${EP_COMMON_TOOLCHAIN} -DCMAKE_RANLIB=${CMAKE_RANLIB})
endif()
?

We don't need a test for it for now. We will not change bzip2's build code too much. If we make a regression around this, we'll add a test for this case.

@asfimport
Copy link
Author

Kouhei Sutou / @kou:
Note that we can use list(APPEND BZIP2_EXTRA_ARGS ...) for this.

@asfimport
Copy link
Author

Kouhei Sutou / @kou:
Issue resolved by pull request 11334
#11334

@asfimport asfimport added this to the 6.0.0 milestone Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant