Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Verdant/boost serialization #1409

Merged
merged 11 commits into from
Jan 24, 2023

Conversation

kartikarcot
Copy link

This PR introduces a new flag to disable boost serialization functionality. Another move towards being able to build GTSAM without boost. Implemented this mainly through a compile time definition that conditionally deactivates all references to boost serialization.

Question for the reviewers:

  • Where should this flag be placed? I chose to put it in the main CMakelists.txt

Note:
If you see some code changes in the diff that don't seem to be part of this PR, it's because it has changes from another branch (feature/remove_boost_in_Values) and will go away when that branch gets merged, which will happen before this will go in.

@dellaert dellaert requested a review from ProfFan January 23, 2023 07:18
@dellaert
Copy link
Member

@ProfFan tagged you for review and advice. We might do another PR down the line with your help to include "cereal", but this one at least allows compilation without boost for the most part. After this PR, I envision only one or more two PRs are needed to make that happen.

@dellaert
Copy link
Member

CI fails, and somehow the diff did not update. I'd recommend (a) merging in develop again, and (b) - before pushing- figuring out what is killing the CI.

@ProfFan
Copy link
Collaborator

ProfFan commented Jan 23, 2023

CMake Error at cmake/GtsamTesting.cmake:216 (add_executable):
[62](https://github.com/borglab/gtsam/actions/runs/3984064957/jobs/6829916408#step:5:63)
  add_executable cannot create target "check_discrete_program" because
[63](https://github.com/borglab/gtsam/actions/runs/3984064957/jobs/6829916408#step:5:64)
  another target with the same name already exists.  The existing target is
[64](https://github.com/borglab/gtsam/actions/runs/3984064957/jobs/6829916408#step:5:65)
  an executable created in source directory
[65](https://github.com/borglab/gtsam/actions/runs/3984064957/jobs/6829916408#step:5:66)
  "/home/runner/work/gtsam/gtsam/gtsam/base/tests".  See documentation for
[66](https://github.com/borglab/gtsam/actions/runs/3984064957/jobs/6829916408#step:5:67)
  policy CMP0002 for more details.
[67](https://github.com/borglab/gtsam/actions/runs/3984064957/jobs/6829916408#step:5:68)
Call Stack (most recent call first):
[68](https://github.com/borglab/gtsam/actions/runs/3984064957/jobs/6829916408#step:5:69)
  cmake/GtsamTesting.cmake:30 (gtsamAddTestsGlob_impl)
[69](https://github.com/borglab/gtsam/actions/runs/3984064957/jobs/6829916408#step:5:70)
  gtsam/geometry/tests/CMakeLists.txt:9 (gtsamAddTestsGlob)
[70](https://github.com/borglab/gtsam/actions/runs/3984064957/jobs/6829916408#step:5:71)

[71](https://github.com/borglab/gtsam/actions/runs/3984064957/jobs/6829916408#step:5:72)

[72](https://github.com/borglab/gtsam/actions/runs/3984064957/jobs/6829916408#step:5:73)
CMake Error at cmake/GtsamTesting.cmake:217 (target_link_libraries):
[73](https://github.com/borglab/gtsam/actions/runs/3984064957/jobs/6829916408#step:5:74)
  Attempt to add link library "CppUnitLite" to target
[74](https://github.com/borglab/gtsam/actions/runs/3984064957/jobs/6829916408#step:5:75)
  "check_discrete_program" which is not built in this directory.

@kartikarcot
Copy link
Author

That's weird I don't remember modifying GtsamTesting.cmake

@kartikarcot
Copy link
Author

But I did change the files that called it :)

Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

LGTM !!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants