Add absl to CMake and Bazel builds. #1301

pifon2a · 2018-07-19T07:50:49Z

Having to support both CMake and Bazel builds, we are adding it as a copy in third_party/ directory.

sebastianklose · 2018-07-19T08:04:41Z

Is there a reason why this is only a subset of whole abseil? For example I don't see the memory or synchronization subfolders which will probably be most interesting for us.

sebastianklose

I think you pulled the wrong rev? This is only including the first three subfolders of absl.

NVM - seems to be an UI issue ...

wohe · 2018-07-19T08:55:25Z

@CodeArno FYI

CodeArno · 2018-07-19T10:21:09Z

(sorry about my previous comment if you got an email about it, I got confused about the repositories).
This looks great! :)

wohe · 2018-07-19T10:24:48Z

Following up on @CodeArno's previous comment. Do we need to copy Abseil here for CMake to work? Or what is the reasoning? Should this rationale be added to the PR description?

pifon2a · 2018-07-19T10:45:53Z

@wohe We can either copy it or add it as a sub-module. In the latter case, the users will have to call git submodule.

ojura · 2018-07-19T12:01:21Z

@pifon2a there is a third option which is a lot like this one (pasting a copy of the 3rd party repo inside your repo), and that is using git subtree. I would recommend git subtree add ... --squash. This keeps the whole third party repo as a single squashed commit in some folder (the subtree), while maintaining metadata (original upstream commit ID) in the commit description so you can easily pull from upstream using git subtree pull.

gaschler · 2018-07-19T12:04:29Z

@pifon2a Since absl already depends on cmake 3.1, we can also use cmake's externalproject_add, which I find better than git submodules, especially for an external dependency that we rarely update.

ojura · 2018-07-19T17:24:36Z

I dimly remember wanting to smash something every time I tried using externalproject_add.

cschuet · 2018-07-24T00:19:59Z

CMakeLists.txt

+
+find_package(Eigen3)
+if(NOT EIGEN3_FOUND)
+  set(EIGEN3_INCLUDE_DIRS ${/usr/include/eigen3})


is this unrelated to absl?

When I install cmake 3.x in trusty/jessie, Cartographer complains that it cannot find FindEigen3.cmake, although Ceres was compiled just prior to that. I do not know whether it is a good solution or not. @gaschler PTAL, because CMake scares and confuses me.

