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

Per-Package Documentation #301

Merged
merged 7 commits into from
Oct 13, 2020
Merged

Per-Package Documentation #301

merged 7 commits into from
Oct 13, 2020

Conversation

maryaB-osr
Copy link
Contributor

Signed-off-by: maryaB-osr marya@openrobotics.org

Signed-off-by: maryaB-osr <marya@openrobotics.org>
@maryaB-osr maryaB-osr requested a review from tfoote October 5, 2020 16:38
articles/per_package_documentation.md Outdated Show resolved Hide resolved
articles/per_package_documentation.md Outdated Show resolved Hide resolved
Signed-off-by: maryaB-osr <marya@openrobotics.org>
Signed-off-by: maryaB-osr <marya@openrobotics.org>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've got a few things inline that I think could be clarified.

articles/per_package_documentation.md Outdated Show resolved Hide resolved
articles/per_package_documentation.md Outdated Show resolved Hide resolved
articles/per_package_documentation.md Outdated Show resolved Hide resolved
articles/per_package_documentation.md Outdated Show resolved Hide resolved
articles/per_package_documentation.md Outdated Show resolved Hide resolved
articles/per_package_documentation.md Outdated Show resolved Hide resolved
Signed-off-by: maryaB-osr <marya@openrobotics.org>
@ros-discourse
Copy link

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

https://discourse.ros.org/t/request-for-comment-per-package-documentation-requrements/16699/1

In general, our vision for the system is:

- Package maintainers document their packages in their repositories following some guidelines or templates we recommend
- Package documentation from the repositories is built on the ROS 2 infrastructure and deployed to the ROS 2 documentation site in an automatic process with no input from the maintainer besides an opt-in `doc` block in the distribution.yaml
Copy link
Contributor

@SteveMacenski SteveMacenski Oct 5, 2020

Choose a reason for hiding this comment

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

"with no input from the maintainer besides an opt-in doc block", I'm not sure what this fragment is trying to get across. Of course the maintainer has some input, they would have needed to merge the PRs with the docs into the branch to build on the build farm.

I think maybe what you mean isn't "input" but "action" but either way, I think the "deployed to the ROS 2 documentation site in an automatic process" makes that clear. Unless there was another point this was trying to get across, I think it should be removed for clarity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We wanted to emphasize the limited amount of investment on the maintainers' end, but you're right it's not "no input".
Making sure to incorporate @sloretz's point about how one would opt in, I think the following satisfies your issue?

Package documentation from the repositories is built on the ROS 2 infrastructure and deployed to the ROS 2 documentation site in an automatic process maintainers can opt in to by adding a doc block to the distribution.yaml

The "Primary requirements" are those that must be in place for the system to be functional and achieve its purpose.
The "Secondary requirements" are important features and functionality that are not necessary to roll out the first stage of implementation, but should be carried out as soon as possible following the initial roll out.

### Primary requirements
Copy link
Contributor

Choose a reason for hiding this comment

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

An additional primary requirement I'd like to propose is that the format is a common markdown language (rst, md, etc) that can be rendered by GitHub - that will help catch a bunch of issues before release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out, it's one of those things that we internally assume by default so we didn't consider, which is exactly why we wanted outside eyes on this!

We're going to default to rst

@33thou
Copy link

33thou commented Oct 6, 2020

As a newcomer to ROS2 coming from the Python ecosystem, it's often surprising and frustrating to come across github repos for packages with no documentation in their Readme files. In general the relationship between ROS packages, locations in ros.org, and repos in Github is super confusing and it's often not clear to me where to find documentation, file an issue, or ask a question. This is especially hard because of the highly federated mapping to many github organizations and child repositories, so sometimes the hierarchical structures are unclear.

Compare with the PyPi website. Packages have relatively simple introductions and metadata there, but in general package maintainers are encouraged to create robust docs directly in Github, or using a system of choice like readthedocs or a static site generator.

If docs.ros.org is being set up as the canonical way to find documentation that seems useful, but I would think two things are important:

  1. a systematic way to link to an external homepage and docs page if the package maintainer chooses to put their docs elsewhere
  2. a systematic way to generate README.md files in github which link to relevant locations in *.ros.org

thanks for considering!

P.S.: as an aside, I've found index.ros.org to be really awkward to use. I was browsing around looking for packages supported by Foxy related to kalman filters, sensor fusion, IMUs, 3D cameras, and visual odometry. Searching is super confusing because it just gives names of packages with no descriptions or any other useful context like popularity or frequency of updates.

- All documentation will be hosted on `docs.ros.org`
- All documentation will be versioned by ROS 2 distribution names
- The URL structure will be `docs.ros.org/<lang>/<distro>/<page>` for generic documentation and `docs.ros.org/<lang>/<distro>/p/<package_name>/<page>` for package documentation
- For more context on where documentation will fall under this URL structure, see [this diagram](https://docs.google.com/drawings/d/1KxzDrcSZzwGgudk-kEXGWnoRuAtc1ffl_KBeIu-V60Y/edit?usp=sharing).
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to embed this diagram in the page as a PNG stored in the repository.

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 can do that, I just figured it's not really central to this proposal. However, that might be a side effect of how we internally separated the work for the docs into two parts. So, in it goes :)

Changes to the documentation in a package repository shouldn't require the maintainer to manually trigger a build.
The changes should be automatically built and uploaded to the site.

**The system must automatically generate content for a package so it's listed on docs.ros.org even if the package maintainer does not explicitly set up anything in the package repository**
Copy link
Member

Choose a reason for hiding this comment

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

This requirement conflicts with this line from the background:

Package documentation from the repositories is built on the ROS 2 infrastructure and deployed to the ROS 2 documentation site in an automatic process with no input from the maintainer besides an opt-in doc block in the distribution.yaml

While there's no conflict about not setting up anything in the package repository itself, the requirement says the system is opt-out while the line from the background is explicit that it's opt-in.

If the intention is that some page will be generated for every package but documentation will not be generated unless the package opts in, then perhaps clarify this requirement and the background so that is clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

I’d like something to autogenerate given many packages would otherwise have nothing. In ROS1 if you asked it to setup docs in Bloom it would generate API docs even if there was no doxygen which was awesome. Something minimal like that and the readme would at least be something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This requirement is slightly different from the one in the background section, but you're right, it's not clear. The background section is talking about building any actual documentation like API docs and such which would be opt in.

This point is talking about generating some generic content just for an index of packages on the package documentation homepage, that'll include the package name and maybe some other generic info like maintainer, activity, etc. That would be opt out


Every package's "home page" should be reachable with minimal effort by the scheme `docs.ros.org/p/<package_name>/`, which will redirect to default language and version, `docs.ros.org/<lang>/<distro>/p/<package_name>/`

**Package documentation must be maintainable in its repository without going through a third party**
Copy link

Choose a reason for hiding this comment

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

This is a good step forward 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We think so too, thanks for reviewing!


**The system should provide a standardized interface for generating an index page/landing page for packages, consistent across ROS 2**

**The system should provide a standardized method for generating package documentation locally, including build error alerts**
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should be a primary requirement - otherwise the round-trip time required to create properly formatted documentation is going to be quite long (merge a change into the distro branch, wait for the build farm to catch the problem, then update and repeat) - and the net result will likely be many people give up and have either broken or very little documentation.

Also, if there is a method for building package documentation locally, it can also be added to the package CI (I imagine that tools like industrial_ci would quickly integrate this) - otherwise the effort for maintainers to check over documentation changes is going to be quite high.

Copy link
Contributor

Choose a reason for hiding this comment

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

While I agree that being able to build the documentation locally is important, we made it a secondary requirement because it is something we can add later. That is, it's the difference between having a system at all, and having a system that is nice to use.

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 agree that this should be something that gets implemented first just because it will greatly increase ease of use

But our definition for secondary requirements is that they're important but "not necessary to roll out the first stage of implementation", which this does fall under. We need to ensure that we can move quickly through the initial implementation and not get tied up in anything that isn't an absolutely necessity. So, we will definitely add this feature and all the others under "secondary requirements", just not at the first roll-out.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's OK to not have it straight away provided we get it really soon after. GitHub Actions provides a good example of why having to go via the online version makes using the tool extremely painful.

This comment was marked as off-topic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it correct to state that the secondary requirements are still absolute requirements on how the system is architected? My concern is that if this is a secondary requirement, the system may end up being reliant on some cloud-based component - which means we might never be able to fulfill this secondary requirement.

(On the cloud-side of things, I've seen several projects where cloud-independence wasn't specified and you end up with people building something that uses all sorts of proprietary offerings from say AWS - because it is fast to implement, but then you're forever stuck using that proprietary offering or doing a major re-design)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it correct to state that the secondary requirements are still absolute requirements on how the system is architected?

Yes, exactly. That's a great way to put it.

Signed-off-by: maryaB-osr <marya@openrobotics.org>
Signed-off-by: maryaB-osr <marya@openrobotics.org>
Signed-off-by: maryaB-osr <marya@openrobotics.org>
@maryaB-osr maryaB-osr marked this pull request as ready for review October 13, 2020 20:58
@maryaB-osr maryaB-osr merged commit f46c37b into gh-pages Oct 13, 2020
@delete-merged-branch delete-merged-branch bot deleted the per_package_documentation branch October 13, 2020 20:59
- All documentation will be versioned by ROS 2 distribution names
- The URL structure will be `docs.ros.org/<lang>/<distro>/...` for generic documentation and `docs.ros.org/<lang>/<distro>/p/<package_name>/...` for package documentation
- See the diagram below for more context on where documentation will fall under this URL structure:
![](per_package_documentation/Version_package_flowchart.png)
Copy link

Choose a reason for hiding this comment

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

I'm late to the game, but two things:

  • As a usability requirement, all images should have alt text.
  • Should independent packages go under docs.ros.org/<lang>/independent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DLu thank you for pointing out the missing alt text!
And yes, technically everything will fall under docs.ros.org/<lang>, we will make that clearer in the next iteration of the document.

@ros-discourse
Copy link

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

https://discourse.ros.org/t/new-ros-ecosystem-paper/17261/8

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.