Use remote-literal-include to support references to remote code [wrid22]

[gavanderhoorn]

trafficstars

As per subject.

Should fix #2444.

This uses the sphinxext-remoteliteralinclude (Github repository: wpilibsuite/sphinxext-remoteliteralinclude) Sphinx extension to directly include code / resources hosted remotely, instead of having to copy whole sections of code verbatim into the .rst. This extension is essentially identical to literalinclude and supports all the configuration options of that extension.

Screenshot showing how this renders (current version for comparison: here):

This is the first code section in Setting up efficient intra-process communication. I edited that page as it's mentioned in https://github.com/ros2/ros2_documentation/issues/2444#issuecomment-1115146117.

See d08758fb7b84f1b4c9e29c9dfba8489e00eee272 and 39c44db8bdac7752fbfbc35cc6515e79f81a5547 for two example 'styles': the former uses an actual permalink (as suggested in #2444), the latter uses the currently used link, which includes REPOS_FILE_BRANCH.

Both have their pros and cons.

Before updating all pages which reference code, decisions should be made whether:

1. depending on remoteliteralinclude for this is acceptable
2. to use permalinks or not
3. which specific settings for each rli:: directive we'd like to standardise on

Once we reach consensus about this, other pages should be updated to migrate to remoteliteralinclude where applicable, literalinclude should be removed as well as any local copies of sources that accompany pages using that directive.

[gavanderhoorn]

remoteliteralinclude comes very close to what we had on the ROS 1 wiki with <<CodeRef(..)>>.

[gavanderhoorn]

@kurshakuz wrote:

I am leaning more towards using the second approach, as using permalinks adds almost no benefit compared to copy&pasting existing code snippets. It will require same maintenance efforts while requiring to updating all current snippets.

I'd like some more opinions here.

The main reason for permalinks is to avoid documentation going stale without anyone noticing.

Especially things like line-references or emphasis on specific code sections can very easily 'break' if we'd just track HEAD of a particular branch.

[gavanderhoorn]

So if we're not going to change from REPOS_FILE_BRANCH to something else in this PR (that's what #2802 is for): which style of linking do we want to use here? Actual permalinks, or tracking HEAD of each branch?

[fujitatomoya]

CC: @clalancette

[gavanderhoorn]

Friendly ping

[gavanderhoorn]

And another friendly ping

[gavanderhoorn]

I have some time to work on this before the new academic year starts.

Could we arrive at a consensus as to what style of link we'd want for the code snippets (ie: perma-links or chasing-HEAD)?

[clalancette]

Could we arrive at a consensus as to what style of link we'd want for the code snippets (ie: perma-links or chasing-HEAD)?

This is a tough call.

So on the one hand, I agree with you that using HEAD can lead to broken documentation that is hard to notice. So that would argue for using permalinks.

On the other hand, if we use permalinks that means we'll likely have a different permalink per branch, which makes backporting documentation changes between branches harder (basically it leads to more conflicts).

On balance, I think that keeping the documentation consumable for end-users is more important. So I'd argue for using permalinks.

[gavanderhoorn]

An option which would require more work would be to add a task to the deployment script(s) which diff the files referenced by remote-literal-include. A warning could be raised if there are differences detected.

Ideally it'd be the other way around (ie: warn the repositories where included files are hosted), but that'd seem even harder to implement.

---

Edit: I've got a prototype script which:

1. crawls the build/html dir for .html files
2. parses them using BeautifulSoup
3. checks whether they contain remote-literal-include blocks (using a heuristic based on CSS class names), if not: ignore
4. for each remote-literal-include block:
   1. extract all text content from the block (ie: sans markup and/or styling)
   2. calculate SHA256 hash of the content
   3. compare hash with previously stored hash, if different:
      1. print to stdout mentioning page, url, old hash, new hash

Example:

---
- hash_new: dd922ef6f2c530eea5db5160db339028ecb9dc2ddf484b257de368d517e8b21e
  hash_old: af101254f33c4c6af7e2cdf5cf79ae30a9ec65f684a2410de6e4320200602cdb
  page: build/html/Tutorials/Demos/Intra-Process-Communication.html
  url: https://raw.githubusercontent.com/ros2/demos/master/intra_process_demo/src/cyclic_pipeline/cyclic_pipeline.cpp

no output: no remote includes changed.

Using this, we could use HEAD links and still be notified of documentation going stale due to changes in included files. Using HEAD links would make backporting much easier.

Could be something to add to the CI or the site generation buildfarm job.

I have to check how to integrate it with the multi-version sphinx build though.

---

Edit:

I have to check how to integrate it with the multi-version sphinx build though.

just another set of directories with .html files.

[clalancette]

Could be something to add to the CI or the site generation buildfarm job.

While we could add it to the site generation job (https://build.ros.org/job/doc_ros2doc/), the fact of the matter is that it can go a long time before being noticed there. I think I'm the only one that ever looks at that output, and I only look at it from time-to-time. It just doesn't make any issues very visible.

The other option of adding it to CI makes it a lot more visible, but it also punishes unrelated PRs that just happen to be opened (or modified) after an upstream tutorial gets changed. That doesn't seem very fair to contributors.

While I appreciate the work you've done in trying to make this more visible, I'm inclined to use permalinks for now and see how it goes. If it turns out that we are having a lot of conflicts, we can revisit your script and try to find a better place to put it.

[gavanderhoorn]

I'm not the one having to update all those permalinks when (back)porting pages.

So if you're happy/OK with it, so am I.

---

Edit:

The other option of adding it to CI makes it a lot more visible, but it also punishes unrelated PRs that just happen to be opened (or modified) after an upstream tutorial gets changed.

there are ways for Github CI phases to annotate changesets instead of failing if they detect something. That would not make CI fail, and would be easily dismissible by reviewers.

[gavanderhoorn]

I've changed the PR to use permalinks everywhere, and converted a few pages to remote-literal-include just to see how it would look.

Something I've noticed is that the sources in (fi) ros2/examples sometimes already diverge from what is included in some of the tutorials right now. A good example is Tutorials/Intermediate/Writing-an-Action-Server-Client/Cpp.rst. The text states something like "an action server requires 6 things" and shows a bit of code (the ctor). However, the sources (rolling branch, here) have 4 more arguments which are not discussed (could also be I'm looking at the wrong file).

This would be something permalinks could help with, as changes upstream don't get reflected here.

---

Unrelated: there are quite some places where it appears references to existing files in some package(s) are made, but I can't find them, or I'm not looking in the right places.

[gavanderhoorn]

I'm going to close this, as it doesn't appear this is going to get merged.

Other contributors feel free to pick this up.