Revamp build process

[ChrisThrasher]

What I'm trying to achieve:

• Better adhere to CMake conventions
• Better adhere to CMake best practices
• Improve new developer experience
• Reduce build times
• Allow for building all code in one (and only one) binary dir
• Improve readability
• Improve maintainability
• Remove unnecessary complexity

[ChrisThrasher]

This let me more easily build the examples and tests so I could get started looking at the important bits in the core library. The C++ Starter Pack was doing some really weird stuff with CMake that I thought would only serve to confuse future contributors. Hope this helps. I'm eager to dive into the C++.

[ChrisThrasher]

How does building the examples, running the tests, or fixing formatting work now?

Examples are included in the build by default. There's a BUILD_EXAMPLES option which can turn them off. Running tests is as simple as running ctest in the binary directory. The format target still exists to autoformat everything.

I saw that you build the docs differently. Do you know if it is now working?

Docs aren't greatly changed. You still have to build that custom target. They don't work on my machine either.

Will the CI scripts need to be changed in order for them to compile?

Possibly. If you enable workflows for this PR then we can see what broke and I can try to fix it.

[ChrisThrasher]

I updated the README to reflect the new simpler way that everything is built. No need to juggle multiple binary directories anymore.

[giacomo-b]

Hi Chris,

First of all thank you for taking the time to contribute! There is certainly a lot of "boilerplate" build code at the moment and some clean up is needed. And I love to hear that you'd like to dive into the C++ code as well.

You made me realize there's no CONTRIBUTING.md file yet, I'm creating it now and this will hopefully simplify future contributions.

I went through the changes, this is a lot of them! A few notes (and questions):

• I'm not sure whether building all the examples at once is the best option, for two reasons:
  1. A user/contributor may be interested in just running a specific examples. There's only a few now, but there will be several more in the future.
  2. The library is templated, meaning that for every example that gets compiled, all (relevant) code must be recompiled. Again, there's only a few examples at the moment and they rely on different classes; but I don't know how big the library could become.
• Related to the point above, you mention reduced build times. Is this the case?
• Where are the examples built now?
• Why are the FATAL_ERRORs removed from cmake_minimum_required?
• Why was the about.dox file removed?
• I saw a few changes acting on CPM.cmake. Since that's what the library relies on as a cross-platform way to get the dependencies, is this going to affect that in any way?
• This commit is supposed to change the build process, but I noticed that, e.g., the Kalman filter is being changed as well.

On a side note, tests would fail for sure at the moment because the automated build process uses other commands.

[ChrisThrasher]

Unrelated but 165 stars already?? That's amazing! Also could you approve workflows for this MR so the CI pipeline can do its thing?

There is certainly a lot of "boilerplate" build code at the moment and some clean up is needed.

That starter project includes a lot of stuff that you might not need so it'd definitely be good to remove all but the bits you understand and can make use of. Much of it is good, but it imposes some weird CMake practices that I disagree with.

I'm not sure whether building all the examples at once is the best option

Examples can be disabled. If the compile time for the examples is unacceptable then you can disable them but keep building the tests so that you can verify your changes to the core library.

Anyone who ingests this project via add_subdirectory won't have to build the tests or examples because those are automatically disabled in such a case. It's only project developers that have to bother with this.

Related to the point above, you mention reduced build times. Is this the case?

I removed a lot of unnecessary stuff from the matplotplusplus build that was blowing up compile times. Also, because the old one-binary-dir-per-example structure meant lots of extra CPMAddPackage calls, it would be harder to maintain those build options across every instance where the package is included. Now it's just built once and as long as you don't delete your binary dir, that package never gets rebuilt. You don't have to build that every single time you want to build a new example.

Where are the examples built now?

See the ${CMAKE_BINARY_DIR}/examples directory to find the example programs. Their exact location matches where they live in the repo.

Why are the FATAL_ERRORs removed from cmake_minimum_required

This isn't required if you're using a version of CMake newer than 2.6. Any newer version will automatically fail to configure the project if that minimum version requirement isn't met. Using a CMake version older than 2.6 is insane these days and it's safe to say no one will try to use that.

Why was the about.dox file removed?

If I recall correctly it was empty and unused. I could be mistaken though.

I saw a few changes acting on CPM.cmake. Since that's what the library relies on as a cross-platform way to get the dependencies, is this going to affect that in any way?

CPM is still being used. I tweaked how we're using it but it's still doing the job of fetching the dependencies. I've briefly used CPM but am not an expert so there might be better ways to use it.

This commit is supposed to change the build process, but I noticed that, e.g., the Kalman filter is being changed as well.

You had enabled -Werror. When I built this project, I got a bunch of errors about unused parameters so I removed the offending parameter names to silence that warning. This meant modifying some code. Not sure why there were so many unused parameters but I didn't want to get rid of -Werror.

On this note, you had these compiler warnings as a usage require of the core library which is a bit heavy-handed. Just because you depend on Robotics::Robotics doesn't mean your target must use those compiler warnings. I strongly recommend everyone use those warnings, but some people will surely be annoyed that this unnecessary restriction was being applied to their targets so I used add_compile_options to apply that set of warnings to all targets in the repo. This way, users won't inherit the compiler warnings used internally for development.

[giacomo-b]

Hi Chris, I approved the CI pipeline, sorry about that. I will get back to you asap!

[ChrisThrasher]

