ARROW-4611: [C++] Rework CMake logic #3688

xhochy · 2019-02-18T18:04:17Z

This changes refactors much of our CMake logic to make use of built-in CMake paths and remove custom logic. It also switches to the use of more modern dependency management via CMake targets instead of plain text variables.

This includes the following fixes:

Use CMake's standard find features, e.g. respecting the *_ROOT variables: https://issues.apache.org/jira/browse/ARROW-4383
Add a Dockerfile for Fedora: https://issues.apache.org/jira/browse/ARROW-4730
Add a Dockerfile for Ubuntu Xenial: https://issues.apache.org/jira/browse/ARROW-4731
Add a Dockerfile for Ubuntu Bionic: https://issues.apache.org/jira/browse/ARROW-4849
Add a Dockerfile for Debian Testing: https://issues.apache.org/jira/browse/ARROW-4732
Change the clang-7 entry to use system packages without any dependency on conda(-forge): https://issues.apache.org/jira/browse/ARROW-4733
Support double-conversion<3.1: https://issues.apache.org/jira/browse/ARROW-4617
Use google benchmark from toolchain: https://issues.apache.org/jira/browse/ARROW-4609
Use the compilers metapackage to install the correct binutils when using conda, otherwise system binutils to fix https://issues.apache.org/jira/browse/ARROW-4485
RapidJSON throws compiler errors with GCC 8+ https://issues.apache.org/jira/browse/ARROW-4750
Handle EXPECT_OK collision: https://issues.apache.org/jira/browse/ARROW-4760
Activate flight build in ci/docker_build_cpp.sh: https://issues.apache.org/jira/browse/ARROW-4614
Build Gandiva in the docker containers: https://issues.apache.org/jira/browse/ARROW-4644

xhochy · 2019-02-18T18:05:48Z

This can be tested using docker-compose build cpp-fedora && docker-compose run cpp-fedora. The build passes beyond the problems reported at https://issues.apache.org/jira/browse/ARROW-4242 but arrow::int32() sadly returns a null pointer and I guess that this is still a compiler bug.

fsaintjacques · 2019-02-18T18:09:16Z

cpp/cmake_modules/SetupCxxFlags.cmake

OR (OMPILER

I think you need to references variables with $ in here, not sure why cmake isn't complaining.

No need for $ for the variables here. VERSION_LESS works with variable names. Although I hoped that it would be complaining about OMPILER_VERSION.

kszucs · 2019-02-18T19:25:46Z

cpp/Dockerfile.fedora

nit: run these in a single layer

cpp/Dockerfile.fedora

kou

I leave some comments but I like this approach.

kou · 2019-02-19T09:26:09Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

DOUBLE_CONVERSION_LIBRARIES will be better for consistency.

I used double-conversion_LIBRARIES as this is what double-conversion's CMake targets provides.

(Ah, *_LIBRARY is better than*_LIBRARIES.)
*_LIBRARY is our rule for specifying the correct target. So I think that we can use our naming rule (XXX_LIBRARY) instead of double-conversion's naming rule here.
But this is a trivial. I don't care with double-conversion_LIBRARY.

kou · 2019-02-19T09:28:45Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

set_target_property(double-conversion::double-conversion INTERFACE_INCLUDE_DIRECTORIES "${DOUBLE_CONVERSION_PREFIX}/include") will be better than this and `include_directories(${double_conversion_INCLUDE_DIRS})".

+1 (if possible)

kou · 2019-02-19T09:30:11Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

Do we set(double-convertion_ROOT $ENV{CONDA_PREFIX}) here?

kou · 2019-02-19T09:41:52Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

Suggested change

if (NOT ${double-conversion_FOUND})

if(${double-conversion_FOUND})

double_conversion_config()

else()

build_double_conversion()

endif()

will be more readable.

kszucs · 2019-02-21T22:53:38Z

Please add an entry to the nightly tests too https://github.com/apache/arrow/blob/master/dev/tasks/tests.yml#L21

xhochy · 2019-02-24T23:33:17Z

@wesm This is currently badly red for me in Travis due to the simple fact that we install all our dependencies for GCC 5.4 into a conda environment but don't activate this conda environment then. I don't think this is the way we should use the packages. I'll have a look tomorrow and will probably add a conda activate to the CI job.

pitrou · 2019-02-28T13:25:44Z

Since we are talking about redoing all the CMake dependency logic, here is a wild idea: one PITA with cmake is that find_package can't be used on a project built with ExternalProject, so you have to use a different mechanism, hardcode library paths, etc. So how about locating or building dependencies in a separate CMake project? Then the main CMake project can always use find_package, which would simplify the logic a lot.

xhochy · 2019-02-28T14:07:09Z

Since we are talking about redoing all the CMake dependency logic, here is a wild idea: one PITA with cmake is that find_package can't be used on a project built with ExternalProject, so you have to use a different mechanism, hardcode library paths, etc. So how about locating or building dependencies in a separate CMake project? Then the main CMake project can always use find_package, which would simplify the logic a lot.

This would definitely simplify the logic a bit more but would also introduce some inconvenience to the user again to get a first running Arrow setup. After juggling around so much with the CMake code, I'd rather keep the logic with the external projects. If we were to separate this out into another project, we would also need to share logic what really needs to be built.

wesm · 2019-03-06T00:33:10Z

On Windows, it looks like the wrong libraries are being linked in the toolchain build:

-- Found GTest: C:/Miniconda36-x64/envs/arrow/Library/lib/gtest-md.lib  
-- Performing Test GTEST_COMPILES_WITHOUT_MACRO
-- Performing Test GTEST_COMPILES_WITHOUT_MACRO - Failed
-- Setting GTEST_LINKED_AS_SHARED_LIBRARY=1 on GTest::GTest
-- RapidJSON found. Headers: C:/Miniconda36-x64/envs/arrow/Library/include
-- Found ZLIB: C:/Miniconda36-x64/envs/arrow/Library/lib/z.lib (found version "1.2.11")

According to https://github.com/conda-forge/gtest-feedstock/blob/master/recipe/bld.bat#L88 it should be using gtest_dll-md.lib

xhochy · 2019-03-06T10:27:45Z

@fsaintjacques Can you post the output of CMake? Please ensure that you have cleaned your build directory first.

@wesm Yes, the Windows issue would need some help. I'm still struggling with getting a basic Arrow build working on Windows. We also need to decide on what to do with the Valgrind errors. They look quite confusing to me and I suspect a bit that this due to clang-7 and not our code. My preference at the moment would be to move the Valgrind runs into the conda-forge toolchain job.,

fsaintjacques · 2019-03-06T15:04:20Z

Attached cmake.log

fsaintjacques · 2019-03-06T15:29:51Z

@xhochy the second valgrind failure is due unrecognized AVX instruction: https://travis-ci.org/apache/arrow/jobs/502177486#L5353-L5356

This is a valgrind problem, not always reproducible because libc (and I suspect libstc++) will dispatch function implementation at runtime depending on the supported cpu features.

The first failure is a failed assertion. Either we upgrade valgrind (ugh), or ignore the 2 tests.

fsaintjacques · 2019-03-06T16:01:19Z

appveyor.yml

Yes, will remove before merge.

ci/appveyor-cpp-build.bat

fsaintjacques · 2019-03-06T16:48:55Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

Should we just vendor it then, I don't think we expose any symbols?

fsaintjacques · 2019-03-06T16:51:03Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

This does not work locally when using dependency AUTO without conda. It seems that find_package(benchmark) "finds" it but

benchmark_FOUND=1

benchmark_INCLUDE_DIR=

benchmark_LIBRARY=

I'm not sure if this is an issue with libbenchmark-dev cmake's config definition.

On which system did you have this combination?

Ubuntu 18.04, with libbenchmark-dev installed.

Will add a Dockerfile for this then.

Found the problem. Empty benchmark_INCLUDE_DIR and benchmark_LIBRARY are expected, we only need the targets benchmark::benchmark and benchmark::benchmark_main. The latter is just not yet available in the version of benchmark. So to support benchmarks on Ubuntu 18.04, we need to compile our own benchmark main.

fsaintjacques · 2019-03-06T17:03:46Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

Something odd is happening, it uses another build directory downloaded rapidjson:

-- RapidJSON found. Headers: /home/fsaintjacques/src/db/arrow/cpp/flight-build/rapidjson_ep/src/rapidjson_ep-install/include

wesm · 2019-03-14T02:04:25Z

Looking at this. Going to see if I can resolve the Windows issue

wesm · 2019-03-14T03:41:56Z

I don't know what's wrong with the conda-forge *-md.lib libraries. Removing the incorrect GTEST_BUILD_AS_SHARED_LIBRARY flag I get the following linking error

Link:
  C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\x86_amd64\link.exe /ERRORREPORT:QUEUE /OUT:"C:\Users\wesmc\code\arrow\cpp\build\relea
  se\Release\arrow-compute-util-internal-test.exe" /INCREMENTAL:NO /NOLOGO ..\..\..\..\release\Release\arrow_testing.lib ..\..\..\..\release\Relea
  se\arrow.lib "C:\Miniconda\envs\arrow-dev\Library\lib\double-conversion.lib" "C:\Miniconda\envs\arrow-dev\Library\lib\brotlienc.lib" "C:\Minicon
  da\envs\arrow-dev\Library\lib\brotlidec.lib" "C:\Miniconda\envs\arrow-dev\Library\lib\brotlicommon.lib" "C:\Miniconda\envs\arrow-dev\Library\lib
  \double-conversion.lib" "C:\Miniconda\envs\arrow-dev\Library\lib\boost_system.lib" "C:\Miniconda\envs\arrow-dev\Library\lib\boost_filesystem.lib
  " "C:\Miniconda\envs\arrow-dev\Library\lib\boost_regex.lib" "C:\Miniconda\envs\arrow-dev\Library\lib\gtest_main-md.lib" "C:\Miniconda\envs\arrow
  -dev\Library\lib\gtest-md.lib" "C:\Miniconda\envs\arrow-dev\Library\lib\gmock-md.lib" kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib
   ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /NODEFAULTLIB:LIBCMT /MANIFEST /MANIFESTUAC:"level='asInvoker' uiAccess='false'" /man
  ifest:embed /PDB:"C:/Users/wesmc/code/arrow/cpp/build/release/Release/arrow-compute-util-internal-test.pdb" /SUBSYSTEM:CONSOLE /TLBID:1 /DYNAMIC
  BASE /NXCOMPAT /IMPLIB:"C:/Users/wesmc/code/arrow/cpp/build/release/Release/arrow-compute-util-internal-test.lib" /MACHINE:X64  /machine:x64 "ar
  row-compute-util-internal-test.dir\Release\util-internal-test.obj"
  gtest-md.lib(gtest-all.obj) : MSIL .netmodule or module compiled with /GL found; restarting link with /LTCG; add /LTCG to the link command line
  to improve linker performance
util-internal-test.obj : error LNK2001: unresolved external symbol "class testing::internal::Mutex testing::internal::g_gmock_mutex" (?g_gmock_mut ex@internal@testing@@3VMutex@12@A) [C:\Users\wesmc\code\arrow\cpp\build\src\arrow\compute\kernels\arrow-compute-util-internal-test.vcxproj]
util-internal-test.obj : error LNK2001: unresolved external symbol "class testing::internal::ThreadLocal<class testing::Sequence *> testing::inter nal::g_gmock_implicit_sequence" (?g_gmock_implicit_sequence@internal@testing@@3V?$ThreadLocal@PEAVSequence@testing@@@12@A) [C:\Users\wesmc\code\ar row\cpp\build\src\arrow\compute\kernels\arrow-compute-util-internal-test.vcxproj]
C:\Users\wesmc\code\arrow\cpp\build\release\Release\arrow-compute-util-internal-test.exe : fatal error LNK1120: 2 unresolved externals [C:\Users\w esmc\code\arrow\cpp\build\src\arrow\compute\kernels\arrow-compute-util-internal-test.vcxproj]
Done Building Project "C:\Users\wesmc\code\arrow\cpp\build\src\arrow\compute\kernels\arrow-compute-util-internal-test.vcxproj" (default targets) - - FAILED.


Build FAILED.

"C:\Users\wesmc\code\arrow\cpp\build\src\arrow\compute\kernels\arrow-compute-util-internal-test.vcxproj" (default target) (1) ->
(Link target) ->
  util-internal-test.obj : error LNK2001: unresolved external symbol "class testing::internal::Mutex testing::internal::g_gmock_mutex" (?g_gmock_m utex@internal@testing@@3VMutex@12@A) [C:\Users\wesmc\code\arrow\cpp\build\src\arrow\compute\kernels\arrow-compute-util-internal-test.vcxproj]
  util-internal-test.obj : error LNK2001: unresolved external symbol "class testing::internal::ThreadLocal<class testing::Sequence *> testing::int ernal::g_gmock_implicit_sequence" (?g_gmock_implicit_sequence@internal@testing@@3V?$ThreadLocal@PEAVSequence@testing@@@12@A) [C:\Users\wesmc\code\ arrow\cpp\build\src\arrow\compute\kernels\arrow-compute-util-internal-test.vcxproj]
  C:\Users\wesmc\code\arrow\cpp\build\release\Release\arrow-compute-util-internal-test.exe : fatal error LNK1120: 2 unresolved externals [C:\Users \wesmc\code\arrow\cpp\build\src\arrow\compute\kernels\arrow-compute-util-internal-test.vcxproj]

    0 Warning(s)
    3 Error(s)

Time Elapsed 00:00:01.46

It isn't reasonable to block the patch on this issue. Using -DGTest_SOURCE=BUNDLED seems to make the build run for me (I'm doing an ALL_BUILD to make sure). I'm going to push this change. Can we merge as soon as we get a green build?

wesm · 2019-03-14T03:54:02Z

I just pushed my changes so you can look at the diff, I think you'll want to squash before merging to make sure you get "credit" (the merge script tie breaks based on the most recent commit for some reason)

xhochy · 2019-03-14T10:27:11Z

@kou Can you have a look at this?

xhochy · 2019-03-14T10:32:53Z

Windows failures are due to #3885, I'm taking care of Plasma on OSX now.

wesm · 2019-03-14T15:09:02Z

+1, bombs away. I'll look at the ZSTD issue shortly

kou · 2019-03-14T20:48:42Z

@xhochy Sorry for not having a look at this! I'll create a pull request when I find something as usual. Thanks for doing this!

fsaintjacques reviewed Feb 18, 2019

View reviewed changes

kszucs reviewed Feb 18, 2019

View reviewed changes

cpp/Dockerfile.fedora Outdated

Copy link

Member

kszucs Feb 18, 2019

Choose a reason for hiding this comment

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

nit: run these in a single layer

kszucs reviewed Feb 18, 2019

View reviewed changes

cpp/Dockerfile.fedora Outdated Show resolved Hide resolved

wesm force-pushed the master branch from 3088183 to 0c6b2d2 Compare February 18, 2019 19:34

kou reviewed Feb 19, 2019

View reviewed changes

xhochy force-pushed the build-on-fedora branch from a1cd7f8 to 5073288 Compare February 21, 2019 18:27

xhochy changed the title ~~[WIP] Add Dockerfile for fedora:29~~ ARROW-4611: [C++] Rework CMake logic Feb 24, 2019

xhochy force-pushed the build-on-fedora branch 2 times, most recently from 151a8d0 to 186ff04 Compare February 25, 2019 16:04

xhochy mentioned this pull request Feb 26, 2019

ARROW-4609: [C++] Use google benchmark from toolchain #3681

Closed

xhochy force-pushed the build-on-fedora branch 12 times, most recently from e29d996 to e2ce187 Compare February 28, 2019 12:38

xhochy force-pushed the build-on-fedora branch from e2ce187 to c8e9f42 Compare February 28, 2019 13:49

xhochy force-pushed the build-on-fedora branch 2 times, most recently from ad5c1ba to 21e88e6 Compare March 1, 2019 17:46

fsaintjacques reviewed Mar 6, 2019

View reviewed changes

robertnishihara mentioned this pull request Mar 11, 2019

ARROW-4418 [Plasma] replace event loop with boost::asio for plasma store #3704

Closed

xhochy force-pushed the build-on-fedora branch 4 times, most recently from ac5057f to a73735c Compare March 12, 2019 16:43

xhochy mentioned this pull request Mar 12, 2019

ARROW-4439: [C++] Improve FindBrotli.cmake #3535

Closed

xhochy force-pushed the build-on-fedora branch 6 times, most recently from bedf17b to dcb1ce2 Compare March 13, 2019 15:35

wesm force-pushed the build-on-fedora branch from dcb1ce2 to 367ab24 Compare March 14, 2019 03:53

ARROW-4611: [C++] Rework CMake logic

88e11fc

xhochy force-pushed the build-on-fedora branch from ed82cb9 to 88e11fc Compare March 14, 2019 12:38

wesm closed this in 5114be6 Mar 14, 2019

wesm deleted the build-on-fedora branch March 14, 2019 15:11

asfimport mentioned this pull request Mar 14, 2019

[C++] Rework CMake third-party logic #21150

Closed

-  if (NOT ${double-conversion_FOUND})
+  if(${double-conversion_FOUND})
+    double_conversion_config()
+  else()
+    build_double_conversion()
+  endif()

ARROW-4611: [C++] Rework CMake logic #3688

ARROW-4611: [C++] Rework CMake logic #3688

Uh oh!

Conversation

xhochy commented Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xhochy commented Feb 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kszucs commented Feb 21, 2019

Uh oh!

xhochy commented Feb 24, 2019

Uh oh!

pitrou commented Feb 28, 2019

Uh oh!

xhochy commented Feb 28, 2019

Uh oh!

wesm commented Mar 6, 2019

Uh oh!

xhochy commented Mar 6, 2019

Uh oh!

fsaintjacques commented Mar 6, 2019

Uh oh!

fsaintjacques commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Mar 14, 2019

Uh oh!

wesm commented Mar 14, 2019

Uh oh!

wesm commented Mar 14, 2019

Uh oh!

xhochy commented Mar 14, 2019

Uh oh!

xhochy commented Mar 14, 2019

Uh oh!

wesm commented Mar 14, 2019

xhochy commented Feb 18, 2019 •

edited

Loading

fsaintjacques commented Mar 6, 2019 •

edited

Loading