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 FindXXX migration : gz-cmake #273

Merged
merged 2 commits into from
Jul 10, 2022
Merged

Conversation

methylDragon
Copy link
Contributor

@methylDragon methylDragon commented Jul 9, 2022

See: gazebo-tooling/release-tools#698

NOTE: THIS MUST BE REBASE MERGED!! Files were moved!

Description

All FindXXX.cmake files have been hard migrated to Gz.

Some downstream packages still use them via the gz_find_package() call, so an intercepting redirection of any Ign prefixed find calls to Gz has been implemented (with warning.)

image

Hard-tocks

  • <Package_name>_FIND_VERSION_MINOR CMake variables are being used (but populated automatically from the find_package() call, which might be the Gz or Ign version
  • Hard-tocked these since the file names changed (and the only way they'll be found is if the redirected find_package() call uses the Gz prefixed package name, which will set the Gz prefixed _FIND_VERSION_MINOR var)
  • Likewise with XXX_FIND_COMPONENTS
    • These are internal to the best of my knowledge, so the user shouldn't be using them

Tick-tocks

  • Downstream libs are using XXX_FOUND variables, I will ticktock those
  • Explicitly being set() with an Ign prefix, just in case they'll be used downstream

Notes

  • gz_import_target is being called to generate targets.

The call itself is made of several components:

  gz_import_target(IgnOGRE2
    TARGET_NAME IgnOGRE2::IgnOGRE2
    LIB_VAR OGRE2_LIBRARIES
    INCLUDE_VAR OGRE2_INCLUDE_DIRS)

That is,

  gz_import_target(<package_name>
    TARGET_NAME <TARGET>::<TARGET>
    LIB_VAR OGRE2_LIBRARIES
    INCLUDE_VAR OGRE2_INCLUDE_DIRS)
  • The package name is hard-tocked, and library aliases are installed for the imported target name.
  • In order to add alias targets, however, I needed to set the imported add_library() call to GLOBAL visibility.

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Jul 9, 2022
@methylDragon methylDragon marked this pull request as ready for review July 9, 2022 02:33
@methylDragon methylDragon requested review from chapulina and removed request for mxgrey and j-rivero July 9, 2022 02:36
@scpeters
Copy link
Member

scpeters commented Jul 9, 2022

testing sdformat against this branch in macOS and windows CI using the ci_matching_branch/ trick in gazebosim/sdformat#1072

Signed-off-by: methylDragon <[email protected]>
methylDragon added a commit that referenced this pull request Jul 9, 2022
methylDragon added a commit that referenced this pull request Jul 9, 2022
methylDragon added a commit that referenced this pull request Jul 9, 2022
methylDragon added a commit that referenced this pull request Jul 9, 2022
methylDragon added a commit that referenced this pull request Jul 9, 2022
methylDragon added a commit that referenced this pull request Jul 9, 2022
methylDragon added a commit that referenced this pull request Jul 9, 2022
methylDragon added a commit that referenced this pull request Jul 9, 2022
methylDragon added a commit that referenced this pull request Jul 9, 2022
methylDragon added a commit that referenced this pull request Jul 9, 2022
if(${PACKAGE_NAME_GZ}_FOUND)

message(DEPRECATION "Ign prefixed package name [${PACKAGE_NAME}] is deprecated! Automatically using the Gz prefix instead: [${PACKAGE_NAME_GZ}]")
set(PACKAGE_NAME ${PACKAGE_NAME_GZ})
Copy link
Member

Choose a reason for hiding this comment

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

this change in PACKAGE_NAME means there is no longer a matching ${PACKAGE_NAME}_pretty variable, since that is set just before this code block. This could be fixed by moving that code block to after this if logic

Copy link
Contributor Author

@methylDragon methylDragon Jul 9, 2022

Choose a reason for hiding this comment

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

Ah, nice catch! It seems despite my best efforts I got bamboozled by macro substitution 😓

Fixed: 98aadc5

Upon approval, I'll squash all the non file-migration commits into one so when we rebase merge we'll just have two commits!

methylDragon added a commit that referenced this pull request Jul 9, 2022
@methylDragon
Copy link
Contributor Author

methylDragon commented Jul 10, 2022

Gonna squash all the non moving commits 👀

For posterity:
image

@methylDragon methylDragon enabled auto-merge (rebase) July 10, 2022 05:04
@methylDragon methylDragon merged commit 5a30eb5 into main Jul 10, 2022
@methylDragon methylDragon deleted the ticktock_find_ign_cmake branch July 10, 2022 05:10
methylDragon added a commit that referenced this pull request Jul 10, 2022
Signed-off-by: methylDragon <[email protected]>
@chapulina chapulina added the ign to gz Renaming Ignition to Gazebo. label Jul 11, 2022
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.

3 participants