Skip to content

Add Other Logging Implementations #858

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

Merged

Conversation

InvincibleRMC
Copy link
Contributor

Added generic Log class and Specializations for LogInfo, LogDebug, LogError, LogWarning.

Closes #821

Copy link
Member

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

A few minor comments, otherwise this looks good

Signed-off-by: Michael Carlstrom <[email protected]>
InvincibleRMC and others added 3 commits April 27, 2025 22:02
Co-authored-by: Christophe Bedard <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
@christophebedard
Copy link
Member

Pulls: #858
Branch: mergify/bp/kilted/pr-36
BUILD args: --packages-above-and-dependencies launch launch_xml launch_yaml
TEST args: --packages-above launch launch_xml launch_yaml
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15809

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

@christophebedard
Copy link
Member

The test failures are unrelated. Thanks for the PR!

@christophebedard christophebedard merged commit b7b31c4 into ros2:rolling Apr 28, 2025
3 checks passed
@christophebedard
Copy link
Member

Pulls: #858
Gist: https://gist.githubusercontent.com/christophebedard/0eed6b9363abeb033c16bff0396696cf/raw/6f35f009047a5ffc1577a49c27074f1c4e1213e1/ros2.repos
BUILD args: --packages-above-and-dependencies launch launch_xml launch_yaml
TEST args: --packages-above launch launch_xml launch_yaml
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15813

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

@christophebedard
Copy link
Member

christophebedard commented Apr 28, 2025

So I just noticed that I didn't properly test this: the first CI run is testing branch mergify/bp/kilted/pr-36 across all repos, which ends up testing ros2/ament_cmake_ros#36 and not this PR. This is because I'm using the ros-ci-for-pr script and re-used a previous command without removing the --branch option. It commented on this PR but didn't test this PR. I only noticed now when triggering CI for another PR.

I think this PR would be fine, but it uses logging.getLevelNamesMapping(), which is only available in Python 3.11. That is greater than the minimum Python language requirement for Rolling/Kilted: https://www.ros.org/reps/rep-2000.html#kilted-kaiju-may-2025-november-2026. Therefore, based on this, I'll revert this PR right away.

christophebedard added a commit that referenced this pull request Apr 28, 2025
christophebedard added a commit that referenced this pull request Apr 28, 2025
@christophebedard
Copy link
Member

Reverted in #865. We'll see if CI passes above, but just based on the minimum Python requirement, we'll have to find an alternative to logging.getLevelNamesMapping().

@InvincibleRMC would you be willing to open another PR that doesn't use logging.getLevelNamesMapping()?

@InvincibleRMC
Copy link
Contributor Author

@InvincibleRMC would you be willing to open another PR that doesn't use logging.getLevelNamesMapping()?

Yeah definitely.

@christophebedard
Copy link
Member

So it does fail on RHEL: https://ci.ros2.org/job/ci_linux-rhel/2748/testReport/junit/launch.test.launch.actions/test_log/test_log_execute/. Windows will probably fail too, knowing that it has an older Python version, but I'll just cancel it.

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.

Add logging actions for severity levels other than 'INFO'
2 participants