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.

Sign up for GitHub

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

Jump to bottom

Add boost to resolve_dependency #3488

Closed
wants to merge 23 commits into from
Closed

Add boost to resolve_dependency #3488

wants to merge 23 commits into from

Conversation

assignUser
Copy link
Collaborator

@assignUser assignUser commented Dec 12, 2022 •
edited by kgpai
Loading

We need to host a pruned version of the boost source as cloning from git with all submodules takes a long time (xx mins).
Or we could use GIT_SUBMODULES <module>... to list the submodules explicitly (which would still take longer than downloading a already pruned archive)

@netlify
Copy link

netlify bot commented Dec 12, 2022 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 02b2071
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/63a4a64e24fde40008ac4b1e

@assignUser assignUser changed the title [WI{] Add boost to resolve_dependency [WIP] Add boost to resolve_dependency Dec 12, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 12, 2022
@assignUser assignUser force-pushed the add-boost-cmake branch 4 times, most recently from e30dae8 to b10f2e5 Compare December 12, 2022 13:02
@assignUser
Copy link
Collaborator Author

assignUser commented Dec 12, 2022 •
edited
Loading

On circleci cloning everything only takes 2 minutes ... so that can probably wait for a follow-up:shrug:

@assignUser assignUser mentioned this pull request Dec 12, 2022
Closed
@assignUser
Copy link
Collaborator Author

I have created #3489 to add icu to the images.

@assignUser
Copy link
Collaborator Author

assignUser commented Dec 12, 2022 •
edited
Loading

If we use system boost the Boost_Libraries does not get populated correctly, probably need to modify resolve_dependency to handle COMPONENTS args

assignUser
assignUser commented Dec 20, 2022
View reviewed changes
CMakeLists.txt
@@ -306,41 +320,33 @@ find_library(EVENT event)

find_library(DOUBLE_CONVERSION double-conversion)

find_package(ZLIB)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

required for boost

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Add this as a comment..

@assignUser assignUser changed the title [WIP] Add boost to resolve_dependency Add boost to resolve_dependency Dec 20, 2022
@assignUser assignUser marked this pull request as ready for review December 22, 2022 14:44
@assignUser
Copy link
Collaborator Author

This is now ready :)

@assignUser assignUser requested review from raulcd and kgpai December 22, 2022 14:45
kgpai
kgpai reviewed Dec 22, 2022
View reviewed changes
Copy link
Contributor

@kgpai kgpai left a comment

Choose a reason for hiding this comment

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

Looks good, some minor nits.

.circleci/config.yml Outdated
@@ -236,6 +236,7 @@ jobs:
- run:
name: "Build"
command: |
dnf install -y libicu-devel
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably move it to the setup-* scripts and have it baked in the docker images- So that its easy to repro problems rather than trying to wonder why build isnt working.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes agreed, this is from before the ICU PR :D

CMake/boost/FindBoost.cmake
@@ -0,0 +1 @@
set(Boost_FOUND TRUE)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Why are these CMake//Find files required ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To override the modules that come with cmake. As they look for already built libs at configure time (aka when we haven't built the dependencies yet) they won't find them.

Cmake >= 3.24 comes with a built in way to do this (OVERRIDE_FIND_PACKAGE) which I tested and works great but that is out of reach for now :D

CMakeLists.txt
@@ -306,41 +320,33 @@ find_library(EVENT event)

find_library(DOUBLE_CONVERSION double-conversion)

find_package(ZLIB)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Add this as a comment..

ThirdpartyToolchain.cmake Outdated
if(DEFINED ENV{VELOX_BOOST_URL})
set(BOOST_SOURCE_URL "$ENV{VELOX_BOOST_URL}")
else()
# We need to sue boost > 1.70 to build it with CMake
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/sue/use

Copy link
Contributor

Choose a reason for hiding this comment

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

also maybe change comment to say 1.80

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The boost repo contains cmake files starting in 1.70 so bit is correct though I have not tested it with a lower version and there were likely changes since then...

ThirdpartyToolchain.cmake Outdated
list(TRANSFORM numeric_subdirs APPEND /include)
include_directories(${boost_INCLUDE_DIRS} ${numeric_subdirs})

# this prevents system boost from leaking in
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/this/This

@facebook-github-bot
Copy link
Contributor

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot pushed a commit that referenced this pull request Dec 22, 2022
@assignUser @facebook-github-bot
Add icu to setup scripts (#3489)
b6cc877 
Summary:
When building boost from source we need icu available. See #3366 and #3488.

Pull Request resolved: #3489

Reviewed By: Yuhta

Differential Revision: D42212599

Pulled By: kgpai

fbshipit-source-id: cbdc6905ed5af0d1aac61337e13cc636b650f612
@facebook-github-bot
Copy link
Contributor

@kgpai merged this pull request in b4b9868.

@assignUser assignUser deleted the add-boost-cmake branch February 2, 2023 12:20
marin-ma pushed a commit to marin-ma/velox-oap that referenced this pull request Dec 15, 2023
@gaoyangxiaozhu
[VL] Disable columnar table cache by default (facebookincubator#3488)
1788834
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kgpai kgpai kgpai approved these changes

@raulcd raulcd Awaiting requested review from raulcd

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@assignUser @facebook-github-bot @kgpai