Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

(rebased) New CMake targets-based build configuration #65

Merged
merged 3 commits into from
Jun 16, 2019
Merged

(rebased) New CMake targets-based build configuration #65

merged 3 commits into from
Jun 16, 2019

Conversation

jlblancoc
Copy link
Member

Just a follow-up PR after closing and rebasing all the commits in #27 .

@jlblancoc jlblancoc requested a review from chrisbeall June 12, 2019 22:37
@jlblancoc jlblancoc mentioned this pull request Jun 12, 2019
Closed
@jlblancoc
Copy link
Member Author

Status of this one: all tests pass in Travis (except one that times out).
Will check on Windows too, but for that I'm preparing a separate PR to fix other MSVC-specific errors first...

@chrisbeall
Copy link
Member

Taking a final look now! Meanwhile, please fix the new conflict?

jlblancoc and others added 2 commits June 15, 2019 23:09
@jlblancoc
fix unused parameter warning
ae79c27
@jlblancoc
Refactor build flags via CMake target properties
7f43054 
Also:
- Allow users to edit cmake target build options in the cache variables.
- We had to add project() commands for  gtsam and gtsam_unstable,
the PROJECT_SOURCE_DIR changed, but the root GTSAM_SOURCE_DIR instead.
- Ensure use of standard C++11 (no extensions)
@jlblancoc
Copy link
Member Author

Taking a final look now! Meanwhile, please fix the new conflict?

Great! Was doing it right now. It's fixed now (rebased and push'ed --force to my fork).

chrisbeall
chrisbeall approved these changes Jun 15, 2019
View reviewed changes
Copy link
Member

@chrisbeall chrisbeall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm. Tried it out on Ubuntu 16.04, and verified that public flags propagate to dependent project. Very nice.

@jlblancoc
Copy link
Member Author

@chrisbeall , @dellaert : feel free of merging. Thanks.

@dellaert
Copy link
Member

Excellent, I will merge.

@dellaert dellaert merged commit 4207560 into borglab:develop Jun 16, 2019
@fishbotics
Copy link

Hey y'all, I'm finding that this commit breaks the flag this PR seems to breaks the GTSAM_INSTALL_MATLAB_TOOLBOX flag on Ubuntu 16.04. Specifically, it was the commit titled "Refactor build flags via CMake target properties" that was included in this merge.

I get this error:

CMake Error at cmake/GtsamMatlabWrap.cmake:106 (message):
You must call find_package(GTSAM) before using wrap
Call Stack (most recent call first):
cmake/GtsamMatlabWrap.cmake:59 (wrap_library_internal)
gtsam/CMakeLists.txt:214 (wrap_and_install_library)

I'm trying to figure out what exactly causes the error. If I figure it out, I'll send a PR.

@fishbotics
Copy link

Ah I should add that this was with Cmake 3.14

@fishbotics fishbotics mentioned this pull request Jul 19, 2019
Closed
@fishbotics
Copy link

Here is the PR to fix the bug: #89

varunagrawal added a commit that referenced this pull request Mar 26, 2021
@varunagrawal
Squashed 'wrap/' changes from 9a467794e..96ccdfd0b
c78ca4b 
96ccdfd0b Merge pull request #65 from borglab/fix/special-cases
04c06b7e6 Merge pull request #63 from borglab/fix/cmake
bf2c91bd2 fix issue in template instantiation generator
152dbcb12 test for special cases
d03004b24 fixed the cmake to discover the correct python version and set all corresponding variables
4cf66e0da Merge pull request #61 from borglab/fix/python-variables
80558e35b added more status messages and set the PYBIND11_PYTHON_VERSION each time
73afd1b0a set both sets of Python variables and find python version when including PybindWrap
REVERT: 9a467794e Merge pull request #61 from borglab/fix/python-variables
REVERT: 6bae7af99 added more status messages and set the PYBIND11_PYTHON_VERSION each time
REVERT: 5129cf3b9 set both sets of Python variables and find python version when including PybindWrap

git-subtree-dir: wrap
git-subtree-split: 96ccdfd0b84a4dbf1b3e9ed31b95ebc2758be9cc
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@chrisbeall chrisbeall chrisbeall approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jlblancoc @chrisbeall @dellaert @fishbotics