-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat(build): Add support for CMake package #14738
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
Conversation
✅ Deploy Preview for meta-velox canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, thanks for getting a start on this. Makes sense to start with MINIMAL
first and iterate.
I have build this PR and I had to fix some minor issues to get it to install correctly but when trying to link against it the configure fails as the config links against targets that we don't discover before, when building MINIMAL
:
double-conversion::double-conversion
stemmer::stemmer
When building MINIMAL_WITH_DWIO
:
protobuf::libprotobuf
This was needed to avoid an issue with diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2a8e3e91b..4cea0c845 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -536,9 +536,9 @@ if(NOT TARGET gflags::gflags)
# target even when velox is built as a subproject which uses
# `find_package(gflags)` which does not create a globally imported target that
# we can ALIAS.
- add_library(gflags_gflags INTERFACE)
- target_link_libraries(gflags_gflags INTERFACE gflags)
- add_library(gflags::gflags ALIAS gflags_gflags)
+ # add_library(gflags_gflags INTERFACE)
+ # target_link_libraries(gflags_gflags INTERFACE gflags)
+ add_library(gflags::gflags ALIAS gflags)
endif()
if(${gflags_SOURCE} STREQUAL "BUNDLED") |
Oh, sorry. I'll add support for them.
Ah, sorry. I should have explained the case. We need to specify |
Let's work on GFlags as a separated task: #14760 |
I've added support for missing dependencies: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this fixed the issues I had, no Thrift::thrift
is missing though ^^
I also ran into issues with missing headers, this happens because there are a number of headers that are not part of any target, the diff below installs those headers.
I tried building an example but it looks like they all require exec or aggregates and are not compatible with minimal. :( When we have added more we should add CI to build examples against the installed Velox.
diff --git a/velox/CMakeLists.txt b/velox/CMakeLists.txt
index 1bf6976e8..ca66e3c82 100644
--- a/velox/CMakeLists.txt
+++ b/velox/CMakeLists.txt
@@ -26,6 +26,7 @@ add_subdirectory(external/date)
add_subdirectory(external/tzdb)
add_subdirectory(external/md5)
add_subdirectory(external/hdfs)
+add_subdirectory(external/utf8proc)
#
# examples depend on expression
diff --git a/velox/common/CMakeLists.txt b/velox/common/CMakeLists.txt
index 666142765..119cfc147 100644
--- a/velox/common/CMakeLists.txt
+++ b/velox/common/CMakeLists.txt
@@ -26,3 +26,6 @@ add_subdirectory(serialization)
add_subdirectory(time)
add_subdirectory(testutil)
add_subdirectory(fuzzer)
+add_subdirectory(future)
+
+velox_install_library_headers()
diff --git a/velox/common/future/CMakeLists.txt b/velox/common/future/CMakeLists.txt
new file mode 100644
index 000000000..a598690b3
--- /dev/null
+++ b/velox/common/future/CMakeLists.txt
@@ -0,0 +1,14 @@
+# Copyright (c) Facebook, Inc. and its affiliates.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+velox_install_library_headers()
diff --git a/velox/external/utf8proc/CMakeLists.txt b/velox/external/utf8proc/CMakeLists.txt
new file mode 100644
index 000000000..a598690b3
--- /dev/null
+++ b/velox/external/utf8proc/CMakeLists.txt
@@ -0,0 +1,14 @@
+# Copyright (c) Facebook, Inc. and its affiliates.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+velox_install_library_headers()
Oh, sorry. I forgot to install
Can we work on it as a separated task?
It makes sense. I'll do it in a future PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment, otherwise I think we can merge this and iterate the issues we found in follow up PRs!
Thanks again for starting this!
@@ -18,7 +18,10 @@ velox_add_library(velox_tpch_gen DBGenIterator.cpp TpchGen.cpp) | |||
|
|||
velox_include_directories(velox_tpch_gen PRIVATE dbgen/include) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this not need the same generator expression as velox_tpcds_gen
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need it here because this is a "PRIVATE" include directory. "PRIVATE" include directories aren't exported.
CMake/VeloxUtils.cmake
Outdated
install(TARGETS velox LIBRARY DESTINATION pyvelox COMPONENT pyvelox_libraries) | ||
if(VELOX_BUILD_PYTHON_PACKAGE) | ||
install(TARGETS velox LIBRARY DESTINATION pyvelox COMPONENT pyvelox_libraries) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry. This is not an intended change. I'll revert this.
We should work on this as #14756 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted.
@@ -18,7 +18,10 @@ velox_add_library(velox_tpch_gen DBGenIterator.cpp TpchGen.cpp) | |||
|
|||
velox_include_directories(velox_tpch_gen PRIVATE dbgen/include) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need it here because this is a "PRIVATE" include directory. "PRIVATE" include directories aren't exported.
Summary: Fixes facebookincubator#14275 Fixes facebookincubator#14756 This is not a complete CMake package support. This works only when: * `VELOX_BUILD_SHARED=ON` * `VELOX_BUILD_MINIMAL_WITH_DWIO=ON` * All dependencies are resolved from system (No bundled dependencies) FYI: I want to use this for Nimble: facebookincubator/nimble#215 Nimble uses `VELOX_BUILD_MINIMAL_WITH_DWIO=ON`. This is disabled by default. We can enabled this by specifying `VELOX_BUILD_CMAKE_PACKAGE=ON`. Users can find Velox by `find_package(Velox)`. We can expand supported cases step by step. How about this as the first step? Pull Request resolved: facebookincubator#14738 Reviewed By: mbasmanova Differential Revision: D81923615 Pulled By: Yuhta fbshipit-source-id: 8a7cb55c5e57c3b87b696fa7298cbd171e80d40d
Fixes #14275
Fixes #14756
This is not a complete CMake package support. This works only when:
VELOX_BUILD_SHARED=ON
VELOX_BUILD_MINIMAL_WITH_DWIO=ON
FYI: I want to use this for Nimble: facebookincubator/nimble#215
Nimble uses
VELOX_BUILD_MINIMAL_WITH_DWIO=ON
.This is disabled by default. We can enabled this by specifying
VELOX_BUILD_CMAKE_PACKAGE=ON
.Users can find Velox by
find_package(Velox)
.We can expand supported cases step by step. How about this as the first step?