Update Quality Declarations to level 3.

[clalancette]

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

[ahcorde]

This is not publicly available -> http://docs.ros2.org/latest/api/rosidl_typesupport_cpp/index.html

I will include it

[clalancette]

Sorry, I don't think I understand this comment. As you say, the documentation isn't publically available, but this quality declaration is also saying "it is not yet publicly hosted". So those would seem to agree.

[ahcorde]

PR is open ros2/docs.ros2.org#37 to make it public

[brawner]

The documentation PR has now been merged. You can link to the api link @ahcorde gives above.

[ahcorde]

Use this format https://github.com/ros2/rclcpp/blob/master/rclcpp/QUALITY_DECLARATION.md#direct-and-optional-runtime-ros-dependencies-5i5ii

[clalancette]

Hm. I honestly don't think that format is a good idea. It means that when we go to change the quality level, not only do we have to change it in the source repository for the package itself, we also have to change it in all of the dependent repositories as well. That seems like a lot of churn to me.

[hidmic]

I agree, and we've talked about the maintenance burden before. I expected quality level automation to take care of it. @wjwwood ?

[wjwwood]

I doubt the automation will address dependencies anytime soon due to the complexity and the timelines I think that will be involved. I wouldn't rely on that for now.

Keeping this information up to date is tedious, but also necessary if you want to stay up to date.

[clalancette]

Keeping this information up to date is tedious, but also necessary if you want to stay up to date.

But I don't actually agree that it is necessary. If you just put the quality levels in the packages they pertain to, then you only have to update it in one spot when you change the quality level. Having it in all of the downstream dependencies mean you then have to make a PR for the package it belongs to and for all of the downstream dependencies. That seems more than tedious, and I'm not convinced it adds a lot of benefit anyway.

[clalancette]

So I went ahead and prototyped a solution in python to find quality declarations of dependencies: https://gist.github.com/clalancette/d0a77b0433670803cf69de73c1f05cd0 .

It's not perfect; it has at least the following flaws:

1. There's no option to recurse into sub-dependencies.
2. It currently only looks at <depends> tags.
3. If the QUALITY_DECLARATION.md is at the repository level, rather than the package level, it will fail to find it. This seems more like a bug in those individual packages, but I could make the script smarter as well.
4. The final output is just a raw print of a dictionary.
5. The regex it uses to find the Quality Level of a package is a bit brittle.
6. It makes at least one assumption about the workspace layout.

Regardless, none of the above problems are show-stoppers, and with a few hours of work I could easily fix them. But this is the direction I'd much rather see us head in, rather than duplicating all of the quality information everywhere.

[wjwwood]

The thing that I think will take time is getting the right version for each. In the future when the foxy version is QL 3, Rolling is QL 2, and master is QL 1, how will you know which to check? This will require the user specifying which but then it will require using rosdistro. It isn't impossible, but it is some more work.

Also, what if not all the dependencies should be considered for QL? My thinking is that you need to annotate the package.xml, then have your script avoid looking at those. We need to come up with a way to do the annotation and then detect it, which might be an issue since looking for XML comments or something may not be trivial.

[clalancette]

The thing that I think will take time is getting the right version for each. In the future when the foxy version is QL 3, Rolling is QL 2, and master is QL 1, how will you know which to check?

In the short-term, the easiest way I can think of to do that would be to clone https://github.com/ros2/ros2/blob/master/ros2.repos at the revision you are interested in looking at, then running the script. If you want to look at packages outside of the ROS 2 core, then you could do the same sort of thing with rosinstall_generator at the revision you are interested in.

Also, what if not all the dependencies should be considered for QL?

It's an interesting point. Could you give an example of what you are thinking of here?

I can think of 2 different ways to handle it; either on the producer (package.xml) or consumer (script) side. On the producer side, you would have to do what you said; add some annotation to package.xml, and then respect that. On the consumer side, we could augment the script to take a list of exceptions to ignore. The benefit of the latter is that we don't need to iterate package.xml, and it seems like that exclusion would be a decision the consumer would want to make (not the producer). But it sort of depends on the situations you are thinking of.

[clalancette]

I ended up spending some more time on this, and ended up fixing most of the issues that were pointed out above. The tool is now at https://github.com/clalancette/walk-qds , and can walk dependencies recursively. It now looks at both <depend> and <build_depend> tags (though it could be trivially extended to look at more). I've also added a command-line option to exclude certain dependencies from being considered. It also fixes a bunch of bugs in my earlier prototype.

I'd encourage people to give it a try and see if it fits what we are looking to do here. If it does, I'd like to try to move forward with this PR as-is, and then go back and remove the (what I consider) redundant information from rclcpp.

[brawner]

I'm not here to weigh in on documenting everything vs automation. But I can suggest one possible approach for documentation that I believe should result in minimal maintenance, unless an upstream package downgrades their status.

State that each dependency is at least "QL 3", then link to the location where their current quality levels can be found.
For example: https://github.com/ros2/rcl_interfaces/blob/master/action_msgs/QUALITY_DECLARATION.md#direct-runtime-ros-dependencies-5i5ii

[brawner]

Suggested change

	The features of `rosidl_typesupport_interface` are tested, and their tests are located in the [test directory](https://github.com/ros2/rosidl_typesupport/tree/master/rosidl_typesupport_c/test).
	The features of `rosidl_typesupport_c` are tested, and their tests are located in the [test directory](https://github.com/ros2/rosidl_typesupport/tree/master/rosidl_typesupport_c/test).

[brawner]

The documentation PR has now been merged. You can link to the api link @ahcorde gives above.

[brawner]

I'm not here to weigh in on documenting everything vs automation. But I can suggest one possible approach for documentation that I believe should result in minimal maintenance, unless an upstream package downgrades their status.

State that each dependency is at least "QL 3", then link to the location where their current quality levels can be found.
For example: https://github.com/ros2/rcl_interfaces/blob/master/action_msgs/QUALITY_DECLARATION.md#direct-runtime-ros-dependencies-5i5ii

[clalancette]

I've now fixed most of the thing from review. However, there are still some dependencies here that do not have QUALITY_DECLARATIONS at all, so I think this needs to be on hold until that is resolved.

[brawner]

Just a couple of nits.

[brawner]

Suggested change

	Current coverage statistics can be viewed [here](https://ci.ros2.org/job/nightly_linux_coverage/917/cobertura/src_ros2_rosidl_typesupport_rosidl_typesupport_c_src/). A description of how coverage statistics are calculated is summarized in this page ["ROS 2 Onboarding Guide"](https://index.ros.org/doc/ros2/Contributing/ROS-2-On-boarding-Guide/#note-on-coverage-runs).
	Current coverage statistics can be viewed [here](https://ci.ros2.org/job/nightly_linux_coverage/lastSuccessfulBuild/cobertura/src_ros2_rosidl_typesupport_rosidl_typesupport_c_src/). A description of how coverage statistics are calculated is summarized in this page ["ROS 2 Onboarding Guide"](https://index.ros.org/doc/ros2/Contributing/ROS-2-On-boarding-Guide/#note-on-coverage-runs).

[brawner]

Suggested change

	Current coverage statistics can be viewed [here](https://ci.ros2.org/job/nightly_linux_coverage/917/cobertura/src_ros2_rosidl_typesupport_rosidl_typesupport_cpp_src/). A description of how coverage statistics are calculated is summarized in this page ["ROS 2 Onboarding Guide"](https://index.ros.org/doc/ros2/Contributing/ROS-2-On-boarding-Guide/#note-on-coverage-runs).
	Current coverage statistics can be viewed [here](https://ci.ros2.org/job/nightly_linux_coverage/lastSuccessfulBuild/cobertura/src_ros2_rosidl_typesupport_rosidl_typesupport_cpp_src/). A description of how coverage statistics are calculated is summarized in this page ["ROS 2 Onboarding Guide"](https://index.ros.org/doc/ros2/Contributing/ROS-2-On-boarding-Guide/#note-on-coverage-runs).

[brawner]

rosidl_typesupport_connext_c and rosidl_typesupport_introspection_c were purposely left off of the quality levels effort. You might need to ask @chapulina or @wjwwood for why that was and just add that reasoning to the Quality Declaration.

I think it's fine to merge, since the push to QL 3 doesn't require its dependencies to be QL 3.

[wjwwood]

They were left out, I assume, because they are not affecting the runtime quality of the package. But I don't know that for sure without digging into it again. That's part of what needs to be considered and captured in the QD for this package. @chapulina and I had a file where there were comments about why things were included/excluded at various times, but it should be possible to reason about it again and come to a similar conclusion.

[clalancette]

They were left out, I assume, because they are not affecting the runtime quality of the package. But I don't know that for sure without digging into it again. That's part of what needs to be considered and captured in the QD for this package. @chapulina and I had a file where there were comments about why things were included/excluded at various times, but it should be possible to reason about it again and come to a similar conclusion.

Yeah, you are right about that. Looking at it, those two packages are excluded because they are there as a workaround for a bloom limitation. As such, there are no direct runtime dependencies in this package on them. So I've now removed them from the QUALITY_DECLARATION.md completely.

As such, I think this is ready to go. I have approval from @brawner ; I would prefer to get approval from @ahcorde (original reviewer) before merging. Thanks!

Out of date.

[clalancette]

OK, looks like this one has 2 approvals, so I'll go ahead and merge. Thanks everyone!