bag_time_manager_rviz_plugin fails to build -Werror=deprecated-declarations

[esteve]

Checklist

• I've read the contribution guidelines.
• I've searched other issues and no duplicate issues were found.
• I'm convinced that this is not my fault but a bug.

Description

The following lines use a deprecated API (rmw_qos_profile_t) which makes bag_time_manager_rviz_plugin fail to build:

https://github.com/autowarefoundation/autoware.universe/blob/main/common/bag_time_manager_rviz_plugin/src/bag_time_manager_panel.cpp#L69-L74

Expected behavior

bag_time_manager_rviz_plugin builds successfully with -Werror=deprecated-declarations enabled

Actual behavior

bag_time_manager_rviz_plugin fails to build with -Werror=deprecated-declarations enabled

Steps to reproduce

Add -Werror=deprecated-declarations as a compiler flag

Versions

No response

Possible causes

No response

Additional context

No response

[taikitanaka3]

@esteve
There are many places we should change to support rolling. So I think we should discuss whether to support rolling or not.

https://github.com/autowarefoundation/autoware.universe/blob/72470412a319238f84e781799f133d832a8c8f5c/common/tier4_state_rviz_plugin/src/autoware_state_panel.cpp

@kenji-miyake @yukkysaito
what do you think about this?

[isamu-takagi]

There are many places we should change to support rolling. So I think we should discuss whether to support rolling or not.

@taikitanaka3 @esteve
There are probably other changes that need to be made to work with rolling. If we support rolling, it is good to create an rolling branch first. And confirming that it works, then checking at the differences to decide whether to merge.

But my opinion is not to support rolling yet as it increases maintenance cost. Is there any reason you want to support rolling?

[xmfcx]

@esteve I've made some changes to make maintenance costs minimum for this PR on following commit: c79c2b1 @isamu-takagi @taikitanaka3 do you think this would be acceptable?

The point is to make it easier to update for the future and at the same time have small maintenance costs in general.

[taikitanaka3]

@xmfcx
so far there is no one to maintain rolling version and I think many change will be neccessary to run with rolling so I also agree to @isamu-takagi 's proposal to make rolling branch in autoware repository first and then we should compare if maintenance cost will be small or not.

[xmfcx]

Why are you against making some packages future proof?

This change is very minimal and it doesn't break anything outside.

Creating a rolling branch will take too long and is not one of our goals in general for now. This change doesn't mean we should make everything work with rolling. It just makes one package future-proof.

@taikitanaka3 @isamu-takagi

[esteve]

I prefer to close this issue and the PR without merging it than creating a new branch. In my experience maintaining multiple branches is more costly than having separate code paths for different versions of ROS 2 (as long as they are minimal), so I'm ok with waiting until Autoware supports ROS 2 Iron to apply this change.

[taikitanaka3]

yes it's better to assign someone responsible for supporting other versions if we do so.

[xmfcx]

yes it's better to assign someone responsible for supporting other versions if we do so.

@taikitanaka3, @esteve means, if he had to choose between "closing this issue" and "creating a rolling branch" he would choose to close this issue. But we still would like to merge this feature.

[esteve]

@xmfcx

But we still would like to merge this feature.

I'll leave it up to you guys if you want to merge it or close it, I'm fine either way, but I'd rather not create a separate branch for rolling.

[taikitanaka3]

@xmfcx
so far motivation is not clear, who wants this change for what？
And this change wouldn't be enough to run at rolling do you plan to change everything？

[taikitanaka3]

@esteve
if we decide to support rolling with awf then it will be finished within a week so there is no worry, but thankyou for PR.

[xmfcx]

@taikitanaka3

And this change wouldn't be enough to run at rolling do you plan to change everything？

If this change makes it enough for this package to run with rolling, that should be enough and ok.

so far motivation is not clear, who wants this change for what？

It is for making this package compile with rolling while maintaining humble compatibility and keeping the changes to minimum.

This could be made for more packages in future too, if necessary. As long as it doesn't increase maintenance costs too much.

[taikitanaka3]

@xmfcx

#2782 (comment)
#2782 (comment)

If this is for future-proof.I recommend to make a unit test for version management to alert compatibility in autoware common package, not in one rviz plugin package. What do you think about it?

[isamu-takagi]

I don't think it's necessary to do now. What is the benefit to support rolling? Since rolling is development version and includes breaking changes, I can't discover advantage in compatibility. Isn't it enough to deal with it all at once when we decide to support iron? And isn't it easier to understand the difference that way?

[mitsudome-r]

From what we discussed in the Autonomy Software WG, I thought the conclusion was that we can merge this PR since the PR is there and it won't break the build in Humble. However, unless there are large demands from users that would like to use Autoware with rolling, I think we don't have to keep Autwoare maintained for rolling because there could be breaking changes everyday in upstream.

[xmfcx]

@taikitanaka3

If this is for future-proof.I recommend to make a unit test for version management to alert compatibility in autoware common package, not in one rviz plugin package. What do you think about it?

