Skip to content

Allow for implicitly convertable loggers as well#2922

Merged
jmachowinski merged 4 commits intoros2:rollingfrom
nobleo:feature/multiple-logger-implementations
Aug 13, 2025
Merged

Allow for implicitly convertable loggers as well#2922
jmachowinski merged 4 commits intoros2:rollingfrom
nobleo:feature/multiple-logger-implementations

Conversation

@Timple
Copy link
Copy Markdown
Contributor

@Timple Timple commented Jul 31, 2025 •
edited
Loading

Description

This way I can inherit from rclpp::Logger and still pass it to the logger macros.

Is this user-facing behavior change?

Backwards compatible

Did you use Generative AI?

No

Additional Information

If accepted I would like to have this backported to active distros as well.

@Timple Timple force-pushed the feature/multiple-logger-implementations branch from 92e99c4 to 99e11a3 Compare July 31, 2025 08:33
fujitatomoya
fujitatomoya approved these changes Aug 1, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm with green CI.

The new check enables more flexible and extensible code, allowing for future enhancements and custom logger types. It also prevents compile errors when a compatible logger type (not strictly rclcpp::Logger) is used.

@fujitatomoya fujitatomoya requested a review from ahcorde August 1, 2025 04:20
@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Aug 1, 2025

Pulls: #2922
Gist: https://gist.githubusercontent.com/fujitatomoya/38955bee08885a052238f60933001082/raw/fccdf9cc7a4da7a40ad9a982049234aafe57abf9/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16659

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@Timple
Copy link
Copy Markdown
Contributor Author

Timple commented Aug 5, 2025

Is CI flaky or is this something I introduced?

@christophebedard
Copy link
Copy Markdown
Member

christophebedard commented Aug 5, 2025

@ros-pull-request-builder retest this please

Timple added 4 commits August 11, 2025 08:35
@Timple
Allow for implicitly convertable loggers as well
ab45483 
Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>
@Timple
Test implicitly convertable logger
e4116f4 
Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>
@Timple
This can be simplified now
3a4ef5c 
Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>
@Timple
fixup! This can be simplified now
f35170d 
Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>
@Timple Timple force-pushed the feature/multiple-logger-implementations branch from 1890f16 to f35170d Compare August 11, 2025 06:35
@Timple
Copy link
Copy Markdown
Contributor Author

Timple commented Aug 11, 2025

I rebased, seemed to have resolved the build error.

@christophebedard
Copy link
Copy Markdown
Member

christophebedard commented Aug 11, 2025 •
edited
Loading

Pulls: #2922
Gist: https://gist.githubusercontent.com/christophebedard/a19fa4a48c58ecf2a1461b62c9fcd87c/raw/fccdf9cc7a4da7a40ad9a982049234aafe57abf9/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16693

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@jmachowinski jmachowinski merged commit e615c7c into ros2:rolling Aug 13, 2025
3 checks passed
@jmachowinski
Copy link
Copy Markdown
Collaborator

jmachowinski commented Aug 13, 2025

https://github.com/Mergifyio backport kilted jazzy humble

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Aug 13, 2025 •
edited
Loading

backport kilted jazzy humble

✅ Backports have been created

Details

mergify bot pushed a commit that referenced this pull request Aug 13, 2025
@Timple @mergify
Allow for implicitly convertable loggers as well (#2922)
1faea29 
* Allow for implicitly convertable loggers as well

Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>

* Test implicitly convertable logger

Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>

* This can be simplified now

Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>

* fixup! This can be simplified now

Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>

---------

Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>
(cherry picked from commit e615c7c)

# Conflicts:
#	rclcpp/include/rclcpp/logging.hpp
@mergify mergify bot mentioned this pull request Aug 13, 2025
Closed
mergify bot pushed a commit that referenced this pull request Aug 13, 2025
@Timple @mergify
Allow for implicitly convertable loggers as well (#2922)
5f5526c 
* Allow for implicitly convertable loggers as well

Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>

* Test implicitly convertable logger

Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>

* This can be simplified now

Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>

* fixup! This can be simplified now

Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>

---------

Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>
(cherry picked from commit e615c7c)

# Conflicts:
#	rclcpp/include/rclcpp/logging.hpp
mergify bot pushed a commit that referenced this pull request Aug 13, 2025
@Timple @mergify
Allow for implicitly convertable loggers as well (#2922)
a540b39 
* Allow for implicitly convertable loggers as well

Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>

* Test implicitly convertable logger

Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>

* This can be simplified now

Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>

* fixup! This can be simplified now

Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>

---------

Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>
(cherry picked from commit e615c7c)

# Conflicts:
#	rclcpp/include/rclcpp/logging.hpp
This was referenced Aug 13, 2025
Closed
Closed
@jmachowinski
Copy link
Copy Markdown
Collaborator

jmachowinski commented Aug 13, 2025

If accepted I would like to have this backported to active distros as well.

The backport is not straight forward, as recently the logging was changed from generated to hand coded (5fd8524).
@Timple are you willing to provide a patch for the old releases ?

@Timple
Copy link
Copy Markdown
Contributor Author

Timple commented Aug 13, 2025

Sure, should I push to the mergify branch or create a new one?

And if I patch for kilted, can that be backported automatically to jazzy and humble?

@christophebedard
Copy link
Copy Markdown
Member

christophebedard commented Aug 13, 2025 •
edited
Loading

Sure, should I push to the mergify branch or create a new one?

You probably don't have the necessary permissions to push to the mergify branch on this repo. You can either open a PR against the mergify branch, or simply open a normal PR against the relevant distro branch (while keeping the original PR number in the PR title and commit title, like Mergify did). The latter is probably simpler.

And if I patch for kilted, can that be backported automatically to jazzy and humble?

Yeah, we can do the same thing @jmachowinski did above from the Kilted backport PR.

Timple added a commit to nobleo/rclcpp that referenced this pull request Aug 14, 2025
@Timple
Allow for implicitly convertable loggers as well (ros2#2922)
6d6332f 
Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>
@Timple Timple mentioned this pull request Aug 14, 2025
Merged
jmachowinski pushed a commit that referenced this pull request Aug 18, 2025
@Timple
Allow for implicitly convertable loggers as well (#2922) (#2936)
8a4cb48 
Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>
mergify bot pushed a commit that referenced this pull request Aug 18, 2025
@Timple @mergify
Allow for implicitly convertable loggers as well (#2922) (#2936)
e9bee01 
Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>
(cherry picked from commit 8a4cb48)
mergify bot pushed a commit that referenced this pull request Aug 18, 2025
@Timple @mergify
Allow for implicitly convertable loggers as well (#2922) (#2936)
671214f 
Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>
(cherry picked from commit 8a4cb48)
This was referenced Aug 18, 2025
Merged
Merged
fujitatomoya pushed a commit that referenced this pull request Aug 19, 2025
@mergify @Timple
Allow for implicitly convertable loggers as well (#2922) (#2936) (#2937)
2096d63 
(cherry picked from commit 8a4cb48)

Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>
Co-authored-by: Tim Clephas <tim.clephas@nobleo.nl>
jmachowinski pushed a commit that referenced this pull request Aug 19, 2025
@mergify @Timple
Allow for implicitly convertable loggers as well (#2922) (#2936) (#2938)
443b69b 
(cherry picked from commit 8a4cb48)

Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>
Co-authored-by: Tim Clephas <tim.clephas@nobleo.nl>
@Timple Timple mentioned this pull request Sep 11, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmachowinski jmachowinski jmachowinski approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@ahcorde ahcorde Awaiting requested review from ahcorde

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Timple @fujitatomoya @christophebedard @jmachowinski