Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[fetch_binary_drivers] new [fetch_drivers] package #27
[fetch_binary_drivers] new [fetch_drivers] package #27
Changes from 10 commits
f2fab5b
8e144f9
32f18f8
5f022ca
529db79
ead867e
b144529
c5ee991
d305db0
4432ece
68d33fa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 can't find a reference in the drivers to auto_dock
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.
@cjds it might be stripped out of the research platform drivers.
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.
Why non-standard name?
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.
More specific?
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.
There are too many to list. I had several TODOs and I just combined them into one generic one.
I see your comments also say
Why not use DRIVER_VERSION
and that's basically when I rolled back and just used the hardcoded path for now.But it goes into the comments on another comment were we'll need to setup more build/test automation.
For now we're just pulling
DRIVER_VERSION = 0.8.0
and until we have a more automated pipeline we'll leave it at this version and just be sure to rebuild that version of our drivers against the latest melodic especially at the time of a sync.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.
Why not use DRIVER_VERSION
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.
Ditto as above
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.
Link
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.
Markdown will render these fine. Is this a style preference?
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.
Link
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.
Do we need to tell them this?
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.
Link here
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.
Are we not syncing version numbers with the drivers themselves?
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.
Are these comments intentionally left 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.
They're just left over from the catkin generate package command. I think it was just reminding me to add the maintainers.
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.
todo: ensure that these are the same as the ones we used when building the binary drivers.
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.
you won't be able to do that -- the ROS buildfarm / repos only contain the latest debs - once a new version is synced, the old ones go away. Hardcoding the value would make your package uninstallable at best.
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.
@mikeferguson: would a nightly build job of our binaries, ensuring we’ve always got the latest dependencies work here?
@cjds and I were hoping to open source the Dockerfile & Python script which builds the
output.tar.gz
- it won’t be helpful without pull access on our drivers but it would at least make the process transparentThere 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.
@moriarty even having the released deb download a nightly binary build is a challenge -- when a sync to public happens, you'd have a ~24 hour window where the version built against public is no longer right. (and that sync happens every ~ 2 weeks, so that's about 7% downtime). Also how do you make it work on both shadow-fixed (where developers tend to test) and the main ros repo?
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.
😞
@dbking77 & @chadrockey maybe long term we reopen the discussion about refactoring some things to better support public/private drivers to make releasing to research customers easier ❓
We might need a second version setup up and built against shadow-fixed, this still doesn't fix the original problem, and it also wouldn't be clear how to set that up so that the shadow-fixed pulled a
fetch-drivers-shadow.tar.gz
and the release pulledfetch-drivers.tar.gz
especially since currently they're pulling:http://packages.fetchrobotics.com/binaries/fetch-drivers-0.8.0.tar.gz
... Fetch3 & my machine are currently setup against shadow-fixed, but these drivers were built against whatever is inside of
FROM ros:melodic-robot-bionic
(As mentioned @cjds will cleanup the creation process and we plan to put them:
https://github.com/fetchrobotics/fetch_firmware_repackager or rename that repository
fetch_drivers_repackager
)And it works now, but that's likely because there hasn't been any API/ABI breaking changes, which is not always guaranteed.
We'll need to check which of the dependencies we have are core dependencies, and which are non-core dependencies.
For the most part, the 7% downtime is a worst case calculation coming from 24hr every 2 weeks. If the dependencies are part of the core ros repositories, I think the API/ABI is stable enough or at least suppose to be... For non-core, and not released by us then it's just a risk that we'll probably need to take.
I don't even know if the melodic sync is triggered every 2 weeks because most of the folks at open robots are developing towards ros2. @jacobperron & @wjwwood?
Our plan, was to setup a mirror of ROS, which we manually trigger a sync on.
So customers with Fetch Research Robots could point to our http://packages.fetchrobotics.com/ and get guaranteed stability. But have the option to get the packages from packages.ros.org either ros or ros-shadow-fixed if they really want to live on the edge and accept some instability.
By pushing this into the public build farm, our internal
packages.fetchrobotics.com
will become almost a pure mirror, which should have much maintainable for our DevOps team and @bluryi.If we sync our mirror manually, after testing to know we're not in the 7% downtime, then we'll get stability and more frequent updates.
For the 7% of possible down time for the
packages.ros.org
I think @erelson will need to update the (not yet existing) documentation to include strong warnings about pointing topackages.fetchrobotics.com
orpackages.ros.org
The packages.fetchrobotics.com mirror for melodic does not yet exist, we were going to quietly get our packages into melodic, and then setup a mirror once they were in and synced.
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.
@clalancette is the ROS Boss for Melodic, so it's up to him to decide when the syncs happen, though I think they've been trying to keep a regular cadence. Looking at the discourse posts, it's more like once a month, though maybe that will change.
Perhaps you can anticipate the syncs and be prepared with updated packages of your own to sync in order to minimize the period where your debs are out of date. We do usually announce an intention to sync a week or two before doing it, with the intention that people could test it for unintended regressions.
I don't know if this would be relevant as well, but there's this recent announcement by @nuclearsandwich:
https://discourse.ros.org/t/announcing-ros-snapshot-repositories/7705
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.
My intention is to sync Melodic every two weeks, though I've basically never been able to achieve that due to regressions that I have to fix for every sync. So the cadence has indeed been more like once a month.
I intend to continue to attempt to sync every two weeks unless there is a compelling reason to change that. So if you have reasons you'd like it to change, please let me know and we can consider it.
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.
@nickswalker and @mikeferguson I wanted to give you a warning: just in case you've been running melodic and testing with Fetch's using ros-shadow-fixed this is now there:
http://repositories.ros.org/status_page/ros_melodic_default.html?q=fetch_
@erelson will do a test on hardware using the ros-shadow-fixed before we make an official announcement with links to upgrade documentation. So: best case, everything works, but it's more likely that the official announcement to the fetch user mailing lists is going to be not the upcoming ros melodic sync, but the one after. If we're really unlucky we could be in one of those 7% downtime scenarios which was mentioned above.
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.
You probably want 11?
even for 14.04
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.
My 14.04 desktop (still on 3.x kernel... nice and safe...) has 9.x debhelper installed. I don't think there's a reason to bump this requirement presently. And it sounds like the plan is we'll move to using bloom for this later anyways.
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.
What does this mean?
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.
Very brief vague statement, but nominally accurate. It installs roscore + robot systemd units which run at computer power-on. I'll reword this probably.