You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@quickstep.apache.org by navsan <gi...@git.apache.org> on 2016/06/09 00:20:06 UTC
[GitHub] incubator-quickstep pull request #13: Add option to build using shared libra...
GitHub user navsan opened a pull request:
https://github.com/apache/incubator-quickstep/pull/13
Add option to build using shared libraries.
Modify the build process to allow optional use of shared libraries instead of the static libraries we use today. The benefits are
- much smaller build directory size (I\u2019m seeing < 5GB for release mode, as opposed to the usual 40GB with static libs)
- slightly faster build time (went from 18-20min to 14-15min on the CloudLab machine with 40 jobs)
- reduced memory consumption (average memory consumption during build went from 12GB to 8.5GB)
To use this, just turn on the CMake flag BUILD_SHARED_LIBS. Currently, we don't support the use of both shared and static libraries in the build, because that leads to some really nasty bugs with the common third party libraries (GFlags, GLog, ProtoBuf) due to double-linking.
All tests passed locally (both with the flag turned on and off).
Also, see the attached shell script [build.shared.sh.txt](https://github.com/apache/incubator-quickstep/files/305928/build.shared.sh.txt) (renamed to .txt to attach to this PR). In a clean build directory, it runs the build commands, outputs them to a log file with timestamps (so that we can profile the build process if we want to), records the memory and cpu load statistics periodically, and computes the time taken and the average memory consumption during the build.
All of the changes in the commit are to do with linking to gflags library, since its name depends on whether it's shared or static.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/incubator-quickstep build_shared
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-quickstep/pull/13.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #13
----
commit 7eb9cf762f3c3ed926cc33f653bed4a868b2873c
Author: Navneet Potti <na...@gmail.com>
Date: 2016-06-08T21:59:07Z
Add option to build using shared libraries.
commit 520b4e0353c4675f9a9fc30d79da6fd7398c2496
Author: Navneet Potti <na...@gmail.com>
Date: 2016-06-08T22:51:23Z
Oops, missed a file
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #13: Add option to build using shared libra...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/13#discussion_r66369808
--- Diff: cli/CMakeLists.txt ---
@@ -30,6 +30,12 @@ if(LIBNUMA_FOUND)
set(QUICKSTEP_HAVE_LIBNUMA TRUE)
endif()
+if (BUILD_SHARED_LIBS)
+ set(GFLAGS_LIB_NAME gflags_nothreads-shared)
+else()
+ set(GFLAGS_LIB_NAME gflags_nothreads-static)
+endif()
--- End diff --
Try remove this part, as I believe `cmake` is smart enough to figure out the variable `GFLAGS_LIB_NAME`.
Otherwise, try use `options(GFLAGS_LIB_NAME ...)`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #13: Add option to build using shared libraries.
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/13
As for the double linking, try the following:
```
if (NOT TARGET gflags_nothreads-static)
add_subdirectory("${THIRD_PARTY_SOURCE_DIR}/gflags" "${CMAKE_CURRENT_BINARY_DIR}/third_party/gflags")
include_directories("${CMAKE_CURRENT_BINARY_DIR}/third_party/gflags/include")
endif()
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #13: Add option to build using shared libra...
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/13#discussion_r66370852
--- Diff: cli/CMakeLists.txt ---
@@ -30,6 +30,12 @@ if(LIBNUMA_FOUND)
set(QUICKSTEP_HAVE_LIBNUMA TRUE)
endif()
+if (BUILD_SHARED_LIBS)
+ set(GFLAGS_LIB_NAME gflags_nothreads-shared)
+else()
+ set(GFLAGS_LIB_NAME gflags_nothreads-static)
+endif()
--- End diff --
Yes, as I noted in the comment below, we can definitely do that. But
- CMake is *not* smart enough to figure out the double-linking on its own.
- Specifically for gflags, there's no common target name that we can use to link whichever static/shared automatically so that CMake can resolve the library name on its own. This works for all other libraries, third_party and our own. Just not for gflags.
- The NOT TARGET test will have to be added everywhere that a third_party library is being linked, in all the CMakeLists. That's a *lot* of changes for very limited benefit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #13: Add option to build using shared libra...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-quickstep/pull/13
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #13: Add option to build using shared libra...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/13#discussion_r66370231
--- Diff: cli/CMakeLists.txt ---
@@ -30,6 +30,12 @@ if(LIBNUMA_FOUND)
set(QUICKSTEP_HAVE_LIBNUMA TRUE)
endif()
+if (BUILD_SHARED_LIBS)
+ set(GFLAGS_LIB_NAME gflags_nothreads-shared)
+else()
+ set(GFLAGS_LIB_NAME gflags_nothreads-static)
+endif()
--- End diff --
FYI, I've used both `gflags` and `glog` as git submodules for a private repo, with the condition `NOT TARGET gflags_nothreads-static` to choose whether compile `gflags`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #13: Add option to build using shared libraries.
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on the issue:
https://github.com/apache/incubator-quickstep/pull/13
Yeah, I saw that we do exactly that in tmb. But we'd need to do this for each of the third_party libraries and that gets really messy. Figuring out which library is getting double-linked is extremely hard to do (I spent hours staring at the symbols in the libraries and executables to figure it out). So we'd best not support such a mixed compilation/linking option for now. It's not particularly useful anyway.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #13: Add option to build using shared libra...
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/13#discussion_r66369946
--- Diff: cli/CMakeLists.txt ---
@@ -30,6 +30,12 @@ if(LIBNUMA_FOUND)
set(QUICKSTEP_HAVE_LIBNUMA TRUE)
endif()
+if (BUILD_SHARED_LIBS)
+ set(GFLAGS_LIB_NAME gflags_nothreads-shared)
+else()
+ set(GFLAGS_LIB_NAME gflags_nothreads-static)
+endif()
--- End diff --
You'd think so, but it's not! There's no common target gflags_nothreads that you can link against. It's only a problem with this particular third_party library. I think it's only fixed in the latest version of gflags (https://github.com/gflags/gflags/issues/117).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---