Skip to content

Disable links to missing API docs.#219

Draft
hidmic wants to merge 3 commits intoros2from
hidmic/disable-api-docs-properly
Draft

Disable links to missing API docs.#219
hidmic wants to merge 3 commits intoros2from
hidmic/disable-api-docs-properly

Conversation

@hidmic
Copy link
Contributor

@hidmic hidmic commented Jan 31, 2020

Closes #218. This isn't particularly pretty, and we should check what impact per-package HTTP HEAD requests have on overall build time, but it has the desired effect.

@hidmic hidmic requested review from dirk-thomas and tfoote January 31, 2020 16:14
@hidmic hidmic force-pushed the hidmic/disable-api-docs-properly branch from 7aca9cd to a446f92 Compare January 31, 2020 16:15
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

Fine with me - assuming a test build shows it works and isn't too much overhead.

@hidmic
Copy link
Contributor Author

hidmic commented Jan 31, 2020

I've run it locally and it does work, but I can't trigger a test build in build.ros.org myself. @tfoote can you?

Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Testing the overhead does sound important.

Test build triggered here: http://build.ros.org/job/doc_rosindex/394/

href="http://http404error.github.io/roseco/graph.html?id=ros.json&focus={{page.package_name}}&height=1&depth=2&tred=standard&metagroup=false&colorby=Health&direction=LR"
title="View RosEco package graph"
{% unless package.snapshot.documented %}disabled{% endunless %}>
{% endif %}
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be an else not an endif+unless

Copy link
Contributor Author

@hidmic hidmic Feb 3, 2020

Choose a reason for hiding this comment

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

Sure, either way it's fine. It occurred to me it was slightly more readable like this, but we can use else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 7fcbabe.

@tfoote
Copy link
Member

tfoote commented Feb 4, 2020

So the build took 20 minutes longer. When I was watching it felt like the scraping was slower. But I can't verify it because the console output is so long that my browser is crashing rendering it with the timestamps though.

@hidmic
Copy link
Contributor Author

hidmic commented Feb 7, 2020

So the build took 20 minutes longer. When I was watching it felt like the scraping was slower.

That's a lot... Is there any other way we can query whether a ROS 2 package had its API documentation generated?

@tfoote
Copy link
Member

tfoote commented Feb 7, 2020

So the run last night was close to 15 minutes longer. This might be in the noise. I'll try another cycle this afternoon to see if it was just an anomaly.

Edit: New run: http://build.ros.org/job/doc_rosindex/400/

We could look for whether there's a doc entry in the rosdistro. Right now that's still going to overlink but in the future that should be the right metric.

@tfoote
Copy link
Member

tfoote commented Feb 13, 2020

Reviewing the latest trends it appears that we've slowed down overall, probably more to index...

  | Build  ↑ | Duration | Agent
-- | -- | -- | --
  | #403 | 1 hr 57 min | agent-c168b4b2
  | #402 | 2 hr 10 min | agent-c168b4b2
  | #401 | 2 hr 6 min | agent-c168b4b2
  | #400 | 2 hr 11 min | agent-c168b4b2
  | #399 | 2 hr 6 min | agent-c168b4b2
  | #398 | 2 hr 11 min | agent-c168b4b2
  | #397 | 1 hr 56 min | agent-c168b4b2
  | #396 | 1 hr 59 min | agent-c168b4b2
  | #395 | 1 hr 56 min | agent-c168b4b2
  | #394 | 2 hr 14 min | agent-c168b4b2
  | #393 | 1 hr 55 min | agent-c168b4b2
  | #392 | 1 hr 54 min | agent-c168b4b2
  | #391 | 1 hr 55 min | agent-c168b4b2
  | #390 | 1 hr 58 min | agent-c168b4b2

The test builds are #394 and #400 which puts this within 5 minutes of the two neighboring runs. That seems like not too much overhead. We can keep this in mind for optimizing in the future, but we'll get a lot more gains be separating the crawling and the generation.

@hidmic
Copy link
Contributor Author

hidmic commented Apr 17, 2020

This was left forgotten. It doesn't seem like we concluded this patch was harmless, or did we? @tfoote

@tfoote
Copy link
Member

tfoote commented Apr 17, 2020

Going back over this we could do this client side on load. It's cross origin, but if we whitelist index.ros.org on docs.ros.org we should be able to have a small javascript checker. Alternatively we can wait and roll this into a refactor where we separate the scraping from the rendering which I'm looking at doing for #107 as well as allowing us to accelerate or update speed.

@hidmic
Copy link
Contributor Author

hidmic commented Jun 10, 2020

And I didn't get back to this, again. @tfoote is that refactor to address #107 making progress? Otherwise, I'll try to implement it client side (though it feels like a hack, not that the rest is any better 😅).

@hidmic
Copy link
Contributor Author

hidmic commented Mar 9, 2021

@clalancette @tfoote is it safe to assume we don't need this anymore after the documentation migration to docs.ros2.org ? If so, I'd rather close it.

@clalancette
Copy link
Contributor

@clalancette @tfoote is it safe to assume we don't need this anymore after the documentation migration to docs.ros2.org ? If so, I'd rather close it.

So my understanding of the issue here is that we are just blindly generating links to package documentation that doesn't exist. (Please correct me if I'm wrong)

If that is the case, the migration that we've done so far actually doesn't change any of that. The consolidation has just moved things around a bit. We still aren't generating per-package documentation (though it is something we are currently working on).

Thus, I think we still need something like this, though from a build-time perspective it seems a bit scary, for 2 reasons:

  1. The builds at https://build.ros.org/job/doc_rosindex/ already take 2 hours to build. I guess it isn't a huge deal if they take 2.5 hours, but it scales linearly with the number of packages we are indexing.
  2. If the buildfarm has some connectivity problems one day for some reason, basically all of the links won't be generated. That doesn't seem ideal (though I don't have a better idea, other than doing it client-side as you proposed).

@tfoote
Copy link
Member

tfoote commented Mar 17, 2021

We do still need/want this. It's related to the per-package documentation that's still in the works and won't have coverage because not all packages will generated API docs. However once we setup the per package documentation builds we should be able to query the rosdistro for any packages that has the doc entry and only render for those packages instead of needing to poll the web resource. So I guess we could probably close out this PR and plan to fix #218 with the rosdistro based solution.

I haven't had any time to work on #107 or any other optimizations. We have for now found a flag we can set on Jenkins to make it use 4 executors at once so it doesn't run out of memory regularly.

@hidmic
Copy link
Contributor Author

hidmic commented Sep 10, 2021

However once we setup the per package documentation builds we should be able to query the rosdistro for any packages that has the doc entry and only render for those packages instead of needing to poll the web resource. So I guess we could probably close out this PR and plan to fix #218 with the rosdistro based solution.

That sounds reasonable to me. @clalancette does this hold today?

@clalancette
Copy link
Contributor

That sounds reasonable to me. @clalancette does this hold today?

Not quite. We have the automatic job generation happening, and, modulo some bugs in rosdoc2's dependencies, the API documentation is being generated and uploaded to https://docs.ros.org nightly.

Due to some $REASONS, the location we upload to on disk doesn't match the URL structure we laid out in https://design.ros2.org/articles/per_package_documentation.html . So before we can actually have index.ros.org do the right thing, we need to put those redirects in place.

I've done some work locally to make that happen, but I have not had time to deploy it. Once that is done, we should be good to go here.

@tfoote tfoote marked this pull request as draft April 6, 2023 18:50
hidmic and others added 3 commits January 2, 2024 00:55
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Fix the formatting lost for the horizontal list that was relying on the a member.
@tfoote tfoote force-pushed the hidmic/disable-api-docs-properly branch from 7fcbabe to 02cc614 Compare January 2, 2024 09:20
@tfoote
Copy link
Member

tfoote commented Jan 2, 2024

I have rebased this up to current and fixed the link rendering and changed it to not be a <a> instance if the url is blank instead of just launching a non-link.

Looking at this though I think that we should be able to do this in javascript in the browser and rewrite the elements placeholder in the browser instead of needing to pre-render it into the static site. This will make the content more dynamic in case some docuementation is added and decrease the build burden.

Now that there's CI this does show an increased running time which I think on our very long runs would be not great.

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.

Avoid placing broken links to documentation on index listings

4 participants