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

Fix up documentation build for rmw when using rosdoc2 #313

Merged
merged 7 commits into from
Sep 14, 2021

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Sep 7, 2021

Precisely what the title says. This patch:

  • Substitutes trailing [] tokens for leading * tokens for pointer argument types.
  • Normalizes type naming to preclude duplication warnings.
  • Handles RCUTILS_DEPRECATED_WITH_MSG() macros
  • Handles RMW_PUBLIC_TYPE macros.
  • De-duplicates some declarations.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Sep 7, 2021

Full CI!

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Argh, build issues in rmw_connextdds_cpp...

@hidmic
Copy link
Contributor Author

hidmic commented Sep 8, 2021

Alright, it wasn't just rmw_connextdds_cpp.

Re-running full CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

De-duplicates some declarations.

Is there a way to make doxygen ignore that instead of renaming *_t to *_s.
I don't mind much, but it seems a bit unnecessary to break API that way.
Though, I think we're never using struct *_t in code and always *_t directly.

@hidmic
Copy link
Contributor Author

hidmic commented Sep 8, 2021

Is there a way to make doxygen ignore that instead of renaming *_t to *_s.

I followed ros2/rcutils#333 precedent here, but perhaps \cond based suppression of typedefs is possible e.g.:

/// \cond flag
typedef
/// \endcond
struct type_name_t {

}
/// \cond flag
type_name_t
/// \endcond
;

Though I do find duplicate type definitions awkward albeit legal.

Though, I think we're never using struct *_t in code and always *_t directly.

Well, I've found some cases. Specially in rcl. Definitely not a policy. More like an easter egg I'd say.

@ivanpauno
Copy link
Member

I followed ros2/rcutils#333 precedent here, but perhaps \cond based suppression of typedefs is possible e.g.:

That seems to be enough precedent, we will have to bump the major number next release.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic requested a review from clalancette September 13, 2021 18:40
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This all looks really good to me with green CI.

Thanks for iterating!

@@ -202,7 +202,7 @@ rmw_topic_endpoint_info_set_endpoint_type(
rmw_ret_t
rmw_topic_endpoint_info_set_gid(
rmw_topic_endpoint_info_t * topic_endpoint_info,
const uint8_t gid[],
const uint8_t * gid,
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? (just curious)

Copy link
Contributor Author

@hidmic hidmic Sep 14, 2021

Choose a reason for hiding this comment

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

WARNING: doxygenfunction: Unable to resolve function "rmw_topic_endpoint_info_set_gid" with arguments (rmw_topic_endpoint_info_t*, const uint8_t, size_t) in doxygen xml output for project "rmw Doxygen Project" from directory: ...
Potential matches:

  • rmw_ret_t rmw_topic_endpoint_info_set_gid(rmw_topic_endpoint_info_t *topic_endpoint_info, const uint8_t gid[], size_t size)

It appears exhale (i.e. the thing that turns Doxygen XML into Sphinx compliant ReST) fails to pick up a trailing [] as a type modifier. I haven't dug into it deep enough to find out why.

@@ -30,7 +30,7 @@ extern "C"
#include "rmw/visibility_control.h"

/// Define publisher/subscription events
typedef enum rmw_event_type_t
typedef enum rmw_event_type_e
Copy link
Member

Choose a reason for hiding this comment

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

Will this break rmw implementations?

Copy link
Contributor

Choose a reason for hiding this comment

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

Any RMW implementation that was using a "bare" rmw_event_type_t before should be fine; we are still defining that as part of the typedef. If an RMW implementation was using enum rmw_event_type_t, then it would now be broken.

I don't think we have any users of that in the core, but I will request a full CI run before we merge this. There might be third-party ones that use enum rmw_event_type_t, so I think a release note is in order for this change.

@hidmic
Copy link
Contributor Author

hidmic commented Sep 14, 2021

Re-running full CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@hidmic
Copy link
Contributor Author

hidmic commented Sep 14, 2021

Alright, all green !

@hidmic hidmic merged commit 05f9735 into master Sep 14, 2021
@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-9-16/22372/1

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

Successfully merging this pull request may close these issues.

5 participants