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.

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

add quality declaration #202

Draft
wants to merge 1 commit into
base: master
from
Draft

add quality declaration #202

wants to merge 1 commit into from

Conversation

@wjwwood
Copy link
Member

wjwwood commented Jan 30, 2020

This is a quality declaration for rcutils as described in the proposed REP-2004 (see ros-infrastructure/rep#218).

This declaration is aspirational, in that it states things which aren't currently true, but we plan to do soon so we can get it up to the highest level. I'll use it as a kind of TODO list for fixing up rcutils, using line comments to keep track of progress of certain tasks.

This will most likely continue to evolve with the REP as well.

Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood wjwwood added the enhancement label Jan 30, 2020
@wjwwood wjwwood self-assigned this Jan 30, 2020

### Version Scheme

`rcutils` uses `semver` according to the recommendation for ROS Core packages in the [ROS 2 Developer Guide](https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#versioning), and is at or above a stable version, i.e. `>= 1.0.0`.

This comment has been minimized.

Copy link
@wjwwood

wjwwood Jan 30, 2020

Author Member

The link is valid, but the content at the link needs to be reviewed and updated most likely.


TODO fix link

`rcutils` follows the recommended guidelines for ROS Core packages in the [ROS 2 Developer Guide](https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#change_control_process).

This comment has been minimized.

Copy link
@wjwwood

wjwwood Jan 30, 2020

Author Member

The ROS 2 developer guide needs a common change control process section for us to link to and not repeat ourselves too much.

This comment has been minimized.

Copy link
@Blast545

Blast545 Feb 3, 2020

Contributor

Although it's a good idea to have the common rules for versioning/documentation in the developer guide, I think that the quality declaration document should be as self contained as possible and at least contain a brief summary of those guidelines. That would avoid users jumping back and forth to the developer guide when reading the packages documentation.

This comment has been minimized.

Copy link
@wjwwood

wjwwood Feb 3, 2020

Author Member

Hmm, I don't completely agree, but I think we're mostly on the same page. I think the document should make sense on its own, but details definitely should be collected in one place if possible. What I don't want to have to do is that if we change our recommendation from 1-review into 2-reviews for each pull request that I have to touch every repository manually.

In the cases where the linked to guidelines leave some flexibility, then I think it's important for the declaration that is referencing it to clarify what it is choosing to do, e.g. if it chooses line versus branch coverage.

This comment has been minimized.

Copy link
@Blast545

Blast545 Feb 4, 2020

Contributor

Yes, didn't thought about the cases where the recommended guidelines are changed, seems good then to have a 'Recommended guidelines' document to reference


TODO fix link

`rcutils` has a [feature list](TODO) and each item in the list links to the corresponding feature documentation.

This comment has been minimized.

Copy link
@wjwwood

wjwwood Jan 30, 2020

Author Member

Currently the feature list is in the README, which is then used by doxygen, but honestly it should probably be moved into a separate file (set of files) in the doc directory:

INPUT = README.md ./include


TODO fix link

`rcutils` has embedded API documentation and it is generated using doxygen and is [hosted](TODO) along side the feature documentation.

This comment has been minimized.

Copy link
@wjwwood

wjwwood Jan 30, 2020

Author Member

Not 100% sure where we should link. docs.ros2.org for now I guess? For example:

https://docs.ros2.org/eloquent/api/rcutils/index.html


TODO fix link

`rcutils` follows the recommendations for ROS Core packages in the [ROS 2 Developer Guide](https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#coverage), and opts to use branch coverage instead of line coverage.

This comment has been minimized.

Copy link
@wjwwood

wjwwood Jan 30, 2020

Author Member

The developer guide needs a section on coverage too.

This comment has been minimized.

Copy link
@Blast545

Blast545 Feb 3, 2020

Contributor

Developer guide says that the metric to be used when measuring coverage will be branch coverage. However, REP2004 says that metric used in ROS Core packages will be line coverage, aiming for percentage above 95% (in fact talks about branch coverage as something optional).

This comes with two issues:

  1. There's an inconsistency to be solved, maybe discussed, for the ROS2 core packages. Will we use branch or line coverage?
  2. There's no percentage coverage required explicitly for packages outside the ROS2 organization, and maybe this has to be changed in the REP2004 to set a desired standard.

Please check this @chapulina

This comment has been minimized.

Copy link
@wjwwood

wjwwood Feb 3, 2020

Author Member

The developer guide is out of date still, see the original proposed pr (ros2/ros2_documentation#460) that led to REP-2004's proposal:

The end goal would be to move most of this into a REP, and then replace this section with the prescriptions on how to achieve the categories for ROS core packages.

Basically, consider the developer guidelines out-of-date until we separate the requirements into the REP and the prescriptions into a (as yet non-existent) pull request against the developer guide lines.


That being said, I don't feel strongly about the line versus branch coverage. My thinking is that in some cases it might not be technically feasible to do branch coverage, so I left it as "at least line, but may use branch".


With respect to the percentage, that's still being discussed but we will have some recommendation for ROS core packages via the developer guide. What's already there is out-of-date and should be ignored for now.


Current coverage statistics can be viewed here:

TODO FIXME

This comment has been minimized.

Copy link
@wjwwood

wjwwood Jan 30, 2020

Author Member

Once we have some hosted coverage history... link to it.


TODO fix link

`rcutils` follows the recommendations for performance testing of C code in the [ROS 2 Developer Guide](https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#performance-c), and opts to do performance analysis on each release rather than each change.

This comment has been minimized.

Copy link
@wjwwood

wjwwood Jan 30, 2020

Author Member

Developer guide needs a section on performance tests (like the coverage one).


TODO fix link

`rcutils` 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-and-static-analysis).

This comment has been minimized.

Copy link
@wjwwood

wjwwood Jan 30, 2020

Author Member

Developer guide needs a section about standard linters and static analyzers.

This comment has been minimized.

Copy link
@Blast545

Blast545 Feb 3, 2020

Contributor

The linters have some documentation in their repositories Like this one, I think it should be made some effort to include links to all those directly in this section of the tutorials, in a section of the developer guide or maybe in a different index.ros2 article.

This comment has been minimized.

Copy link
@wjwwood

wjwwood Feb 3, 2020

Author Member

Yeah, links to how to use the linters, as well as which ones are on by default, and links to each of those own documentation, should all be part of what's in the developer guide, to which this will link.

Copy link

chapulina left a comment

👍


The license for `rcutils` is Apache 2.0, and a summary is in each source file, the type is declared in the `package.xml` manifest file, and a full copy of the license is in the `LICENSE` file.

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

This comment has been minimized.

Copy link
@chapulina

chapulina Jan 30, 2020

Consider adding links to the files mentioned (package.xml and LICENSE), as well as a link to the results of the automated test.


### Feature Testing

Each feature in `rcutils` has corresponding tests which simulate typical usage, and they are located in the `test` directory.

This comment has been minimized.

Copy link
@chapulina

chapulina Jan 30, 2020

Consider adding a full link to the test directory, for the case when this declaration is rendered outside the repo.


### Public API Testing

Each part of the public API have tests, and new additions or changes to the public API require tests before being added.

This comment has been minimized.

Copy link
@chapulina

chapulina Jan 30, 2020

Suggested change
Each part of the public API have tests, and new additions or changes to the public API require tests before being added.
Each part of the public API has tests, and new additions or changes to the public API require tests before being added.

`rcutils` 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-and-static-analysis).

TODO any qualifications on what "passing" means for certain linters

This comment has been minimized.

Copy link
@chapulina

chapulina Jan 30, 2020

Also add a link to the linter results.


`rcutils` supports all of the tier 1 platforms as described in [REP-2000](https://www.ros.org/reps/rep-2000.html#support-tiers), and tests each change against all of them.

TODO make additional statements about non-tier 1 platforms?

This comment has been minimized.

Copy link
@chapulina

chapulina Jan 30, 2020

It would also be nice to add a link to supporting evidence here. CI results? Binary packages?

This comment has been minimized.

Copy link
@Blast545

Blast545 Feb 3, 2020

Contributor

I think for this would be a good idea to have a link pointing to the latest success jobs in packaging for each of the tier1 platforms. Like the panel shown on the main page of CI but only showing some metrics, maybe including test metrics here too. Not sure if this can be generated easily.

This comment has been minimized.

Copy link
@wjwwood

wjwwood Feb 3, 2020

Author Member

Each job has a badge already, we can use those from the packaging jobs, like these:

But those are for all of the things in the ros2.repos file, not just this package...

We also have the .deb packaging jobs, which are per package, but they are only for Linux and aren't built nightly (right now), e.g.:

There are dev jobs, but those are per repository, not per package, e.g.:

This comment has been minimized.

Copy link
@Blast545

Blast545 Feb 4, 2020

Contributor

Yeah, I thought about those, in the sense that these links can be referenced and checked easily, but done for each package.

Maybe we could point to the latest successful build tests of each package, per platform, something like what's found here:
Test results ament_index_python nightly_osx_debug.


### Copyright Statements

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

This comment has been minimized.

Copy link
@mjcarroll

mjcarroll Jan 30, 2020

Contributor

Do we need to mention DCO here?

This comment has been minimized.

Copy link
@wjwwood

wjwwood Feb 3, 2020

Author Member

What do you think we should mention here? Wouldn't that belong under the "change process" part? The DCO doesn't have anything to do with copyright as far as I know (though it is related), instead thee DCO checks that each commit is signed-off by the person who authored it.

This comment has been minimized.

Copy link
@wjwwood

wjwwood Feb 3, 2020

Author Member

Also, it would be good to comment about this on the REP-2004 pr instead: ros-infrastructure/rep#218

@Blast545 Blast545 mentioned this pull request Feb 6, 2020
10 of 19 tasks complete
@ros-discourse

This comment has been minimized.

Copy link

ros-discourse commented Mar 9, 2020

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/rfc-rep-2004-package-quality-categories/13150/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.