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

ARROW-4873: [C++] Clarify documentation about how to use external ARROW_PACKAGE_PREFIX while also using CONDA dependency resolution #3901

Closed
wants to merge 3 commits into from

Conversation

wesm
Copy link
Member

@wesm wesm commented Mar 14, 2019

This clarifies the "hybrid" developer case where the library toolchain is maintained externally, but the Python bits are managed by an active conda environment.

@wesm
Copy link
Member Author

wesm commented Mar 14, 2019

Hm this seems to break the Windows builds. I'll take a closer look...

https://ci.appveyor.com/project/wesm/arrow/builds/23076536

@bkietz
Copy link
Member

bkietz commented Mar 14, 2019

@wesm I'm seeing a similar error to this, it seems that some of the conda dependencies don't have a Find<dep>.cmake in $CONDA_PREFIX/share/cmake-3.13/Modules, so setting <dep>_ROOT isn't getting picked up by anything. This was (hackily) fixed by passing -D<dep>_DIR=$CONDA_PREFIX/<path to directory containing depConfig.cmake>

@bkietz
Copy link
Member

bkietz commented Mar 14, 2019

In your case, it looks like FindDoubleConversion.cmake is looking for DoubleConversion_ROOT but ThirdPartyToolchain.cmake is setting double-conversion_ROOT

@bkietz
Copy link
Member

bkietz commented Mar 14, 2019

@xhochy am I reading this wrong?

@wesm
Copy link
Member Author

wesm commented Mar 14, 2019

@xhochy This removes the logic that overrides the AUTO default dependency source when $CONDA_PREFIX is set. I think that we should make -DARROW_DEPENDENCY_SOURCE=CONDA explicit

@wesm
Copy link
Member Author

wesm commented Mar 14, 2019

@bkietz yeah that is probably the issue. Is there a JIRA about the double-conversion thing?

@bkietz
Copy link
Member

bkietz commented Mar 14, 2019

@wesm no jira for double-conversion, I created 4879 for Flatbuffers (since that's what's happening to me)

@wesm
Copy link
Member Author

wesm commented Mar 14, 2019

The modules should be in $CONDA_PREFIX/lib/cmake or $CONDA_PREFIX/lib/pkgconfig not share/cmake-3.13. For me it seems to be building Flatbuffers from source. I'll comment in ARROW-4879

@wesm
Copy link
Member Author

wesm commented Mar 15, 2019

Hmm looks like the macOS build is finding the system boost =/

-- Boost version: 1.66.0
-- Found the following Boost libraries:
--   regex
--   system
--   filesystem
-- Boost include dir: /usr/local/include
-- Boost libraries: /usr/local/lib/libboost_regex-mt.dylib/usr/local/lib/libboost_system-mt.dylib/usr/local/lib/libboost_filesystem-mt.dylib
-- Added shared library dependency boost_system_shared: /usr/local/lib/libboost_system-mt.dylib
-- Added shared library dependency boost_filesystem_shared: /usr/local/lib/libboost_filesystem-mt.dylib
-- Added shared library dependency boost_regex_shared: /usr/local/lib/libboost_regex-mt.dylib

@wesm
Copy link
Member Author

wesm commented Mar 15, 2019

Here's an in-progress Appveyor build on my fork: https://ci.appveyor.com/project/wesm/arrow/builds/23089752

@wesm
Copy link
Member Author

wesm commented Mar 15, 2019

Needed to rebase to get the zstd fix. Will take this up in the morning

# * BREW: Use SYSTEM but search for select packages with brew.
if(NOT "$ENV{CONDA_PREFIX}" STREQUAL "")
Copy link
Member

Choose a reason for hiding this comment

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

I would actually like to keep this as the main strategy. Having conda packages and the build toolchain split is something that I would only advise to expert users since the compiler migration. With the new toolchain using system compilers, binutils, … is going to be a pain unless one is on a brand new system. I rather have CMake error out on missing conda packages than weird compilers as the binutils/compiler/ABI is not matching.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, I can write -DARROW_DEPENDENCY_SOURCE=SYSTEM -DARROW_PACKAGE_PREFIX=$TOOLCHAIN in the meantime

endif()

message(STATUS "Using ${ARROW_ACTUAL_DEPENDENCY_SOURCE} approach to find dependencies")
Copy link
Member

Choose a reason for hiding this comment

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

Moving this message here, we should also add a messge when the CONDA_PREFIX was applied.

@wesm wesm changed the title ARROW-4873: [C++] Do not override user-supplied ARROW_PACKAGE_PREFIX with ENV{CONDA_PREFIX} by default ARROW-4873: [C++] Clarify documentation about how to use external ARROW_PACKAGE_PREFIX while also using CONDA dependency resolution Mar 15, 2019
@wesm
Copy link
Member Author

wesm commented Mar 15, 2019

@xhochy done. I updated the PR title and JIRA to make this essentially a documentation issue

@wesm
Copy link
Member Author

wesm commented Mar 15, 2019

BTW I'm going to do a pass today on our developer docs and will incorporate this into them

# If your system packages are in general in a non-default location, you can
# set a general default for the *_ROOT variables using ARROW_PACKAGE_PREFIX.
# * CONDA: Same as system but set all *_ROOT variables to ENV{CONDA_PREFIX}.
# from the outside using the *_ROOT variables. If your system packages are
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
# from the outside using the *_ROOT variables. If your system packages are
# using the environment by setting *_ROOT variables. If your system packages are

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this right? What the documentation currently says is to do:

cmake .. -DGTest_ROOT=$GTEST_PATH

https://github.com/apache/arrow/blob/master/cpp/cmake_modules/ThirdpartyToolchain.cmake#L86

Copy link
Member

Choose a reason for hiding this comment

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

Actually both are working in CMake 3.11+, before that only -DGTest_ROOT=$GTEST_PATH works.

@wesm
Copy link
Member Author

wesm commented Mar 15, 2019

Looks like I have to rebase again. Is this OK to merge once the build runs?

Copy link
Member

@xhochy xhochy left a comment

Choose a reason for hiding this comment

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

+1, LGTM, merge on green

@wesm
Copy link
Member Author

wesm commented Mar 15, 2019

Appveyor build in my fork: https://ci.appveyor.com/project/wesm/arrow/builds/23104032. I'll merge once this makes it past the 3rd and 4th builds

@xhochy
Copy link
Member

xhochy commented Mar 15, 2019

@wesm This fails to find Boost in the toolchain build :(

@wesm
Copy link
Member Author

wesm commented Mar 15, 2019

Ugh, ok, having a look

@wesm
Copy link
Member Author

wesm commented Mar 15, 2019

Fixed, I had accidentally undone the \Library thing when I rebased

@wesm
Copy link
Member Author

wesm commented Mar 15, 2019

Appveyor build just started on my fork: https://ci.appveyor.com/project/wesm/arrow/builds/23109006

@wesm
Copy link
Member Author

wesm commented Mar 15, 2019

All systems go. Merging

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.

None yet

3 participants