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.

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

Build examples using native CMake #301

Merged
merged 18 commits into from
May 19, 2023
Merged

Conversation

mjcarroll
Copy link
Contributor

@mjcarroll mjcarroll commented Aug 10, 2022

🦟 Bug fix

Fixes (for the most part) Windows examples_build and deduplicates some build steps

Summary

When testing on the newest conda/Visual Studio 2022/Windows 11 configuration I ran into some issues with the "FAKE_INSTALL" step that we were doing in many packages, which was seen to be invalid. I was able to reproduce on Linux by trying to use the "ninja" generator for CMake.

After some investigation, I found that FAKE_INSTALL was actually causing the packages to be built twice, which adds significant overhead to the build.

This is an alternative mechanism for building examples as part of the test suite. Any package that wants to build examples will have to be updated to take advantage of this macro.

This is a pure CMake implementation that doesn't rely on calling out to other scripts/executables.

Downstream PRs:

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.

Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Aug 10, 2022
@osrf-triage osrf-triage added this to Inbox in Core development Aug 10, 2022
@chapulina chapulina added bug Something isn't working Windows Windows support labels Aug 10, 2022
@chapulina chapulina moved this from Inbox to In review in Core development Aug 10, 2022
Signed-off-by: Michael Carroll <michael@openrobotics.org>
…bosim/gz-cmake into mjcarroll/examples_from_cmake
cmake/GzUtils.cmake Outdated Show resolved Hide resolved
Signed-off-by: Michael Carroll <michael@openrobotics.org>

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
cmake/GzUtils.cmake Outdated Show resolved Hide resolved
@mjcarroll mjcarroll requested review from chapulina and azeey and removed request for chapulina September 27, 2022 16:28
@mjcarroll mjcarroll self-assigned this Oct 26, 2022
# Optional. List of components built by this project that are used in the examples.
# For example "cli" for gz-utils
#
# DEPENDENCIES:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me see if I understand this right: in DEPENDENCIES you can specify gz- libraries that are not being used by the build system of the project but are required to make the examples to build/run? Is this correct? If so, bellow there is a comment:

we are passing in the resolved dependency directories via -Ddep_DIR=${dep_DIR}

How does this work if a dependency is not in the current buildsystem?

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 a slight rework to this I can push shortly that drops this gymnastics a bit.

Note that this is only necessary for gazebo packages, as they may be built in the same workspace, so this special forwarding happens.

Currently, examples aren't allowed to depend on Gazebo packages things that the containing package doesn't. E.g. gz-gui examples cannot have a dependency on gz-gazebo, as this will introduce circular dependencies.

If an example depends on a non-Gazebo package, then typical find_package infrastructure should work.

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.

I finally got around to reviewing this. Sorry for the long delay.

My main issue is that we're adding all the resolved dependency directories when it doesn't seem necessary. Maybe something is misconfigured in my local system and there's a real need to do this.

cmake/GzUtils.cmake Show resolved Hide resolved
cmake/GzUtils.cmake Outdated Show resolved Hide resolved
cmake/GzUtils.cmake Outdated Show resolved Hide resolved
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
@mjcarroll mjcarroll merged commit d0fb580 into gz-cmake3 May 19, 2023
6 checks passed
Core development automation moved this from In review to Done May 19, 2023
@mjcarroll mjcarroll deleted the mjcarroll/examples_from_cmake branch May 19, 2023 14:03
@azeey azeey moved this from Done to Highlights in Core development May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🌱 garden Ignition Garden Windows Windows support
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants