HBASE-24400: Fixup cmake infrastructure to allow dependencies to be built locally #2
Conversation
| @@ -0,0 +1,15 @@ | |||
| Apache HBase | |||
| Copyright 2019 The Apache Software Foundation | |||
| @@ -0,0 +1,19 @@ | |||
| # Distributed under the OSI-approved MIT License. See accompanying | |||
| # file LICENSE or https://github.com/Crascit/DownloadProject for details. | |||
There was a problem hiding this comment.
Weird that Crascit doesn't have the normal MIT license header. I guess we leave it rather than put the real MIT license text.
| @@ -34,7 +34,7 @@ JNIEnv *MiniCluster::CreateVM(JavaVM **jvm) { | |||
| char *classpath = getenv("CLASSPATH"); | |||
| std::string clspath; | |||
| if (classpath == NULL || strstr(classpath, "-tests.jar") == NULL) { | |||
| std::string clsPathFilePath("../../../hbase-build-configuration/target/cached_classpath.txt"); | |||
| std::string clsPathFilePath("../target/cached_classpath.txt"); | |||
There was a problem hiding this comment.
I reckon this doesn't work after removing the native client out of the "main" HBase java repository.
|
@joshelser the patches are to ensure we have builds on the current compiler. Previously these succeeded because the standard accepted these as warnings. I moved the standard up a bit which caused those warnings to become errors. They are rectified in newer versions of zookeeper but I do not intend to update to 3.5 ( and likely can't? ) |
| @@ -3,6 +3,9 @@ | |||
| *.lo | |||
| *.o | |||
|
|
|||
| # Proto files | |||
| src/hbase/if/*.proto | |||
There was a problem hiding this comment.
copying of protos can be rolled into the cmake script instead of manually invoking copy-protos script?
| ExternalProject_Add( | ||
| facebook-fizz-proj | ||
| GIT_REPOSITORY "https://github.com/facebookincubator/fizz.git" | ||
| GIT_TAG "v2020.05.18.00" |
There was a problem hiding this comment.
nit: Define all the thirdparty version variables in one place in the CMakeLists? Easier to make changes in the future if someone wants to try out a specific git tag..
| ExternalProject_Add( | ||
| facebook-folly-proj | ||
| GIT_REPOSITORY "https://github.com/facebook/folly.git" | ||
| GIT_TAG "v2020.05.18.00" |
There was a problem hiding this comment.
Any reason you bumped up the versions from the master branch? Probably better to use the same versions and upgrade them in a separate commit to limit the scope of changes?
There was a problem hiding this comment.
I believe the master branch had 2017.09.04, which gave me concern due to its age. This was one of two reasons why I submitted this as a draft ( the other being that not all tests passed ). It allows me to get feedback on this very item so thanks for pointing this out.
-
On one hand I can keep the versions as-is from master ( thus reducing or eliminating the code changes ). It would be a smaller PR, less to review, and if I were the reviewer I'd probably prefer that :) Large PRs like this are sometimes difficult to review IMO and the version bumps cause scope creep.
-
On the other hand bumping versions after such an age gap did require some code resolutions, which I think when the are combined in this PR give perspective.
The other consideration for this when I approached this I'm on a much newer toolchain and am building outside of docker, so managing the numerous dependencies of folly/wangle caused compatibility concerns. I haven't fully validated that this will be a problem so splitting up this PR would make total sense to me.
I can extract the code changes required for the version bump very easily, and ultimately this is why I posted it as a draft-- so I can get this feedback early and get opinions on this.
Would appreciate @joshelser 's take too RE version bumping.
Look forward to your responses! Thanks!
There was a problem hiding this comment.
Ya, I was more talking about the scope creep and a reviewing perspective. I'm totally up for bumping the version but I think it'd be nice to do it in a follow up patch _ if it is not too much work for you_. I'll let @joshelser weigh in, he has more context here than I do.
There was a problem hiding this comment.
Yep. I'll take a look. I updated folly ( which resulted in a lot of other changes ) because of this
https://github.com/facebook/folly/blob/v2017.09.04.00/CMakeLists.txt#L24
My preference was to use CMake if and when possible but it definitely becomes difficult as we aim to control the dependency tree.
There was a problem hiding this comment.
On the other hand bumping versions after such an age gap did require some code resolutions, which I think when the are combined in this PR give perspective.
From what I remembered the last time I was in this code, I assumed the version bumps were necessitated to avoid making changes for the sake of not updating folly/wangle (et al) versions. Totally OK if this gets "big" just for the sake of getting back to a good position.
| GIT_REPOSITORY "https://github.com/facebook/folly.git" | ||
| GIT_TAG "v2020.05.18.00" | ||
| SOURCE_DIR "${BINARY_DIR}/dependencies/facebook-folly-proj-src" | ||
| CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS} |
There was a problem hiding this comment.
Interestingly I'm not able to build this git tag, tried it separately too, didn't you run into this? (file name FindFmt vs Findfmt)
CMake Error at CMake/folly-deps.cmake:227 (find_package):
No "Findfmt.cmake" found in CMAKE_MODULE_PATH.
Call Stack (most recent call first):
CMakeLists.txt:113 (include)
CMake Warning (dev) at CMake/folly-deps.cmake:227 (find_package):
Findfmt.cmake must either be part of this project itself, in this case
adjust CMAKE_MODULE_PATH so that it points to the correct location inside
its source tree.
Or it must be installed by a package which has already been found via
find_package(). In this case make sure that package has indeed been found
and adjust CMAKE_MODULE_PATH to contain the location where that package has
installed Findfmt.cmake. This must be a location provided by that package.
This error in general means that the buildsystem of this project is relying
on a Find-module without ensuring that it is actually available.
Call Stack (most recent call first):
CMakeLists.txt:113 (include)
This warning is for project developers. Use -Wno-dev to suppress it.
There was a problem hiding this comment.
I had to trace my steps back but I already had libfmt-dev, which installs the configuration mechanism (in my case ) within the cmake root module path.
Folly/Fizz/Wangle have a lot of dependencies, but if this was the only other one you encountered it would not be difficult to add a similar mechanism for installing libfmt. thanks for pointing this out.
There was a problem hiding this comment.
I had libfmt-dev too but I probably messed up something. I tried again and I could get past that, but then I ran into this..
In file included from hbase-native-client/build/dependencies/facebook-folly-proj-src/folly/logging/Logger.h:22:0,
from hbase-native-client/build/dependencies/facebook-folly-proj-src/folly/logging/BridgeFromGoogleLogging.cpp:20:
hbase-native-client/build/dependencies/facebook-folly-proj-src/folly/logging/LogStreamProcessor.h:19:10: fatal error: fmt/core.h: No such file or directory
#include <fmt/core.h>
^~~~~~~~~~~~
Turns out the problem is this facebook/folly#1263. libfmt-dev in ubuntu 18.04 doesn't install the right headers. If we end up dockerizing this on ubuntu, we may run into this problem in which case we may have to install fmt dependency like other dependencies, just FYI.
There was a problem hiding this comment.
Next problem is the "version.h"...
cp: cannot stat '../bin/../../hbase-common/target/generated-sources/native/utils//*': No such file or directory
CMakeFiles/copy_version_h.dir/build.make:57: recipe for target 'CMakeFiles/copy_version_h' failed
make[2]: *** [CMakeFiles/copy_version_h] Error 1
CMakeFiles/Makefile2:67: recipe for target 'CMakeFiles/copy_version_h.dir/all' failed
make[1]: *** [CMakeFiles/copy_version_h.dir/all] Error 2
Makefile:140: recipe for target 'all' failed
make: *** [all] Error 2
I don't see hbase-common in the hbase repo generating a native header file. It only generates a Version.java file.. the following code looks sketchy to me. How did get the version.h in your case?
# Copy the version.h generated from hbase-common/src/saveVersion.sh script via the mvn build
BIN_DIR=$(dirname "$0")
VERSION_SOURCE_DIR="${BIN_DIR}/../../hbase-common/target/generated-sources/native/utils/"
VERSION_DEST_DIR="${BIN_DIR}/../include/hbase/utils/"
cp $VERSION_SOURCE_DIR/* $VERSION_DEST_DIR/
(I looked at the git history for that file and it looks like it never generated a header file, I don't know what this is referring to, also the directory paths look wrong).
There was a problem hiding this comment.
Its probably worth noting, I ran into similar problems (as libfmt) with gmock and gtest. apt install libgtest doesn't install the sharedlibs, I had to manually compile and install again (weird).
There was a problem hiding this comment.
Yeah version is an issue I'd like to address with cmake getting a known tag. I manually created a version.h for the time being. I started working on a solution for this, but then the size of the PR would grow even more. My hope is to leverage an internal java project that runs a mini cluster and relies on maven to build a jar we can run for integration tests. Right now there is a relative path expecting classpath information to be valid.
libfmt issues are one I think I need to address. In the case of gtest did you use libgtest-dev? My hope is to control these ( like I started doing with Sodium ). The ultimate issue I would hope to avoid is not being in control of the dependency tree. I think having a build mode for libhabaseclient.so that allows for static linking ( with perhaps GLIB as the only dependency ) will allow better control over not only the dependency tree, but also allow the client to be used across a variety of systems without regard to dependencies on those machines.
This would also allow python bindings to eventually be a breeze and they can be distributed via pypi with relative ease.
Finally, RE the copy_version [1] agree it's reliant on a relative path that probably won't exist for most people. A more sustainable solution would be to have a variable specifying an hbase release target, and we generate the version file from the tag.
I think for the non-draft PR I would probably make the dependency reliance (BUILD_FB_DEPENDENCIES, BUILD_ZOOKEEPER) off by default. That would help to shrink the PR some as the need to download all dependencies and solve every problem is minimized.
[1] https://github.com/apache/hbase-native-client/blob/master/bin/copy-version.sh
There was a problem hiding this comment.
Yeah version is an issue I'd like to address with cmake getting a known tag. I manually created a version.h for the time being. I started working on a solution for this, but then the size of the PR would grow even more.
Finally, RE the copy_version [1] agree it's reliant on a relative path that probably won't exist for most people. A more sustainable solution would be to have a variable specifying an hbase release target, and we generate the version file from the tag.
Thanks. To keep things simple, IMHO it is also reasonable to pull the version.h from hbase source in the parent directory (basically the current approach). Just that mvn compilation in hbase project is not generating that header file. Let me know if you think this is a reasonable approach and I can fix it in a separate patch and you can integrate it with your change (that'll keep your changes simple for now).
My hope is to leverage an internal java project that runs a mini cluster and relies on maven to build a jar we can run for integration tests. Right now there is a relative path expecting class-path information to be valid.
Agreed.
libfmt issues are one I think I need to address. In the case of gtest did you use libgtest-dev?
Yes, I was using the dev library. I think this is a known problem, see the second answer on this page.
https://stackoverflow.com/questions/13513905/how-to-set-up-googletest-as-a-shared-library-on-linux
but also allow the client to be used across a variety of systems without regard to dependencies on those machines.
+1. I think an ideal end state would be a prebuilt toolchain for various commonly used platforms we just download them on the fly during compilation.
There was a problem hiding this comment.
apache/hbase#1794 is what I was talking about. I couldn't add you to the PR for review but feel free to take a look. Hope it helps you in this effort.
There was a problem hiding this comment.
My hope is to leverage an internal java project that runs a mini cluster and relies on maven to build a jar we can run for integration tests.
This would be great. https://github.com/apache/hbase/tree/master/hbase-testing-util should have "enough" of what's needed to get going. Shout when you're ready for this and we can help. The hardest part will be explicitly listing all of the test scope dependencies, as maven won't naturally bring them in transitively (super fun -- test-scope deps have no transitive deps).
If we end up dockerizing this on ubuntu, we may run into this problem in which case we may have to install fmt dependency like other dependencies, just FYI.
I think Docker is going to be a "requirement" for the rest of the HBase team to try to be "helpful". I'm not sure if there are any frameworks out there which would help us do the same setup+build+test across multiple OS's docker containers. That said, having just "one" platform which works is more than we have now :)
There was a problem hiding this comment.
Totally agree. I'm going to submit a separate PR where it should have just the cmake changes without changing dependencies. Since I already have these changes that should prep a separate PR for that change set. For now I'm working through compilation issues on the older folly.
| ExternalProject_Add( | ||
| facebook-wangle-proj | ||
| GIT_REPOSITORY "https://github.com/facebook/wangle.git" | ||
| GIT_TAG "v2020.05.18.00" |
|
In addition to the minicluster automation for testing, shout when you're ready just to get a "real" hbase up and running. I imagine you can figure it out, but we should be around to help as needed with the accumulo->hbase "hump". apache-hbase.slack.com is good, too, if you have a quick question. |
|
superseded. |
Has test failures.