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.

Sign up for GitHub

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

Jump to bottom

Improve standalone examples (and a bit of example CMake) #2387

Open
wants to merge 1 commit into
base: gz-sim8
Choose a base branch
from
Open

Improve standalone examples (and a bit of example CMake) #2387

wants to merge 1 commit into from

Conversation

Ryanf55
Copy link
Contributor

@Ryanf55 Ryanf55 commented Apr 26, 2024 •
edited
Loading

Docs

Summary

  • 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
  • Surround commands in bash tags to get syntax highlighting

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

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.

@Ryanf55 Ryanf55 requested a review from mjcarroll as a code owner April 26, 2024 20:50
@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Apr 26, 2024
Ryanf55
Ryanf55 commented Apr 26, 2024
View reviewed changes
examples/standalone/gtest_setup/CMakeLists.txt Outdated
include(FetchContent)
FetchContent_Declare(
googletest
URL https://github.com/google/googletest/archive/609281088cfefc76f9d0ce82e1ff6c30cc3591e5.zip
Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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:

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

azeey
azeey reviewed May 6, 2024
View reviewed changes
Copy link
Contributor

@azeey azeey left a 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.

examples/standalone/acoustic_comms_demo/README.md Show resolved Hide resolved
examples/standalone/gtest_setup/CMakeLists.txt Show resolved Hide resolved
examples/standalone/gtest_setup/CMakeLists.txt Outdated
include(FetchContent)
FetchContent_Declare(
googletest
URL https://github.com/google/googletest/archive/609281088cfefc76f9d0ce82e1ff6c30cc3591e5.zip
Copy link
Contributor

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.

@azeey azeey mentioned this pull request May 7, 2024
Merged
8 tasks
@azeey
Copy link
Contributor

azeey commented May 7, 2024

Can you fix the conflict on examples/standalone/gtest_setup/CMakeLists.txt?

Blast545
Blast545 reviewed May 13, 2024
View reviewed changes
examples/standalone/gtest_setup/README.md
cd build
ctest
```

Copy link
Contributor

@Blast545 Blast545 May 13, 2024 •
edited
Loading

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?

@Blast545
Copy link
Contributor

🧑‍🌾 @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 🚀

@Ryanf55
Improve examples
2cca64e 
* 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]>
@Ryanf55
Copy link
Contributor Author

Ryanf55 commented May 14, 2024

Looks like gtest can't be found in CI. Should I be making changes to the local CI, or is this managed elsewhere?
https://build.osrfoundation.org/job/gz_sim-ci-pr_any-jammy-amd64/610/cmake/new/

@azeey
Copy link
Contributor

azeey commented May 20, 2024

Looks like gtest can't be found in CI. Should I be making changes to the local CI, or is this managed elsewhere? https://build.osrfoundation.org/job/gz_sim-ci-pr_any-jammy-amd64/610/cmake/new/

I think it's best to revert your changes. I don't think we're prepared to change our CI for this.

@azeey
Copy link
Contributor

azeey commented Jul 22, 2024

@Ryanf55 have you had a chance to look at the review feedback?

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Jul 22, 2024

Yea, I'll put this at the top of my todo list.

@Ryanf55 Ryanf55 closed this Jul 22, 2024
@Ryanf55 Ryanf55 reopened this Jul 22, 2024
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024
@azeey azeey removed the beta Targeting beta release of upcoming collection label Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Blast545 Blast545 Blast545 left review comments

@azeey azeey azeey left review comments

@mjcarroll mjcarroll Awaiting requested review from mjcarroll mjcarroll is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
🎵 harmonic Gazebo Harmonic
Projects
Core development
Status: In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Ryanf55 @azeey @Blast545 @mjcarroll