We don't need to maintain compatibility with rolling for every package, this issue is not about that. It's for just one package. It could be for another package too.

@isamu-takagi

I don't think it's necessary to do now. What is the benefit to support rolling? Since rolling is development version and includes breaking changes, I can't discover advantage in compatibility.

Maybe for some reason, someone wanted to use a specific package with rolling, this is just for that. This just helps maintaining compatibility without adding much maintenance cost.

Isn't it enough to deal with it all at once when we decide to support iron? And isn't it easier to understand the difference that way?

To me there is no difference. If anything, having some packages be compatible with both humble and rolling will make the transition easier in the future.

In general my reason for discussing this is not only for this particular PR. I saw some resistance for sort of similar PRs:

• feat(contributing): add task scheduling autoware-documentation#282
  • this one could be merged and extra information could be added/updated later
• feat(behavior_velocity_planner): add speed bump module #647
  • this one took too long to merge

I think we should be a bit more accepting PRs to the Universe, especially when they

• don't break the system
• don't add unnecessary maintenance costs
• they provide benefits

[kenji-miyake]

I'm sorry to be late.
I'll write my comments below:

Q: Should Autoware support Rolling?

I believe, Autoware doesn't need to "ensure" builds or behaviors with Rolling, but PRs for Rolling can be merged unless they don't break Humble.
Of course, we should consider maintenance costs.
As Fatih-san has already simplified the code, I think it's acceptable.

Q: Should we merge this PR? If yes, how?

If API updates are only required for this single node, I think as-is is acceptable. (But I suppose it's not.)

If not, we should avoid merging these kinds of PRs separately.
Considering future tracking and review efficiency, we should create a tracking issue (for example, "Follow API changes of Iron") and merge similar PRs at closer timings.
And based on the whole changes, we'll decide whether to wrap with libraries or not, etc.

However, unless there are large demands from users that would like to use Autoware with rolling, I think we don't have to keep Autwoare maintained for rolling because there could be breaking changes everyday in upstream.

The comment above is related to this part by @mitsudome-r.
I agree with this. Always following the latest Rolling APIs would be a little tough for us.
However, not supporting Rolling at all is also not good, I feel.
So instead, I recommend updating with slower frequency (for example, once in a quarter/a half year/a year). Do not create PRs randomly, but systematically update APIs periodically.

Q. Are there alternative approaches?

If we just want to pass Rolling builds, I think we can pass -Wno-deprecated-declarations to colcon build.
If we prepare for Iron and the next LTS, the current approach seems to be good.

---

Also, I agree with @xmfcx -san's comments, but there is a small additional comment.
Regarding don't add unnecessary maintenance costs, I think we can be a bit more careful with this PR. For example, creating a tracking issue as stated above.

@xmfcx @mitsudome-r @esteve @isamu-takagi @taikitanaka3 What do you think about it?

[isamu-takagi]

Maybe for some reason, someone wanted to use a specific package with rolling

@xmfcx I agree if someone finds this plugin useful and wants use it alone with rolling. But I disagree if it's just for compatibility and future proof testing. I know the differences are small, but the maintainers need to manage extra test code. In that case, it is better to create test or sample code separately.

[kenji-miyake]

Just FYI, I'll link Google's recommendation about deprecation.
https://abseil.io/resources/swe-book/html/ch15.html#managing_the_deprecation_process

[isamu-takagi]

@kenji-miyake Totally agree. The point is that some nodes also need the same fix. I still don't understand why only this package needs to be changed.

[xmfcx]

@isamu-takagi

I agree if someone finds this plugin useful and wants use it alone with rolling. But I disagree if it's just for compatibility and future proof testing.

From my point of view, because @esteve has created this PR, he must have had his reasons. And it just helps the package being future proof, that was enough for me to decide.

I also don't know why he tried to use this package on rolling.

[isamu-takagi]

@xmfcx
What I want to say is that this package should not be used as a sandbox. I asked the reason for this issue several times, but I only got answers that seemed to be test code or sample code. They should not be included in main code.

[taikitanaka3]

@isamu-takagi @kenji-miyake @xmfcx @mitsudome-r
I made a package for future proof. I don't think this is necessary but If you would like to add version testing in current autoware . I can open this PR
autowarefoundation/autoware_common#151

[esteve]

From my point of view, because @esteve has created this PR, he must have had his reasons. And it just helps the package being future proof, that was enough for me to decide.

I also don't know why he tried to use this package on rolling.

The only reason I submitted this PR is because I mistakenly ran the Debian generation action for rolling as well as humble, noticed the API incompatibility and submitted a small change to fix it because it wasn't too invasive. I thought it'd be nice to fix this before we do a full migration to iron in the future, which may take some time.

I believe that small incremental changes are easier to maintain that full migrations in separate branches, but as I said, I'm fine with closing this ticket and the associated PR if there's no consensus on whether the change should be merged or not.

[taikitanaka3]

@esteve
now I am ok to merge this. but can you reply to this comment ? And please fix if necessary