Improve standalone examples (and a bit of example CMake) #2387
Conversation
| include(FetchContent) | ||
| FetchContent_Declare( | ||
| googletest | ||
| URL https://github.com/google/googletest/archive/609281088cfefc76f9d0ce82e1ff6c30cc3591e5.zip |
There was a problem hiding this comment.
I have no clue why google test still recommends fetchcontent. Google doesn't really use CMake internally. People should use the gtest supplied with their OS unless they have a good reason not to.
There was a problem hiding this comment.
Okay, but then we should probably update the README and to say that users should first install gtest.
There was a problem hiding this comment.
I'm afraid this breaks CI:
- https://build.osrfoundation.org/job/gz_sim-ci-pr_any-homebrew-amd64/414/testReport/junit/(root)/Plugins_ExamplesBuild/Build_standalone_gtest_setup/
- https://build.osrfoundation.org/job/gz_sim-ci-pr_any-jammy-amd64/584/testReport/junit/(root)/Plugins_ExamplesBuild/Build_standalone_gtest_setup/
Probably because we don't install gtest in our CI machines. @j-rivero any thoughts on how to proceed? Is it better to revert this?
There was a problem hiding this comment.
Alternatively, we could add gtest as a build dependency to the package.xml, assuming dependencies are managed through rosdep.
There was a problem hiding this comment.
They are not handled via package.xml. That is a recent addition (#2337) and none of our CI currently uses it yet.
There was a problem hiding this comment.
Probably because we don't install gtest in our CI machines
This is correct. We actually vendor a specific gtest version in each of our packages.
| include(FetchContent) | ||
| FetchContent_Declare( | ||
| googletest | ||
| URL https://github.com/google/googletest/archive/609281088cfefc76f9d0ce82e1ff6c30cc3591e5.zip |
There was a problem hiding this comment.
Okay, but then we should probably update the README and to say that users should first install gtest.
|
Can you fix the conflict on examples/standalone/gtest_setup/CMakeLists.txt? |
Ryanf55
force-pushed
the
improve-tutorial-docs
branch
from
8a98cd9
to
315397e
Compare
| cd build | ||
| ctest | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🧑🌾 I think CI is complaining on this trailing newline, can you remove it?
|
🧑🌾 @Ryanf55 I was taking a look to this to fix our CI, it's pretty close to be mergeable, let us know if you need help with the remaining conflicts 🚀 |
* Simpler and standard build workflow * Utilize CTest with the recommended workflow of optional tests * Skip using make directly so these become more platform portable * Skip having to manually create the build directory * Do all operations from the source directory when possible to reduce the number of commands Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>
Ryanf55
force-pushed
the
improve-tutorial-docs
branch
from
315397e
to
2cca64e
Compare
|
Looks like |
I think it's best to revert your changes. I don't think we're prepared to change our CI for this. |
Docs
Summary
Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-bymessages.