This is unrelated to abseil and should be a separate PR where only the cmake version is bumped.
(It's hard to say why a newer cmake makes the package search stricter.)

So, no change here, please. (Setting an include path manually is not portable and wouldn't be part of a cmake style guide.)
A proper solution would be to add a FindEigen3.cmake to cmake/modules similar to (but much shorter than) what ceres does: https://github.com/ceres-solver/ceres-solver/blob/master/cmake/FindEigen.cmake

Also, when bumping cmake, it would be good to bump cmake_minimum_required in this file and update the install instructions in the documentation.

gaschler

Thank you.
Do you have a plan to verify (and update if necessary)

Catkin build of cartographer_ros/_fetch,
the ROS release build https://github.com/ros-gbp/cartographer-release/
manual install instructions following documentation?

gaschler · 2018-07-24T09:19:56Z

scripts/install_debs_cmake.sh

+  sudo apt-get install python-software-properties apt-file -y
+  sudo apt-file update
+  sudo apt-get install software-properties-common -y
+  sudo add-apt-repository ppa:george-edison55/cmake-3.x


no need to install personal (untrusted) packages,
just install cmake3 https://packages.ubuntu.com/en/trusty/cmake3

oh, thanks, done

gaschler · 2018-07-24T09:21:45Z

cmake/modules/FindAbseil.cmake

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# DO NOT SUBMIT, would break on CMake versions.


drop this comment (after bumping cmake)

gaschler · 2018-07-24T09:23:08Z

CMakeLists.txt

+
+find_package(Eigen3)
+if(NOT EIGEN3_FOUND)
+  set(EIGEN3_INCLUDE_DIRS ${/usr/include/eigen3})


Also, when bumping cmake, it would be good to bump cmake_minimum_required in this file and update the install instructions in the documentation.

gaschler · 2018-07-24T09:24:20Z

cmake/modules/FindAbseil.cmake

+    "${ABSEIL_PROJECT_BUILD_DIR}/absl/numeric/libabsl_numeric.a"
+    "${ABSEIL_PROJECT_BUILD_DIR}/absl/strings/libabsl_strings.a"
+    "${ABSEIL_PROJECT_BUILD_DIR}/absl/types/libabsl_bad_any_cast.a"
+    "${ABSEIL_PROJECT_BUILD_DIR}/absl/types/libabsl_any.a"


sort alphabetically

gaschler · 2018-07-24T09:25:56Z

cmake/modules/FindAbseil.cmake

+  set(ABSEIL_INCLUDE_DIRS ${ABSEIL_PROJECT_SRC_DIR})
+  set(ABSEIL_LIBRARY_PATH
+    "${ABSEIL_PROJECT_BUILD_DIR}/absl/synchronization/${CMAKE_STATIC_LIBRARY_PREFIX}absl_synchronization${CMAKE_STATIC_LIBRARY_SUFFIX}")
+  set(dependent_libraries


style: rename to ABSEIL_DEPENDENT_LIBRARIES

gaschler · 2018-07-24T09:28:07Z

cmake/modules/FindAbseil.cmake

+    ${CMAKE_CURRENT_BINARY_DIR}/${ABSEIL_PROJECT_NAME}/src/${ABSEIL_PROJECT_NAME}-build)
+  set(ABSEIL_INCLUDE_DIRS ${ABSEIL_PROJECT_SRC_DIR})
+  set(ABSEIL_LIBRARY_PATH
+    "${ABSEIL_PROJECT_BUILD_DIR}/absl/synchronization/${CMAKE_STATIC_LIBRARY_PREFIX}absl_synchronization${CMAKE_STATIC_LIBRARY_SUFFIX}")


drop the prefix and suffix (for consistency with dependent_libraries), this isn't portable anyways

gaschler · 2018-07-24T09:29:24Z

cmake/modules/FindAbseil.cmake

+    PROPERTIES IMPORTED_LOCATION
+    ${ABSEIL_LIBRARY_PATH}
+    INTERFACE_LINK_LIBRARIES
+    "${dependent_libraries}"


@hauke76 Is there a cleaner way to glob all static libraries into one target?
ABSEIL_LIBRARY_PATH=synchronization/libabsl_synchronization.a was an arbitrary choice only because this is the first one we will use.

As discussed offline, this is fine.

pifon2a requested review from sebastianklose and cschuet July 19, 2018 07:50

googlebot added the cla: yes label Jul 19, 2018

sebastianklose suggested changes Jul 19, 2018

View reviewed changes

sebastianklose approved these changes Jul 19, 2018

View reviewed changes

pifon2a force-pushed the absl branch from c5bb154 to 86621f6 Compare July 20, 2018 14:06

cschuet force-pushed the absl branch from 86621f6 to c5bb154 Compare July 20, 2018 14:06

pifon2a force-pushed the absl branch 4 times, most recently from a50d9b5 to a02a547 Compare July 23, 2018 19:01

pifon2a added 3 commits July 23, 2018 22:00

Add Absl-support to Bazel and CMake.

cfd60d8

Fix CMake.

ed38f0a

Fix jessie.

6d2fcdc

pifon2a force-pushed the absl branch 2 times, most recently from 6d2fcdc to 9e18129 Compare July 23, 2018 23:43

Fix jessie.

1c3cf52

pifon2a force-pushed the absl branch from 9e18129 to 1c3cf52 Compare July 23, 2018 23:44

cschuet approved these changes Jul 24, 2018

View reviewed changes

pifon2a and others added 2 commits July 24, 2018 06:38

Merge branch 'master' into absl

85c2fdc

Merge.

c2a92c8

pifon2a requested a review from gaschler July 24, 2018 04:47

gaschler reviewed Jul 24, 2018

View reviewed changes

pifon2a added 3 commits July 25, 2018 10:41

Address some of the comments.

1ef5a34

Merge remote-tracking branch 'upstream/master' into absl

11788be

Add Eigen.

0f8207c

pifon2a force-pushed the absl branch from 0ae33e0 to 0f8207c Compare July 25, 2018 10:50

pifon2a and others added 3 commits July 25, 2018 13:10

Merge branch 'master' into absl

68cc4be

Merge remote-tracking branch 'upstream/master' into absl_local

72aad87

Merge with upstream.

ae13877

cschuet force-pushed the absl branch from ae13877 to 68cc4be Compare July 25, 2018 20:15

pifon2a changed the title ~~Add absl to third_party.~~ Add absl to CMake and Bazel builds. Jul 25, 2018

pifon2a force-pushed the absl branch from 7940ba9 to ae13877 Compare July 25, 2018 20:16

pifon2a merged commit e3a7f39 into cartographer-project:master Jul 26, 2018

pifon2a deleted the absl branch July 26, 2018 04:49

gaschler mentioned this pull request Jul 26, 2018

Reuse absl::Mutex #1138

Closed

ojura mentioned this pull request Jul 28, 2018

Fix Abseil build command #1350

Merged

akshay-prsd pushed a commit to BossaNova/cartographer that referenced this pull request Jul 18, 2019

Add absl to CMake and Bazel builds. (cartographer-project#1301)

392b735

schayapathy pushed a commit to schayapathy/cartographer that referenced this pull request Aug 15, 2019

Add absl to CMake and Bazel builds. (cartographer-project#1301)

46eb0cb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add absl to CMake and Bazel builds. #1301

Add absl to CMake and Bazel builds. #1301

pifon2a commented Jul 19, 2018 •

edited

sebastianklose commented Jul 19, 2018

sebastianklose left a comment •

edited

wohe commented Jul 19, 2018

CodeArno commented Jul 19, 2018

wohe commented Jul 19, 2018

pifon2a commented Jul 19, 2018

ojura commented Jul 19, 2018 •

edited

gaschler commented Jul 19, 2018

ojura commented Jul 19, 2018

cschuet Jul 24, 2018

pifon2a Jul 24, 2018

gaschler Jul 24, 2018

gaschler Jul 24, 2018

gaschler left a comment

gaschler Jul 24, 2018

pifon2a Jul 25, 2018

gaschler Jul 24, 2018

pifon2a Jul 25, 2018

gaschler Jul 24, 2018

gaschler Jul 24, 2018

pifon2a Jul 25, 2018

gaschler Jul 24, 2018

pifon2a Jul 25, 2018

gaschler Jul 24, 2018

pifon2a Jul 25, 2018

pifon2a Jul 25, 2018

gaschler Jul 24, 2018

gaschler Jul 25, 2018

Add absl to CMake and Bazel builds. #1301

Add absl to CMake and Bazel builds. #1301

Conversation

pifon2a commented Jul 19, 2018 • edited

sebastianklose commented Jul 19, 2018

sebastianklose left a comment • edited

Choose a reason for hiding this comment

wohe commented Jul 19, 2018

CodeArno commented Jul 19, 2018

wohe commented Jul 19, 2018

pifon2a commented Jul 19, 2018

ojura commented Jul 19, 2018 • edited

gaschler commented Jul 19, 2018

ojura commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaschler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pifon2a commented Jul 19, 2018 •

edited

sebastianklose left a comment •

edited

ojura commented Jul 19, 2018 •

edited