Use Libraries in the RocksDB Makefile Build #6660
Conversation
mrambacher
force-pushed
the
Makefile
branch
2 times, most recently
from
a9c6e4f
to
c9aa8fc
Compare
mrambacher
changed the title
|
I'm digging into something else at the moment, but I get this (yes, I did make clean): util/heap_test.cc:136: error: undefined reference to 'gflags::ParseCommandLineFlags(int*, char***, bool)' |
What platform? I have done builds on RedHat and MacOS and do not see this error. |
Linux/x86_64. Doesn't happen with ROCKSDB_NO_FBCODE=1, so it must be a specific incompatibility with build_tools/fbcode_config_platform007.sh (used by build_detect_platform when building on Facebook machines) |
|
Do the tests other than heap_test build successfully? How can I see the command for a successful versus failed build? At first glance, I am not sure how/where the command to build heap_test has changed. |
|
Some pass, some fail. Although I told you that TARGETS isn't really affected by Makefile changes, these changes are deep enough that the buckifying has been gutted. Sorry about that. To accept this, I think we need to break this up in into pieces. But also, the size savings is not as much as I had hoped for. About 80% of the space is unit test executables. Since the .o files for the tests (test-specific parts) are only about 5%, we should be able to build a unified test executable that saves almost all of that 80%. We would just need to make the defined "main" functions either a true main or sub-main depending on compile time parameter, implement some resigtry+dispatch based on argv[0], and generate symlinks from old test executable names to unified test executable. And probably resolve a few new linking conflicts between tests. Note that we would want the test subset CI builds only to build and unify the necessary subset. (Not hygienic for incremental re-builds, but OK as it's intended for CI.) |
So that we can update buckifier incrementally, rather than in a giant leap. |
Probably makes sense to simply use gtest as the registry and don't try to preserve usability as segregated executables for unified unit test executable. (But maybe keep ability to build those.) |
|
As another point of reference, if I build RocksDB as a shared library and build the tests and tools against the shared libraries, the size drops substantially. The shared libraries are missing a few tests (about 6 from an older Makefile) but gives you an idea of the scope. Release builds are also around 1/6 the size. |
That's not available in these changes though? (I haven't figured out how SHARED_TEST_LIBRARY is referenced by any targets.) |
|
I pushed a new commit with the a shared mode for the tests. I figured that would be too much for a single PR. |
|
Now I can run 'make check' with ~90% space savings vs. static linking. That's more motivation for me to dig into some of the compatibility issues. (I haven't tested my change on clang, just sharing it so that test binaries run as usual but with dynamic linking.) |
My inclination was to do the minimum for unit tests to work as they have historically, and they should always be run with DEBUG_LEVEL>0 anyway. I don't think we want to manipulate rpath for binaries that even might be installed (release build), because of security risk. I don't know enough about rpath to know if "." works. And that might ask the question of which is more likely: changing the absolute path of your repo clone without rebuild or running executables from path other than ./ (I consider it more kosher to break the former case than the latter.) |
|
Has any more thought been put into this PR? If we can head down this path and do the customizable work, we may be able to solve quite a few problems. With some effort, the Java system could be more pluggable for one. And components that may be add-ons (like HDFS, ClockCache, CuckooTable, Cassandra, etc) could be removed from the core and put into separate libraries. |
|
I still need to fix buckifier. Shouldn't be hard |
|
Anyone know how to fix the java_test build on Travis? (UnsatisfiedLinkError on _ZTIN7rocksdb6LoggerE) |
|
I fixed this problem on my local machine so that rocksdbjava and rocksdbjavastatic both build now. It would be really nice to further clean up the Java build (in a later PR) so that it just used the shared library build (rather than rebuilding everything itself). |
|
I saw another PR with a similar error (6997), so a merge or rebase might help. |
… a list of objects. This change substantially reduces the size of the objects produced. Added OBJ_DIR option to Makefile to allow objects to be placed in an alternative location. This value is currently unused (set to the current directory) but will allow objects to be placed in different directories to simplify the build process when building multiple targets (like LITE, release/debug, shared/static, java, etc).
This change adds a LIB_MODE flag to control how RocksDB binaries, libraries, and tests are built. Using shared libraries reduces the size of the executables substantially. Additionally, shared libraries will make it easier/cleaner to develop and deploy extension libraries to RocksDB.
There was a problem hiding this comment.
@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@mrambacher has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@mrambacher has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
pdillinger
changed the title
| EXEC_LDFLAGS += -Wl,-rpath="$(PWD)" | ||
| endif | ||
| # So that binaries are executable from build location, in addition to install location | ||
| EXEC_LDFLAGS += -Wl,-rpath='$$ORIGIN' |
There was a problem hiding this comment.
This is failing to compile on MacOS:
CCLD cache_bench
ld: unknown option: -rpath=$ORIGIN
clang: error: linker command failed with exit code 1 (use -v to see invocation)
If I specify -rpath . on the Mac (no =), it works. I have not gotten it to work with ORIGIN however
There was a problem hiding this comment.
I wonder if this option should go into build_detect_platform as part of PLATFORM_LDFLAGS?
There was a problem hiding this comment.
Except it's EXEC_LDFLAGS (only for linking executables), so PLATFORM_EXEC_LDFLAGS?
| @@ -215,7 +231,8 @@ LIB_SOURCES += utilities/env_librados.cc | |||
| LDFLAGS += -lrados | |||
| endif | |||
|
|
|||
| AM_LINK = $(AM_V_CCLD)$(CXX) $^ $(EXEC_LDFLAGS) -o $@ $(LDFLAGS) $(COVERAGEFLAGS) | |||
| AM_LINK = $(AM_V_CCLD)$(CXX) $(patsubst lib%.a, -l%, $(patsubst lib%.$(PLATFORM_SHARED_EXT), -l%, $^)) $(EXEC_LDFLAGS) -o $@ -L. $(LDFLAGS) $(COVERAGEFLAGS) | |||
There was a problem hiding this comment.
I think the -L. should be before the libraries (patsubst).
There was a problem hiding this comment.
It failed for me the way you had it so I semi-reverted. b94b715
There was a problem hiding this comment.
But moving just the -L. does seem ok testing it just now. (Now for follow-up PR)
|
@pdillinger merged this pull request in 80f71b5. |
Summary: Follow-up to #6660. Release build had linker error. fbcode+clang+shared build was erroring on unused parameter '-nostdinc'. Fixes #7061 Pull Request resolved: #7062 Test Plan: make release, USE_CLANG=1 LIB_MODE=shared make check, etc. Reviewed By: siying Differential Revision: D22335663 Pulled By: pdillinger fbshipit-source-id: 261cd959ca1f6c273dc763a70020a535ba8e81de
Summary: Was broken by facebook#6660 Test Plan: CI inspection
Summary: Was broken by #6660 Travis times before this change, after 6660: platform_dependent: 17 min group 1: 15 min group 2: 44 min (often timeout on non-x86 or non-Linux) group 3: 31 min group 4: 21 min After this change: TODO Pull Request resolved: #7360 Test Plan: CI inspection Reviewed By: ajkr Differential Revision: D23586917 Pulled By: pdillinger fbshipit-source-id: 4c67cf33180b0b833c39a817e6c1f128727941d2
…cebook#6660 This was fixed in facebook#7359 and this commit is just a partial back-port of the necessary parts of that PR.
This was fixed in #7359 and this commit is just a partial back-port of the necessary parts of that PR.
Summary: Was broken by facebook#6660 Travis times before this change, after 6660: platform_dependent: 17 min group 1: 15 min group 2: 44 min (often timeout on non-x86 or non-Linux) group 3: 31 min group 4: 21 min After this change: TODO Pull Request resolved: facebook#7360 Test Plan: CI inspection Reviewed By: ajkr Differential Revision: D23586917 Pulled By: pdillinger fbshipit-source-id: 4c67cf33180b0b833c39a817e6c1f128727941d2
Change the linking of tests/tools to be against a library rather than a list of objects. This change substantially reduces the size of the objects produced.
peterd clean repo size: 264M
Before this change, with make all: 40G
After this change, with make all: 28G
With make LIB_MODE=shared all: 7.0G
The list of TESTS was changed from being hard-coded to generated from the test sources variable. Note that there are some test sources that are not built as tests (though the set of tests is identical to the previous version).
Added OBJ_DIR option to Makefile to allow objects to be placed in an alternative location. By default, OBJ_DIR is the same as before ("./").
This change is a precursor to being able to build/run the tests/tools linked against static libraries. Additionally, it should be possible to clean up and merge some of the rules for building tests and the like if so desired.