-
Notifications
You must be signed in to change notification settings - Fork 22
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
base: ros2
Are you sure you want to change the base?
Conversation
7aca9cd
to
a446f92
Compare
There was a problem hiding this 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.
I've run it locally and it does work, but I can't trigger a test build in |
There was a problem hiding this 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/
_includes/package_links.html
Outdated
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 %} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 7fcbabe.
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. |
That's a lot... Is there any other way we can query whether a ROS 2 package had its API documentation generated? |
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. |
Reviewing the latest trends it appears that we've slowed down overall, probably more to index...
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. |
This was left forgotten. It doesn't seem like we concluded this patch was harmless, or did we? @tfoote |
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. |
@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:
|
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 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. |
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. |
Signed-off-by: Michel Hidalgo <[email protected]>
Signed-off-by: Michel Hidalgo <[email protected]>
Fix the formatting lost for the horizontal list that was relying on the a member.
7fcbabe
to
02cc614
Compare
I have rebased this up to current and fixed the link rendering and changed it to not be a 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. |
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.