-
Notifications
You must be signed in to change notification settings - Fork 254
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
Improve standalone examples (and a bit of example CMake) #2387
base: gz-sim8
Are you sure you want to change the base?
Conversation
include(FetchContent) | ||
FetchContent_Declare( | ||
googletest | ||
URL https://github.com/google/googletest/archive/609281088cfefc76f9d0ce82e1ff6c30cc3591e5.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the cleanup!! I just have a few minor comments.
include(FetchContent) | ||
FetchContent_Declare( | ||
googletest | ||
URL https://github.com/google/googletest/archive/609281088cfefc76f9d0ce82e1ff6c30cc3591e5.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
8a98cd9
to
315397e
Compare
cd build | ||
ctest | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧑🌾 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 <[email protected]>
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. |
@Ryanf55 have you had a chance to look at the review feedback? |
Yea, I'll put this at the top of my todo list. |
Docs
Summary
Checklist
codecheck
passed (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-by
messages.