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.

Sign up for GitHub

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

Jump to bottom

Update Quality Declarations to level 3. #77

Merged
merged 5 commits into from
Jul 17, 2020
Merged

Update Quality Declarations to level 3. #77

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
323527d
Update Quality Declarations to level 3.
clalancette Jun 25, 2020
5f40330
Fixes from review.
clalancette Jun 25, 2020
4cb9459
Update from review.
clalancette Jul 10, 2020
02e1ad2
Fixes from review.
clalancette Jul 10, 2020
89a6fd3
Remove packages from QD that aren't runtime deps.
clalancette Jul 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 19 additions & 10 deletions rosidl_typesupport_c/QUALITY_DECLARATION.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ This document is a declaration of software quality for the `rosidl_typesupport_c

# rosidl_typesupport_c Quality Declaration

The package `rosidl_typesupport_c` claims to be in the **Quality Level 4** category.
The package `rosidl_typesupport_c` claims to be in the **Quality Level 3** category.

Below are the rationales, notes, and caveats for this claim, organized by each requirement listed in the [Package Requirements for Quality Level 4 in REP-2004](https://www.ros.org/reps/rep-2004.html).
Below are the rationales, notes, and caveats for this claim, organized by each requirement listed in the [Package Requirements for Quality Level 3 in REP-2004](https://www.ros.org/reps/rep-2004.html).

## Version Policy [1]

Expand All @@ -14,7 +14,7 @@ Below are the rationales, notes, and caveats for this claim, organized by each r

### Version Stability [1.ii]

`rosidl_typesupport_c` is not yet at a stable version, i.e. `>= 1.0.0`.
`rosidl_typesupport_c` is at a stable version, i.e. >= 1.0.0. The current version can be found in its [package.xml](./package.xml), and its change history can be found in its [CHANGELOG](./CHANGELOG.rst).

### Public API Declaration [1.iii]

Expand All @@ -30,7 +30,6 @@ All installed headers are in the `include` directory of the package, headers in

`rosidl_typesupport_c` contains C code and therefore must be concerned with ABI stability, and will maintain ABI stability within a ROS distribution.


## Change Control Process [2]

`rosidl_typesupport_c` follows the recommended guidelines for ROS Core packages in the [ROS 2 Developer Guide](https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#package-requirements).
Expand All @@ -40,6 +39,7 @@ All installed headers are in the `include` directory of the package, headers in
This package requires that all changes occur through a pull request.

### Contributor Origin [2.ii]

This package uses DCO as its confirmation of contributor origin policy.
More information can be found in [CONTRIBUTING](../CONTRIBUTING.md).

Expand Down Expand Up @@ -77,23 +77,29 @@ The license for `rosidl_typesupport_c` is Apache 2.0, and a summary is in each s

There is an automated test which runs a linter that ensures each file has a license statement.

Most recent test results can be found [here](http://ci.ros2.org/job/nightly_linux_release/lastBuild/testReport/rosidl_typesupport_c/copyright).

### Copyright Statements [3.iv]

The copyright holders each provide a statement of copyright in each source code file in `rosidl_typesupport_c`.

There is an automated test which runs a linter that ensures each file has at least one copyright statement.

Most recent test results can be found [here](http://build.ros2.org/view/Epr/job/Epr__rosidl_typesupport__ubuntu_bionic_amd64/lastBuild/testReport/rosidl_typesupport_c/)
Most recent test results can be found [here](http://ci.ros2.org/job/nightly_linux_release/lastBuild/testReport/rosidl_typesupport_c/copyright).

## Testing [4]

### Feature Testing [4.i]

There are currently no public features undergoing tests.
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).
Copy link
Contributor

Choose a reason for hiding this comment

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

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).


Most recent test results can be found [here](https://ci.ros2.org/job/nightly_linux_release/lastBuild/testReport/rosidl_typesupport_c).

### Public API Testing [4.ii]

There are currently no tests for the public API.
The public API of `rosidl_typesupport_interface` is tested, and the tests are located in the [test directory](https://github.com/ros2/rosidl_typesupport/tree/master/rosidl_typesupport_c/test).

Most recent test results can be found [here](https://ci.ros2.org/job/nightly_linux_release/lastBuild/testReport/rosidl_typesupport_c).

### Coverage [4.iv]
brawner marked this conversation as resolved.
Show resolved Hide resolved

Expand All @@ -105,13 +111,14 @@ There are currently no tests for the public API.

### Linters and Static Analysis [4.v]

`rosidl_typesupport_c` uses and passes all the standard linters and static analysis tools for a C++ package as described in the [ROS 2 Developer Guide](https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#linters).
`rosidl_typesupport_c` uses and passes all the standard linters and static analysis tools for a C package as described in the [ROS 2 Developer Guide](https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#linters).

Results of the linting tests can be found [here](http://build.ros2.org/view/Epr/job/Epr__rosidl_typesupport__ubuntu_bionic_amd64/lastBuild/testReport/rosidl_typesupport_c/).
Results of the linting tests can be found [here](https://ci.ros2.org/job/nightly_linux_release/lastBuild/testReport/rosidl_typesupport_c/).

## Dependencies [5]

### Direct Runtime ROS Dependencies [5.i/5.ii]

`rosidl_typesupport_c` has the following runtime ROS dependencies:
* `rcpputils`
* `rosidl_runtime_c`
Expand All @@ -136,6 +143,8 @@ Currently nightly results can be seen here:
* [mac_osx_release](https://ci.ros2.org/view/nightly/job/nightly_osx_release/lastBuild/testReport/rosidl_typesupport_c/)
* [windows_release](https://ci.ros2.org/view/nightly/job/nightly_win_rel/lastBuild/testReport/rosidl_typesupport_c/)

## Vulnerability Disclosure Policy [7.i]
## Security [7]

### Vulnerability Disclosure Policy [7.i]

This package conforms to the Vulnerability Disclosure Policy in [REP-2006](https://www.ros.org/reps/rep-2006.html).
38 changes: 25 additions & 13 deletions rosidl_typesupport_cpp/QUALITY_DECLARATION.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ This document is a declaration of software quality for the `rosidl_typesupport_c

# rosidl_typesupport_cpp Quality Declaration

The package `rosidl_typesupport_cpp` claims to be in the **Quality Level 4** category.
The package `rosidl_typesupport_cpp` claims to be in the **Quality Level 3** category.

Below are the rationales, notes, and caveats for this claim, organized by each requirement listed in the [Package Requirements for Quality Level 4 in REP-2004](https://www.ros.org/reps/rep-2004.html).
Below are the rationales, notes, and caveats for this claim, organized by each requirement listed in the [Package Requirements for Quality Level 3 in REP-2004](https://www.ros.org/reps/rep-2004.html).

## Version Policy [1]

Expand All @@ -14,7 +14,7 @@ Below are the rationales, notes, and caveats for this claim, organized by each r

### Version Stability [1.ii]

`rosidl_typesupport_cpp` is not yet at a stable version, i.e. `>= 1.0.0`.
`rosidl_typesupport_cpp` is at a stable version, i.e. >= 1.0.0. The current version can be found in its [package.xml](./package.xml), and its change history can be found in its [CHANGELOG](./CHANGELOG.rst).

### Public API Declaration [1.iii]

Expand All @@ -28,7 +28,7 @@ All installed headers are in the `include` directory of the package, headers in

### ABI Stability Within a Released ROS Distribution [1.v]/[1.vi]

`rosidl_typesupport_cpp` contains C code and therefore must be concerned with ABI stability, and will maintain ABI stability within a ROS distribution.
`rosidl_typesupport_cpp` contains C++ code and therefore must be concerned with ABI stability, and will maintain ABI stability within a ROS distribution.

## Change Control Process [2]

Expand All @@ -39,6 +39,7 @@ All installed headers are in the `include` directory of the package, headers in
This package requires that all changes occur through a pull request.

### Contributor Origin [2.ii]

This package uses DCO as its confirmation of contributor origin policy.
More information can be found in [CONTRIBUTING](../CONTRIBUTING.md).

Expand Down Expand Up @@ -68,31 +69,37 @@ All pull requests must resolve related documentation changes before merging.

### Public API Documentation [3.ii]

`rosidl_typesupport_cpp` has documentation of its public API, but it is not yet hosted.
`rosidl_typesupport_cpp` has documentation of its public API, but it is not yet publicly hosted.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

I will include it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


### License [3.iii]

The license for `rosidl_typesupport_cpp` is Apache 2.0, and a summary is in each source file, the type is declared in the [package.xml](package.xml) manifest file, and a full copy of the license is in the [LICENSE](./LICENSE) file.

There is an automated test which runs a linter that ensures each file has a license statement.

Most recent test results can be found [here](http://build.ros2.org/view/Epr/job/Epr__rosidl_typesupport__ubuntu_bionic_amd64/lastBuild/testReport/rosidl_typesupport_cpp/)
Most recent test results can be found [here](http://ci.ros2.org/job/nightly_linux_release/lastBuild/testReport/rosidl_typesupport_cpp/copyright).

### Copyright Statements [3.iv]

The copyright holders each provide a statement of copyright in each source code file in `rosidl_typesupport_cpp`.

There is an automated test which runs a linter that ensures each file has at least one copyright statement.

Most recent test results can be found [here](http://ci.ros2.org/job/nightly_linux_release/lastBuild/testReport/rosidl_typesupport_cpp/copyright).

## Testing [4]

### Feature Testing [4.i]

There are currently no public features undergoing tests.
The features of `rosidl_typesupport_cpp` are tested, and their tests are located in the test directory.

Most recent test results can be found [here](https://ci.ros2.org/job/nightly_linux_release/lastBuild/testReport/rosidl_typesupport_cpp).

### Public API Testing [4.ii]

There are currently no tests for the public API.
The public API of `rosidl_typesupport_cpp` is tested, and the tests are located in the test directory.

Most recent test results can be found [here](https://ci.ros2.org/job/nightly_linux_release/lastBuild/testReport/rosidl_typesupport_cpp).

### Coverage [4.iii]
brawner marked this conversation as resolved.
Show resolved Hide resolved

Expand All @@ -106,17 +113,20 @@ There are currently no tests for the public API.

`rosidl_typesupport_cpp` uses and passes all the standard linters and static analysis tools for a C++ package as described in the [ROS 2 Developer Guide](https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#linters).

The latest linting results can be found [here](http://build.ros2.org/view/Epr/job/Epr__rosidl_typesupport__ubuntu_bionic_amd64/lastBuild/testReport/rosidl_typesupport_cpp/).
Results of the linting tests can be found [here](https://ci.ros2.org/job/nightly_linux_release/lastBuild/testReport/rosidl_typesupport_cpp/).

## Dependencies [5]

### Direct Runtime ROS Dependencies [5.i/5.ii]
### Direct and Optional Runtime ROS Dependencies [5.i/5.ii]

`rosidl_typesupport_cpp` has the following runtime ROS dependencies:
* `rcpputils`
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@clalancette clalancette Jun 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@clalancette clalancette Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

* `rosidl_runtime_c`
* `rosidl_typesupport_cpponnext_c`
* `rosidl_runtime_cpp`
* `rosidl_typesupport_c`
* `rosidl_typesupport_connext_cpp`
* `rosidl_typesupport_interface`
* `rosidl_typesupport_introspection_c`
* `rosidl_typesupport_introspection_cpp`

It has "buildtool" dependencies, which do not affect the resulting quality of the package, because they do not contribute to the public library API.
It also has several test dependencies, which do not affect the resulting quality of the package, because they are only used to build and run the test code.
Expand All @@ -135,6 +145,8 @@ Currently nightly results can be seen here:
* [mac_osx_release](https://ci.ros2.org/view/nightly/job/nightly_osx_release/lastBuild/testReport/rosidl_typesupport_cpp/)
* [windows_release](https://ci.ros2.org/view/nightly/job/nightly_win_rel/lastBuild/testReport/rosidl_typesupport_cpp/)

## Vulnerability Disclosure Policy [7.i]
## Security [7]

### Vulnerability Disclosure Policy [7.i]

This package conforms to the Vulnerability Disclosure Policy in [REP-2006](https://www.ros.org/reps/rep-2006.html).