Project option: REPLACE_IGNITION_INCLUDE_PATH

[scpeters]

🎉 New feature

Helps with gazebosim/sdformat#181

Summary

Currently all cmake projects that use IgnConfigureProject are required to use include paths that start with ignition/${IGN_DESIGNATION} (i.e. ignition/math, ignition/transport). We would like to use ign-cmake2 with libsdformat, but its cmake project name and include paths don't follow this pattern. This pull request adds an option to IgnConfigureProject to allow more flexibility:

• INCLUDE_DIR: option to specify the project include paths, as an alternative to ignition/${IGN_DESIGNATION}

Test it

Look at the examples/no_ignition_prefix folder and build with cmake .. -DBUILDSYSTEM_TESTING=ON && make -j

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

[koonpeng]

I'm trying to migrate sdformat to ign-cmake, I found that there are a few more things which are not compatible

1. headers are still installed to ignition/ prefix, it also enforces the ${IGN_DESIGNATION}${PROJECT_VERSION_MAJOR} pattern, while sdformat uses a slightly different pattern sdformat-${SDF_VERSION} (there is a hypen). https://github.com/scpeters/ign-cmake/blob/787cebd91c0ba16562ea6d545b86d5a87d90775c/cmake/IgnPackaging.cmake#L106.
2. The master header is named ${IGN_DESIGNATION}.hh, where IGN_DESIGNATION is derived from the project name, which in sdformat's case is sdformat, but the master header of sdformat is sdf.hh. https://github.com/scpeters/ign-cmake/blob/787cebd91c0ba16562ea6d545b86d5a87d90775c/cmake/IgnUtils.cmake#L724.
3. Headers are installed to ${IGN_INCLUDE_INSTALL_DIR_FULL}/${PROJECT_INCLUDE_DIR}, and the master header is installed one directory up. But sdformat expects the master header to be in the same directory as the other headers.
4. The config header must be named config.hh, but sdf's config header is named sdf_config.h.

I haven't gotten around to migrating tests and docs yet, I will report more issues as I find them.

[scpeters]

I'm trying to migrate sdformat to ign-cmake, I found that there are a few more things which are not compatible

1. headers are still installed to ignition/ prefix, it also enforces the ${IGN_DESIGNATION}${PROJECT_VERSION_MAJOR} pattern, while sdformat uses a slightly different pattern sdformat-${SDF_VERSION} (there is a hypen). https://github.com/scpeters/ign-cmake/blob/787cebd91c0ba16562ea6d545b86d5a87d90775c/cmake/IgnPackaging.cmake#L106.
2. The master header is named ${IGN_DESIGNATION}.hh, where IGN_DESIGNATION is derived from the project name, which in sdformat's case is sdformat, but the master header of sdformat is sdf.hh. https://github.com/scpeters/ign-cmake/blob/787cebd91c0ba16562ea6d545b86d5a87d90775c/cmake/IgnUtils.cmake#L724.
3. Headers are installed to ${IGN_INCLUDE_INSTALL_DIR_FULL}/${PROJECT_INCLUDE_DIR}, and the master header is installed one directory up. But sdformat expects the master header to be in the same directory as the other headers.
4. The config header must be named config.hh, but sdf's config header is named sdf_config.h.

I haven't gotten around to migrating tests and docs yet, I will report more issues as I find them.

I'd like to see if reviewers are ok with this PR as is in its partially useful state or if they want to try to address all of this at. once

cc @adlarkin

[adlarkin]

Thanks @scpeters and @koonpeng for the things you've discovered so far! Regarding how to proceed with addressing the issues mentioned above, I think we can take the following approach:

1. One PR that gives users options to specify the include directories that are searched for and installed
2. One PR that provides flexibility in specifying the cmake prefix project name
3. One PR that addresses the master header issues (name of the master header and where it's installed)
4. One PR that allows configuration of the name of the config header

Having separate, smaller PRs that focus on solving one issue in particular will make the review process easier, and also give us a better way to track changes being made. If there are no objections to the approach I've suggested, then this PR will need to be broken into 2 PRs because right now, this PR solves part of point 1 (searched include directories) and point 2 above. If help is needed in creating more PRs that solve the remaining issues found by @koonpeng, let me know, and I can help open a few.

[scpeters]

2\. One PR that provides flexibility in specifying the cmake prefix project name

I've split that feature out from this PR into #191

I can rebase this branch after that is merged, if that is not too presumptuous

[adlarkin]

I can rebase this branch after that is merged, if that is not too presumptuous

I'm okay with you waiting for #191 and then rebasing. Would you mind updating the PR of this title since you put the NO_IGNITION_PREFIX portion in #191?

[scpeters]

I can rebase this branch after that is merged, if that is not too presumptuous

I'm okay with you waiting for #191 and then rebasing. Would you mind updating the PR of this title since you put the NO_IGNITION_PREFIX portion in #191?

done

[scpeters]

I can rebase this branch after that is merged, if that is not too presumptuous

I'm okay with you waiting for #191 and then rebasing. Would you mind updating the PR of this title since you put the NO_IGNITION_PREFIX portion in #191?

done

just rebased

[scpeters]

Thanks @scpeters and @koonpeng for the things you've discovered so far! Regarding how to proceed with addressing the issues mentioned above, I think we can take the following approach:

1. One PR that gives users options to specify the include directories that are searched for and installed

2. One PR that provides flexibility in specifying the cmake prefix project name

3. One PR that addresses the master header issues (name of the master header and where it's installed)

4. One PR that allows configuration of the name of the config header

Having separate, smaller PRs that focus on solving one issue in particular will make the review process easier, and also give us a better way to track changes being made. If there are no objections to the approach I've suggested, then this PR will need to be broken into 2 PRs because right now, this PR solves part of point 1 (searched include directories) and point 2 above. If help is needed in creating more PRs that solve the remaining issues found by @koonpeng, let me know, and I can help open a few.

I've added a checklist in gazebosim/sdformat#181 (comment) with an additional entry for the prefix to the macros in Export.hh

[adlarkin]

LGTM! Just a few nits about grammar before merging

[koonpeng]

diff --git a/cmake/ign_auto_headers.hh.in b/cmake/ign_auto_headers.hh.in
index 42fc964..a078c63 100644
--- a/cmake/ign_auto_headers.hh.in
+++ b/cmake/ign_auto_headers.hh.in
@@ -19,5 +19,5 @@
 // This file is automatically generated by CMake. Changes should instead be
 // made to cmake/ign_auto_headers.hh.in in ignition-cmake
 
-#include <ignition/@IGN_DESIGNATION@/config.hh>
+#include <@PROJECT_INCLUDE_DIR@/config.hh>
 ${ign_headers}

Fixes config.hh still referencing ignition/ prefix.

[j-rivero]

Sorry to jump in a bit late.

* `INCLUDE_DIR`: option to specify the project include paths, as an alternative to `ignition/${IGN_DESIGNATION}`

INCLUDE_DIR in CMake drives my mind immediately to INCLUDE_DIRECTORIES even if we have it under the context of ign_configure_project. Maybe we can be a bit more verbose to help code reading with something like: CUSTOM_INCLUDE_DIR_PATH or REPLACE_IGNITION_INCLUDE_PATH or other more explicit name about what we are doing with the parameter.

[adlarkin]

@scpeters would you mind addressing the comments from @j-rivero and @koonpeng above? For the config.hh fix, it would be nice to update the example to show that this file can be found without the ignition prefix.

[scpeters]

Fixes config.hh still referencing ignition/ prefix.

nice catch! fixed in 7c12783

[scpeters]

Sorry to jump in a bit late.

* `INCLUDE_DIR`: option to specify the project include paths, as an alternative to `ignition/${IGN_DESIGNATION}`

INCLUDE_DIR in CMake drives my mind immediately to INCLUDE_DIRECTORIES even if we have it under the context of ign_configure_project. Maybe we can be a bit more verbose to help code reading with something like: CUSTOM_INCLUDE_DIR_PATH or REPLACE_IGNITION_INCLUDE_PATH or other more explicit name about what we are doing with the parameter.

I'll go with REPLACE_IGNITION_INCLUDE_PATH as the ign_configure_project parameter, but I think I'll keep PROJECT_INCLUDE_DIR as the internal variable name

[scpeters]

Sorry to jump in a bit late.

* `INCLUDE_DIR`: option to specify the project include paths, as an alternative to `ignition/${IGN_DESIGNATION}`

INCLUDE_DIR in CMake drives my mind immediately to INCLUDE_DIRECTORIES even if we have it under the context of ign_configure_project. Maybe we can be a bit more verbose to help code reading with something like: CUSTOM_INCLUDE_DIR_PATH or REPLACE_IGNITION_INCLUDE_PATH or other more explicit name about what we are doing with the parameter.

I'll go with REPLACE_IGNITION_INCLUDE_PATH as the ign_configure_project parameter, but I think I'll keep PROJECT_INCLUDE_DIR as the internal variable name

changed in 6c2ffe5. It's easy to change the name now, so please speak up now if you have a better idea; it won't hurt my feelings

[j-rivero]

changed in 6c2ffe5. It's easy to change the name now, so please speak up now if you have a better idea; it won't hurt my feelings

The change looks good to me, thanks! Since the internal variable was there before I think is fine to leave as it is.

[j-rivero]

ready to go. It would be great if we can write a Changelog entry in this PR but can done at releasing time.

[scpeters]

@scpeters would you mind addressing the comments from @j-rivero and @koonpeng above? For the config.hh fix, it would be nice to update the example to show that this file can be found without the ignition prefix.

6787937: I've added the ign_install_all_headers to the example and include the generated headers in AlmostEmpty.cc to confirm their location

[adlarkin]

Newest changes LGTM, thanks for iterating @scpeters!

It would be great if we can write a Changelog entry in this PR but can done at releasing time.

+1

[osrf-triage]

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

https://community.gazebosim.org/t/new-ignition-releases-2022-01-10/1228/1