-
Notifications
You must be signed in to change notification settings - Fork 24
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
Update system_dependencies, fixing bugs, removing cruft, and adding descriptions from Python #416
Update system_dependencies, fixing bugs, removing cruft, and adding descriptions from Python #416
Conversation
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.
I don't have a good answer on the width. Updating it's fine. The mobile rendering is not the best, but it gives the same amount of info as before on the default view without scrolling to the right.
If you are aware of a better source of descriptions, please tell me. Also the ROS-packaged dependencies presumably have descriptions available somewhere, do you know how to find those?
Unfortunately we don't have this. We mostly rely on systems packaging things similarly, and generally default to the debian/ubuntu version as canonical as it's our default development platform. This is something that we've had occasional discussions about how to cannonicalize it, with proposals such as using the CMake Find rules etc. But so far we're still working in a best effort without canonicalizing.
Descriptions change rarely, and the file is big, so I thought it better to download it to the repo rather than every time rosindex is run.
I'm not sure that this is a good optimization. Downloading 7MB should be relatively quick and should avoid us having to pay attention to when it goes out of sync. This would become one more thing for us to keep track of keeping up to date. And that existing in the codebase means that it's something that will require regular manual interventions. I'd rather keep this out of the source code. Where did you find the data to source? We could potentially keep this data in a cache locally in the workspace to avoid needing to redownload it if you rerun the build by fetching via rsync or the like.
So other than not adding the package list to the repo this looks like a great improvement to me.
f8d6128
to
efe7578
Compare
I've moved the deps description download into ruby, doing every time it runs as requested. |
On my nightly run of this, I had a failure in the descriptions download:
But that is the first after around 3-4 successful runs. Not sure if that means this read is going to be unreliable. |
Thanks for moving it to a pull model. The whole layout gets a lot more information through. For the download, packages.debian.org sometimes is flakey. Two requests for that, if we can put a short retry logic in that will make it a lot more robust. Aka retry after 10 second and maybe 60 seconds. In addition to that since we don't believe that this content will be updating regularly we can allow it to not fail the build if the content already exists in the workspace cache. We run our nightly builds with the same cache to allow faster restarts so if it has the cache to fall back on it should not be a problem being out of date until the pull can succeed again. |
On request could we add a count of packages in which it's used in the main display table. This is really helpful for understanding what's something that has gone out of date or us barely used, versus something that's a core dependency. |
…r down This fixes ros-infrastructure#388 It fixes the broken links by removing them as the extra column's info has been reduced. And while here make it symmetric so you navigate packages both up and down the dependency list.
d37851f
to
3722635
Compare
I rebased this on current ros2 + #427 and force pushed, maybe that wasn't the right approach? Anyway added the debian retry and the counts to the system dependencies. You mentioned that you wanted the cache to backup a failed download of debian descriptions, so I explicitly added code to support that. I have mixed feelings though. The way the code is now, if the debian descriptions download stopped working completely, there is no indication other than a couple of lines of code added in a very dense output file. That could go on forever on the build farm. Developers will notice though, as there will be this annoying 10 sec then 60 sec freeze in the rosindex run with a warning showing. I also have mixed feelings about the cache in general (#405). But the only other choices are to fail rosindex completely after failed retries, or allow rosindex to succeed but with empty descriptions. Neither are good options if the download fails frequently. Anyway, https://index.rosdabbler.com now has this PR in it on the last run. |
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.
Yeah, the caching can be an issue. We need to make sure it doesn't take us too wrong. But the build time for this overall is a problem.
I think that the tradeoff that devs will see it pretty promptly if it's not working should be enough that we'll catch it and fix it before the site goes too far out of date. As long as it's eventually healing that should be enough. And we will see it occasionally if the Jenkins instance workspace gets cleared.
I'm happy with the rebase and force push especially as we close in on the final review.
I started looking at the system_dependencies page (mainly because it was getting in the way of something else I was doing), and one thing led to another, so here is an update.
This contains a number of commits, which could be viewed and landed separately if desired. The changes:
"Fix System Dependencies to properly show availability" This just eliminates some bugs that were keeping the dependencies from appearing.
"Switch to 1920 width": For some reason the width was limited to 1000px. Expanding the system_dependencies page needs all the space it can get, so I expanded this to 1920. I'm not sure why width is being limited at all, bootstrap generally deals with smaller screens, but I am not really sure what the issues might be.
"Add debian descriptions to system dependencies". This is probably the most controversial change. rosdeps does not appear to have any descriptions of the dependencies, but displays of them are not very meaningful without descriptions. I found a compact page from Debian that shows the descriptions, and this works for the majority of our rosdep entries. More work would be needed to get other descriptions. Having the majority of descriptions available is better than nothing IMHO.
If you are aware of a better source of descriptions, please tell me. Also the ROS-packaged dependencies presumably have descriptions available somewhere, do you know how to find those?
Descriptions change rarely, and the file is big, so I thought it better to download it to the repo rather than every time rosindex is run.
"Move 'Used in Packages' to top of 'System Dependencies'". I think this section is more useful than the list of all of the systems that might contain the dependency, so I moved it to the top.
"Remove deps list dependence on distro". The dependency list had a strange dependency on rosdistro that was partly commented out. I don't see any actual dependence though of rosdeps on rosdistro, so I tried to disentangle this, which reduces by a lot the space required for serving the system_dependencies list.
You can see the results at http://index.rosdabbler.com/deps/ (That has a few additional patches landed, but I don't think those affect the system dependencies. EXCEPT I limit older repos, so some packages that show no usage might be used on older packages).