@giacomo-b I think I fixed the CI pipeline but you'll have to approve it to run again. For the most part I just had to change the source directory passed to cmake. I did remove the examples.yml workflow since examples are already built by default so we don't need an extra workflow for building them; all the other workflows build the examples.

[ChrisThrasher]

@giacomo-b I just pushed some more pipeline fixes but something you'll have to decide is what to do about shadowed variable warnings with MSVC. /W4 includes the equivalent of -Wshadow (which we're not using). Do we go down to /W3 or fix the shadowed variables?

[giacomo-b]

Unrelated but 165 stars already?? That's amazing!

This was so unexptected! I don't have a lot of free time, but I will definitely keep working on it.

Examples can be disabled. If the compile time for the examples is unacceptable then you can disable them but keep building the tests so that you can verify your changes to the core library.

I think the best way to build all the examples at once would be to leave the choice to the user when running all. So all example projects could be included in all/CMakeLists.txt.

That way, cmake -S all -B build would build all the examples, but the conditional branching implied by environment variables definitions would be avoided.

This would also allow easier CI testing of all the examples, but would still leave the possibility of running just cmake -S examples/example -B build

This isn't required if you're using a version of CMake newer than 2.6. Any newer version will automatically fail to configure the project if that minimum version requirement isn't met.

Great to know.

If I recall correctly it was empty and unused. I could be mistaken though.

I'll double check the original repo asap.

You had enabled -Werror. When I built this project, I got a bunch of errors about unused parameters so I removed the offending parameter names to silence that warning. This meant modifying some code. Not sure why there were so many unused parameters but I didn't want to get rid of -Werror.

What OS are you using? I am asking this because I tried to build the examples with Linux and Windows, but didn't receive any errors.

On this note, you had these compiler warnings as a usage require of the core library which is a bit heavy-handed. Just because you depend on Robotics::Robotics doesn't mean your target must use those compiler warnings. I strongly recommend everyone use those warnings, but some people will surely be annoyed that this unnecessary restriction was being applied to their targets so I used add_compile_options to apply that set of warnings to all targets in the repo. This way, users won't inherit the compiler warnings used internally for development.

This is a very good point.

[giacomo-b]

something you'll have to decide is what to do about shadowed variable warnings with MSVC. /W4 includes the equivalent of -Wshadow (which we're not using). Do we go down to /W3 or fix the shadowed variables?

Hi Chris, are there several warnings at the moment?

[ChrisThrasher]

That way, cmake -S all -B build would build all the examples, but the conditional branching implied by environment variables definitions would be avoided.

Environment variables aren't involved. The options are specified when configuring the project like cmake -S . -B build -DBUILD_EXAMPLES=OFF. Conditional branching is still involved, it's just that users must know how to handle multiple environment directories to do that branching instead of simply changing the value of one CMake option. Both solution achieve the same thing, but using CMake options is much more idiomatic and easier for new developers to understand and work with.

Would you prefer it if examples were off by default?

This would also allow easier CI testing of all the examples, but would still leave the possibility of running just cmake -S examples/example -B build

Configuring the project just takes a few moments. After that you can run cmake --build build --target <your_example_here> to just build that one example and its dependencies. Because everything is already in one binary directory, there's a good chance that some of those dependencies are already compiled and don't need to be recompiled thus saving you time over having to create a brand new binary directory.

All these features are present in CMake without having to get clever about using multiple binary directories. There's little to gain by requiring that users juggle multiple binary directories. Most potential developers probably won't have a solid grasp of how multiple binary directories work and it will make it harder for them to contribute. I'm not even sure how you go about configuring such a multi-binary-dir project with an IDE.

What OS are you using? I am asking this because I tried to build the examples with Linux and Windows, but didn't receive any errors.

Clang 12 in macOS 10.15.

Hi Chris, are there several warnings at the moment?

The CI pipeline reports at least one /W4 warning from MSVC. Eigen's GitLab repo is offline so the CI is pipeline is failing before you can see some of the warnings being produced.

[codecov-commenter]

Codecov Report

Merging #5 (15e6aea) into master (469ce89) will decrease coverage by 48.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #5       +/-   ##
==========================================
- Coverage   50.00%   1.85%   -48.15%     
==========================================
  Files           2      12       +10     
  Lines          16     270      +254     
==========================================
- Hits            8       5        -3     
- Misses          8     265      +257     

Impacted Files	Coverage Δ
...ude/robotics/estimation/extended_kalman_filter.hpp	0.00% <ø> (ø)
include/robotics/linear_control/lqr.hpp	15.15% <0.00%> (-42.00%)	⬇️
include/robotics/common.hpp	0.00% <0.00%> (ø)
examples/infinite_horizon_lqr/main.cpp	0.00% <0.00%> (ø)
examples/extended_kalman_filter/main.cpp	0.00% <0.00%> (ø)
examples/pid/main.cpp	0.00% <0.00%> (ø)
include/robotics/system/nonlinear_system.hpp	0.00% <0.00%> (ø)
include/robotics/system/system_base.hpp	0.00% <0.00%> (ø)
examples/infinite_horizon_lqr_plot/main.cpp	0.00% <0.00%> (ø)
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 469ce89...15e6aea. Read the comment docs.

[ChrisThrasher]

@giacomo-b are you still interested in this PR?

[giacomo-b]

Hi @ChrisThrasher, sorry for the incredible delay. Yes, I am still interested, although I have been extremely busy with work.