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

Replace custom cmake code with ign-cmake2 #780

Merged
merged 29 commits into from
Dec 22, 2021

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Dec 9, 2021

🎉 New feature

Closes #181, borrowing from #747 and targeting the sdf10 branch. It should be possible to backport these changes to sdf9 if an appropriate find-module is provided for TinyXML.

Summary

This replaces most of the custom cmake code in libsdformat with the functionality provided by ignition-cmake2. The root CMakeLists.txt is much shorter now and most of the cmake folder has been deleted. This is made possible by the NO_IGNITION_PREFIX and REPLACE_IGNITION_INCLUDE_PATH parameters added to ign_configure_project in gazebosim/gz-cmake#190 and gazebosim/gz-cmake#191. Some other details are listed below:

  • Use ign-cmake find modules for Tinyxml2 and URDFDOM (added in FindIgnURDFDOM cmake module gz-cmake#193), which provide cmake targets
  • the sdf_config.h.in template is renamed to config.hh.in to match the ign-cmake convention, and an sdf_config.h static header is added to preserve API
  • the autogenerated header is now sdformat.hh instead of sdf/sdf.hh, so a static header sdf/sdf.hh is added to preserve API
  • the SDFORMAT_* visibility macros in system_util.h are now based on the IGNITION_SDFORMAT_* macros from the auto-generated sdf/Export.hh
  • the LEGACY_PROJECT_PREFIX option from Add LEGACY_PROJECT_PREFIX option gz-cmake#199 is used to ensure the legacy cmake variables from the cmake config file retain the SDFormat_ prefix

This is currently a draft pull request since There are some differences between the output of this branch and the currently installed versions:

  • headers are installed to {prefix}/include/ignition/sdformat10 rather than {prefix}/include/sdformat-10.6. EDIT: I think this is ok because the header folder changes with each minor release anyways. As long as the cmake config and pkgconfig files point to the correct place, it should be fine.
  • the name / description in the .pc file have changed from SDF / Robot Modeling Language (SDF) to Ignition sdformat / A set of sdformat classes for robot applications
  • the SDFORMAT_RUN_VALGRIND_TESTS cmake option is removed since ign_build_tests does not support valgrind

There are is also two one remaining custom cmake function in the cmake folder:

  • find_python_module: this function is used to find the psutil python module that is used by an integration test.

Test it

It should continue to work normally. Testing in a colcon workspace with other packages would be very helpful.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

@github-actions github-actions bot added the 🔮 dome Ignition Dome label Dec 9, 2021
@scpeters scpeters mentioned this pull request Dec 9, 2021
8 tasks
Signed-off-by: Steve Peters <[email protected]>
@scpeters
Copy link
Member Author

scpeters commented Dec 9, 2021

testing gazebo-tooling/action-gz-ci#46

scpeters added a commit to gazebo-tooling/release-tools that referenced this pull request Dec 9, 2021
@scpeters
Copy link
Member Author

scpeters commented Dec 9, 2021

the ABI-checker needs gazebo-tooling/release-tools#589

@scpeters scpeters mentioned this pull request Dec 9, 2021
Signed-off-by: Steve Peters <[email protected]>
This may represent a breakage for downstream users
of the SDFormat_* cmake variables.

Signed-off-by: Steve Peters <[email protected]>
@scpeters
Copy link
Member Author

scpeters commented Dec 9, 2021

@osrf-jenkins run tests please

Signed-off-by: Steve Peters <[email protected]>
@scpeters
Copy link
Member Author

the warnings in the Ubuntu build should be fixed by gazebo-tooling/release-tools#590

@scpeters scpeters marked this pull request as ready for review December 10, 2021 01:08
j-rivero pushed a commit to gazebo-tooling/release-tools that referenced this pull request Dec 10, 2021
* sdformat-abichecker: set GZDEV_PROJECT_NAME

Needed by gazebosim/sdformat#780

Signed-off-by: Steve Peters <[email protected]>

* detect_ci_matching_branch: use format()

This is compatible with more verisons of python3.

Signed-off-by: Steve Peters <[email protected]>
@scpeters
Copy link
Member Author

since it looks like no more changes are needed from ign-cmake, I'll open a PR for a stable 2.10.0 release

gazebosim/gz-cmake#201

scpeters added a commit to gazebo-release/sdformat10-release that referenced this pull request Dec 22, 2021
@scpeters
Copy link
Member Author

preparing sdformat10-release debian metadata:

@scpeters
Copy link
Member Author

since it looks like no more changes are needed from ign-cmake, I'll open a PR for a stable 2.10.0 release

ignitionrobotics/ign-cmake#201

the release is done and I just deleted the matching branch on gzdev that was enabling prerelease repositories, so it should build from stable packages now

@scpeters
Copy link
Member Author

@osrf-jenkins run tests please

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I just took a really quick look and didn't see anything fishy. I think we can merge this and make a pre-release to see if anything breaks downstream.

The only thing to keep in mind is that we should make the last 10.X release in the coming days for Dome's EOL. So in the worst case scenario, if we identify issues after the pre-release, we may need to revert this PR soon.


echo -n "Upload code API(Y/n)? "
read ans

if [ "$ans" = "y" ] || [ "$ans" = "Y" ]; then
s3cmd sync @CMAKE_BINARY_DIR@/doxygen/html/* s3://osrf-distributions/sdformat/api/@SDF_VERSION_FULL@/ -v
s3cmd sync @CMAKE_BINARY_DIR@/doxygen/html/* s3://osrf-distributions/sdformat/api/@PROJECT_VERSION_FULL@/ -v
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking, did anyone try uploading docs from this branch?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, sorry I haven't tried that yet

CMakeLists.txt Show resolved Hide resolved
@scpeters scpeters merged commit 92d20d2 into sdf10 Dec 22, 2021
@scpeters scpeters deleted the ci_matching_branch/use-ign-cmake-10 branch December 22, 2021 22:56
@koonpeng
Copy link

@ahcorde @scpeters how do i go about forward porting this to main? I wans't able to find any docs on this.

@scpeters
Copy link
Member Author

@ahcorde @scpeters how do i go about forward porting this to main? I wans't able to find any docs on this.

we forward port by merging forward and resolving conflicts. I'm testing a forward port to sdf11

@chapulina
Copy link
Contributor

how do i go about forward porting this to main? I wans't able to find any docs on this.

It's documented here: https://ignitionrobotics.org/docs/all/contributing#process

@scpeters scpeters mentioned this pull request Dec 29, 2021
@scpeters
Copy link
Member Author

@ahcorde @scpeters how do i go about forward porting this to main? I wans't able to find any docs on this.

we forward port by merging forward and resolving conflicts. I'm testing a forward port to sdf11

merging #789 forward first since its test needs a few changes with SDFormat 1.8: #807

@scpeters
Copy link
Member Author

@ahcorde @scpeters how do i go about forward porting this to main? I wans't able to find any docs on this.

we forward port by merging forward and resolving conflicts. I'm testing a forward port to sdf11

merging #789 forward first since its test needs a few changes with SDFormat 1.8: #807

merging the rest forward to sdf11 in #808

@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-25-fortress-edifice-citadel/1343/1

@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants