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

Enhancement/use hpp for headers #3113

Merged
merged 60 commits into from
Nov 29, 2024

Conversation

TSNoble
Copy link
Contributor

@TSNoble TSNoble commented Nov 17, 2024

Description

First of all, apologies for the huge PR 😅

  • Partially addresses Use .hpp extension for headers #857 by replacing all .h files within moveit with .hpp files and, where possible, modifying imports to use .hpp
  • Adds a script to autogenerate .h files from .hpp files with a deprecation warning (see comments)
  • Updates MIGRATION.md

The main aim of this PR is to tackle a good portion of the low-hanging fruit so that subsequent (and less intrusive) PRs can be raised for specific types of imports.

I've left alone the following types of imports:

  • Imports to C libraries e.g. <math.h> (will raise a PR using C++ equivalents)
  • Imports to <ros/ros.h> (will raise a PR using rclcpp imports)
  • Imports to ROS1 msg packages (will raise a PR using ROS2 equivalents where possible)
  • Imports to <fmtlib/fmt.h> (I might raise a PR replacing these with <format> if there aren't downsides)
  • Imports to ROS2 dependencies where I'm unsure if .hpp files exist e.g. <tf2/etc.h>
  • Imports non-ROS libraries e.g. <kdl/etc.h> as I'm unsure if .hpp files exist (possibly worth tackling one dependency at a time)
  • Imports of the form <moveit_..._export.h> (I couldn't get these working, but would be happy to raise a PR with guidance)
    • It seems they come from the GENERATE_EXPORT_HEADER() CMake macro, which generates .h by default.
    • I wonder if we could get it to produce .hpp files?

The import style seems quite inconsistent, I've seen "", <>, and relative imports used for internal headers

  • It might be worth raising an issue to standardise this, if there isn't one already
  • It might be nice to enforce some kind of ordering (I'm biased towards built-in, external, internal, separated by newlines)

I'm a bit concerned about breaking downstream projects which import these headers.

I wonder if it'd be useful to add a script to the CI that generates a .h for every .hpp, which simply imports the .hpp and prints a deprecation warning?

Something like the following for all files "x.h":

#warning .h imports are deprecated. Consider using .hpp
#include <x.hpp>

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference (Doesn't affect user-facing functionality)
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference (existing tests passing should be a good signifier)
  • Include a screenshot if changing a GUI (not changing GUI per-se, just the imports used within)
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@TSNoble

This comment was marked as outdated.

@TSNoble

This comment was marked as outdated.

@TSNoble

This comment was marked as resolved.

@TSNoble

This comment was marked as resolved.

@TSNoble

This comment was marked as resolved.

@TSNoble

This comment was marked as resolved.

@rr-tom-noble

This comment was marked as resolved.

@rr-tom-noble

This comment was marked as resolved.

Copy link

mergify bot commented Nov 18, 2024

This pull request is in conflict. Could you fix it @TSNoble?

@TSNoble
Copy link
Contributor Author

TSNoble commented Nov 23, 2024

@sjahr @sea-bass

I believe this is ready for review now. Summary, since the thread is quite long:

  • I've replaced all .h files with .hpp (except ikfast and the autogenerated exports)
  • I've added #pragma once to headers which were missing them
  • I've added a script which autogenerates .h files from .hpp files (with BSD-3, Authoriship, and non-warning deprecation msg)
  • I've run this manually over the source tree and committed the .h files
  • The only files I wasn't able to generate .h files for were ones that aren't being installed (tests, default_capabilities, moveit_py)
  • Hopefully, the passing CI pipelines show that the imports work internally
  • Hopefully, the passing Tutorial pipelines show that the imports work externally
  • I'd think I'd like to include auto-gen of headers in the CI, but this can wait until we've seen the method works well, and with some tweaks / improvements
  • I still think we should upgrade these imports to warnings, and eventually remove the headers, however, I agree it'd be unwise to do so on short notice, without informing downstream repositories:
    • It might be useful to raise an issue with a hard deadline to turn these into warnings, with some kind of announcement?
    • And the same for removing support for c-style headers entirely
  • Failing humble build seems to be unrelated (resolved)

@rr-tom-noble
Copy link
Contributor

@sea-bass

Would you mind running the humble-ci job on this PR again when #3130 is resolved, just to make sure everything passes?

Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

CI is all green -- gave your script a look, and obviously not the headers :)

moveit/scripts/create_deprecated_headers.py Outdated Show resolved Hide resolved
moveit/scripts/create_deprecated_headers.py Outdated Show resolved Hide resolved
moveit/scripts/create_deprecated_headers.py Outdated Show resolved Hide resolved
moveit/scripts/create_deprecated_headers.py Outdated Show resolved Hide resolved
moveit/scripts/create_deprecated_headers.py Outdated Show resolved Hide resolved
@tylerjw
Copy link
Member

tylerjw commented Nov 26, 2024

You should merge this so my email inbox can rest again 😆

@rr-tom-noble
Copy link
Contributor

@sea-bass

I've applied your suggestions now (and tidied up the script a little more, and made the output a little nicer). I'd also be in favour of merging this soon so we can deal with any merge conflicts with other PRs quickly, but I also understand if you want to wait until the maintainer meeting tomorrow

Copy link
Contributor

@nbbrooks nbbrooks left a comment

Choose a reason for hiding this comment

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

I suggest adding a bit more context to the autogenerated message at the top of the h files. Your changelog notes are very descriptive, how about we use the:

/* This file was autogenerated by create_deprecated_headers.py */
becomes
/* All MoveIt 2 headers have been updated to use the .hpp extension. .h headers are now autogenerated with a deprecation warning, and may be removed in future releases. Please update your imports to use the .hpp headers. See https://github.com/moveit/moveit2/pull/3113 for details. */

@TSNoble
Copy link
Contributor Author

TSNoble commented Nov 28, 2024

I suggest adding a bit more context to the autogenerated message at the top of the h files. Your changelog notes are very descriptive, how about we use the:

/* This file was autogenerated by create_deprecated_headers.py */ becomes /* All MoveIt 2 headers have been updated to use the .hpp extension. .h headers are now autogenerated with a deprecation warning, and may be removed in future releases. Please update your imports to use the .hpp headers. See https://github.com/moveit/moveit2/pull/3113 for details. */

@nbbrooks

Thanks for the suggestion. I've modified the script (with a few tweaks to the formatting for readability & consistency with the BSD-3 comments), and have regenerated the header files.

/*********************************************************************
 * All MoveIt 2 headers have been updated to use the .hpp extension.
 *
 * .h headers are now autogenerated via create_deprecated_headers.py,
 * and will import the corresponding .hpp with a deprecation warning.
 *
 * imports via .h files may be removed in future releases, so please
 * modify your imports to use the corresponding .hpp imports.
 *
 * See https://github.com/moveit/moveit2/pull/3113 for extra details.
 *********************************************************************/

After this is merged, I'll likely look at tackling the outstanding headers listed in #857 as smaller, more targeted, (and more review-friendly) PRs 😄

Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Let's do this! Thanks again @TSNoble

@sea-bass sea-bass requested a review from sjahr November 29, 2024 02:24
Copy link
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

Thanks!

@sea-bass sea-bass added this pull request to the merge queue Nov 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 29, 2024
@rr-tom-noble
Copy link
Contributor

@sea-bass

Humble tests failed on the merge queue with what looks like #3130 😢

@sea-bass
Copy link
Contributor

Ah yes I need to merge the other PR first. One sec

@sea-bass sea-bass added this pull request to the merge queue Nov 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 29, 2024
@sea-bass
Copy link
Contributor

I'm gonna go ahead and merge this since the deprecation warning in #3139 just snuck in today and I want to keep things moving for now.

@sea-bass sea-bass merged commit 9766451 into moveit:main Nov 29, 2024
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants