Streamline CMake and build system, move to newer protobuf#150
Streamline CMake and build system, move to newer protobuf#150Otterverse merged 17 commits intoviam-modules:mainfrom
Conversation
viambot
added
the
safe to test
jeremyrhyde
left a comment
jeremyrhyde
left a comment
There was a problem hiding this comment.
Waiting on protobuf update in brew before approving. Plus I have a few questions
| cd viam-cartographer && ./scripts/build_cartographer.sh && ./scripts/build_viam_cartographer_debug.sh | ||
| endif | ||
| build: ensure-submodule-initialized buf build-module | ||
| cd viam-cartographer && cmake -Bbuild -G Ninja ${EXTRA_CMAKE_FLAGS} && cmake --build build |
There was a problem hiding this comment.
we dont have to run the ninja command anymore?
There was a problem hiding this comment.
cmake --build build runs it. (It says run a build in the directory "build", if the double-naming is confusing.) But if you ever switch to another cmake tooling instead of ninja, it'll run that too.
There was a problem hiding this comment.
[q] so if we wanted separate build directories for different architectures, we would modify cmake --build build here?
[nit] if so, could we add a comment here for this, as that's something the slam team wants to look into doing in the future?
There was a problem hiding this comment.
Yeah, in theory you can just change -Bbuild to -Bbuild-`uname -m` (or whatever name scheme you want, and it'll configure the output there. Then you can actually compile with the same thing just changing build to build-`uname -m` in the final command as well.
More realistically, you'd just want to make them variables in Make, then you can set the BUILD_DIR variable once and use everywhere. Do note that some of the cartographer tests in go are written using cgo and depending on explicit paths (which is why I had to change one) so there's not as easy of a way to handle that.
Likewise, it's not just the cmake part that would need to be per-architecture, as the golang stuff would be as well, so it's likely a bit of a more complex change you really want, to completely split out architectures if you really want to be able to build both in the same code space (without just doing "make clean" instead.)
jeremyrhyde
left a comment
jeremyrhyde
left a comment
There was a problem hiding this comment.
Note: When I run the default make command from a new repo, it now downloads artifacts and stops there.
viambot
added
safe to test
Otterverse
left a comment
Otterverse
left a comment
There was a problem hiding this comment.
Updated the makefile and renamed the setup script.
viambot
added
safe to test
viambot
added
safe to test
jeremyrhyde
left a comment
jeremyrhyde
left a comment
There was a problem hiding this comment.
LGTM, thanks for answering all my questions James!
There was a problem hiding this comment.
First off:
- Thank you so much for this update @Otterverse ! I learned a lot about make and cmake from your PR.
- I've left a few questions
| @@ -1,30 +0,0 @@ | |||
| # Copyright 2018 The Cartographer Authors | |||
There was a problem hiding this comment.
why don't we need this anymore?
| @@ -1,143 +0,0 @@ | |||
| # Copyright 2016 The Cartographer Authors | |||
There was a problem hiding this comment.
why don't we need this anymore?
viambot
added
safe to test
Otterverse
left a comment
Otterverse
left a comment
There was a problem hiding this comment.
Answered nick's questions, and removed that stray echo. Also rebased on main. I think it's good hopefully.
|
@Otterverse this looks great to me! Thanks for all the work you put in & for making the cartographer build system much more reliable & maintainable. |
…)" This reverts commit ec07b69.
6 participants
Should be ready for final review. Changes a lot of stuff, but tested a lot so far (but could use more) More notes on testing in comments below.