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

Disable links to missing API docs. #219

Draft
wants to merge 3 commits into
base: ros2
Choose a base branch
from

Conversation

hidmic
Copy link

@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 tfoote and dirk-thomas 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
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
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
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
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
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
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
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

@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
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

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