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

[ign ➡️ gz] Internal parsed variables #245

Merged
merged 4 commits into from
May 4, 2022
Merged

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented May 3, 2022

Summary

This PR updates just the variables parsed by cmake_parse_arguments to use gz instead of ign. Those variables are internal to each function and shouldn't affect the public API.

⬅️ This PR can be backported to ign-cmake2 later to reduce the diff between the branches.

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.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

@github-actions github-actions bot added the 🌱 garden Ignition Garden label May 3, 2022
@chapulina chapulina added the ign to gz Renaming Ignition to Gazebo. label May 3, 2022
@chapulina chapulina mentioned this pull request May 3, 2022
8 tasks
@methylDragon
Copy link
Contributor

methylDragon commented May 4, 2022

A test build of packages up to ignition-transport12 failed with this version of CMake3, throwing...

This error :(

image

I'm not sure what's up, but it might be something wonky with macro substitutions, or worse.., an erroneous macro-set variable that bubbles up into the parent scope that is now causing issues because of the gz change??

Copy link
Contributor

@methylDragon methylDragon left a comment

Choose a reason for hiding this comment

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

https://github.com/ignitionrobotics/ign-cmake/pull/245/files#diff-955d1f97e2360702839ba9277cc71b9797e65d417d5cb88f383fec0e53ad5124R1000
Found it! Line 1000:
_ign_handle_cxx_standard(ign_create_core_library -> _ign_handle_cxx_standard(gz_create_core_library

(:

_ign_handle_cxx_standard is a macro that looks for a prefix, not a macro/function name, so we just need to pass in that prefix so it can resolve the appropriate arg!

It builds! (The stderr from math is just the overlay/override check, no worries on that.)
image

Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

Found it! Line 1000:

Thanks! Well spotted! Committed in e897659

Copy link
Contributor

@methylDragon methylDragon left a comment

Choose a reason for hiding this comment

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

👍 pending green CI

@chapulina chapulina merged commit 2fbea3b into main May 4, 2022
@chapulina chapulina deleted the chapulina/3/parse_vars branch May 4, 2022 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden ign to gz Renaming Ignition to Gazebo.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants