-
Notifications
You must be signed in to change notification settings - Fork 79
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
Release for bionic doesn't include #42 #45
Comments
@j-rivero @clalancette Was a new release of |
I'm afraid it was not, same version than in artful, upstream 1.0.0. Bionic is already released so it is too late to follow the normal course and the only option is to consider is to check if the problem falls into the Stable Release Update policy. |
In your experience would it be covered by that policy? Essentially for machines with different locales, urdf parsing does not work, so it is high impact in the user-facing sense, but it's not a security-related bug or anything. If it's not possible, we need to consider alternatives. Not sure if custom packaging would be feasible, or if we have to fallback to increased visibility of the issue and workaround. Since there is a workaround, the latter might be the most appropriate if the former is time-consuming, but I imagine it will still cost some users hours of debugging before they discover the workaround, no matter how visible we try to make it. |
I don't know Ubuntu's update policy in particular, but in general this should be a safe change to put into a stable update. It doesn't change any external API/ABI, and has some soak time now, so it should have a low chance of regressions. I'd suggest we attempt to get an update into Ubuntu before falling back to workarounds, though I don't know the process there. Maybe @j-rivero can give us a hint. |
Yes, the problem could be accepted as an SRU, not 100% sure but we can try it. The process of submitting is well described in the SRU page. You can get some inspiration from one of previous submissions: sdformat or urdfdom-headers for example. If you need my help to do it, I can try to allocate some time to do it next week, |
I know we're supposed to use Github's |
@clalancette are you working on this or anybody else? |
I'm not currently working on it, no. I probably won't have time for this in the near future, so if someone else wants to take up getting the update into Ubuntu, that would be appreciated. I'm happy to provide review/support as needed. |
@clalancette could you do a new release 1.0.1 including the fix? @j-rivero is updating to a patch release allowed as SRU? My interpretations is yes and it would be cleaner I would then file a SRU bug report asking to update to 1.0.1 |
@simonschmeisser I'm not the maintainer of this package, so I can't directly do the release. @scpeters would you mind doing a new release here just to make sure we have an official tag with the locale fixes? |
RViz doesn't display surfaces when LC_NUMERIC has no value. Source: |
@scpeters ping? |
Here's a draft of the release notes for a 1.1.0 release: https://github.com/ros/urdfdom_headers/releases/tag/untagged-1fc2b13e629cb2aa7873 |
Thanks for getting to this so quickly after soccer ;) The link doesn't work for me |
@j-rivero any progress? |
Yes. I've updated the debian-sid repository with the latest version: https://packages.debian.org/source/sid/urdfdom-headers. With this step the gate to submit the SRU is open. |
@j-rivero Thanks a lot, that is good news! Just to be certain, will you submit the SRU or should I (who has never done that) do that? |
I can do it but for it we would need an small example (test code) that demonstrate the problem with the current version and how it is fixed with the patch. |
No problem. We can make a new patch release as soon as the PR is merged and import it to debian. |
Just realised: #42 is the answer to everything. |
I filed an Ubuntu bug report asking for an update, you can vote there by clicking "this bug affects me": https://bugs.launchpad.net/ubuntu/+source/urdfdom-headers/+bug/1817595 |
I saw on ROS Discourse that @kyrofa from Canonical is hiring roboticists, perhaps he has some suggestion on how to proceed on the bug/SRU. |
If the time it takes to release the fixed package to bionic is expected to be longer, it might be handy to add a temporary env_hook to the dependent ROS packages. Or a build-time warning issued by urdfdom-headers (which would although not help people who just install everything via apt and run rviz). In my packages, I'm adding the env hook that sets |
Today, the updated urdfdom_headers package has been released to bionic. It includes the fix for #42. |
@j-rivero Do you think it'd be possible to at least increase patch version number for the released Ubuntu package? This way it's pretty difficult to tell if the user has the fixed version or not (e.g. from CMake). |
@gavanderhoorn probably from the SRU bug. We released the fix into bionic-updates on the 7th thanks to @j-rivero's hard work. |
Ok, thanks. Obviously 💯 👍 🎉 🎂 🍻 🍔 😄 etc for getting this released, but As @simonschmeisser already asked on the SRU: does anyone know what the major blockers/bottlenecks were here? If it's always this difficult to get an obvious bug fixed in a released package in Debian/Ubuntu it makes me wonder whether it's worth it to upstream these sort of dependencies. Also: @clalancette. |
To be fair, that bug was logged on 2019-02-25. Not trying to say that's a great timeline either, but it's better than 1.5 years, heh.
I have a few thoughts to share, if I may. SRUs are a foreign conceptUbuntu takes stability incredibly seriously. Once a release has been cut you can only get security and critical bugfixes back into it by way of a Stable Release Update (SRU), the process and workflow for which is well-documented and hasn't changed for years. I won't pretend it's perfect:
However, it's still the process for how to get an SRU and what types of things are okay to SRU. Getting familiar with that process will streamline things, as I'll discuss below. The bug didn't follow SRU guidelines at firstThis bug was logged in February, as I mentioned. It didn't follow the SRU guidelines until August, though. Once it did it should have been reviewed, but sadly that didn't happen until September, where changes were requested. After some back and forth, it got sponsored in October, put into -proposed in October, tested, and released 7 days later in November. That timeline is still nothing to brag about, of course. SRUs typically take a matter of weeks, even for us. Why did this one take three months? One of the reasons was... This patch was not a simple bugfixIf the person submitting patches doesn't have the ability to get them into Ubuntu itself and babysit them, they need a sponsor to do it (I'm included in that group-- I also need a sponsor). That sponsor takes responsibility for your code and will make sure that tests pass and it doesn't break anything else. However, Ubuntu developers aren't experts in every package, so the patches for which they're willing to take responsibility tend to be easy to understand and test. This did not fall into that category, so it took some convincing from both myself and @j-rivero to get it sponsored in the first place, and then we had to have the same conversation a number of times with various reviewers. Not every patch will look like that, thus not every SRU will require those conversations to happen. In conclusion, I do think there are some improvements to be had that can improve this in the future, but SRUs are always going to be slower than you want them to be. If you're willing to see them through, though, you can get the fix to the maximum number of people. |
I'm really glad this journey is over, what an endurance test ...
Those guidelines mention cases in which an update to a upstream patch release (1.0.3) is possible. As I had and still have the opinion that those are fulfilled, I argued for going this way as it would solve the problem @peci1 now has about differences between upstream and Ubuntu version with an identical version tag (1.0.0). Obviously increasing the patch version at the Ubuntu side as suggested by @peci1 would only increase the mess. In hindsight it might have been better not to argue and to go for the "minimal patch" version. I somewhat doubt that however, as this seems mostly like a combination of missing procedures at Ubuntu/Canonical (there seems to be no "default assignee" or "caretaker" looking at and promoting SRUs that go unnoticed) and a lack of overlap of social networks between Ubuntu and ROS people. I'm currently trying to "infiltrate" Debian and call for others to join! 😉 What we as a comunity (this includes @clalancette and people at OSRF) should do in future is to either stop upstreaming stuff or try to align better with Debian and especially Ubuntu release schedule. Just as a recap the process seems to be as following:
So basically we should release and test everything we need for Noetic before February 2020 and upload it to Debian. This is all meant in a constructive way and I hope I'm not offending anybody unintentionally! (Just as a side note, I haven't received any feedback in two weeks from any Debian mentor on my attempts to update ogre in Debian so I'm not sure how well above theory turns out in practice 😞 https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=943675 ) |
Indeed, step four of the procedure is "Prepare the upload, attach a debdiff to the bug, and request sponsorship by subscribing 'ubuntu-sponsors' to the bug." That will notify folks that an SRU needs to be reviewed and sponsored. Those are the guidelines to which I'm referring. To be clear, I'm not trying to defend them (I think they're way too heavy), I'm simply pointing out that a process exists that was not followed, in response to a "how can we do this better next time if we decide to do so" question.
Yes, that would allow you to skip the SRU process altogether, which I wholeheartedly recommend! |
This discussion definitely goes OT here, maybe a discourse thread should be started for the upstreaming topic? Just my piece: does anybody know (i.e. has OSRF discussed/decided) what's the actual benefit of upstreaming packages. In my point of view, it only creates mess and unnecessary burden. I got pretty much disoriented e.g. here: https://discourse.ros.org/t/gazebo-version-in-packages-ros-org-is-quite-outdated/10950 . And look at this issue - if urdfdom-headers were released via ROS repo, there would be no problem at all. |
It was reported on the Ubuntu tracker then, yes. I was referring to when this issue (#45) was opened (and even that is conservative, as it was already fixed before this one, but unreleased for Bionic). |
I would be interested to know what the rationale was for upstreaming these packages in the first place. It may have been discussed, but I don't recall where or what the outcome was. If @clalancette or anyone from O(S)R(F) could point us to the discussion that would be great. |
To close this and follow up with the offtopic discussion, I would to do/propose to things:
|
I'll respond there as well, but for me (and I believe others as well) the topic was a bit broader: not just this particular case, but a general rationale for why certain packages get upstreamed to Debian/Canonical. |
I'm honestly not sure; that decision was taken before I was involved. I will say that in general, I am a fan of upstreaming packages into the Linux distributions where it make sense. It adds an additional layer of...accountability to releasing changes to many downstream users that I find highly desirable as a user. That being said, this clearly dragged on too long. Thanks to @kyrofa for pointing out the problems in the way this was undertaken, both from our side and from the Ubuntu side. I think in the future we will have a better idea of how to approach this. @kyrofa , would you be willing to be a point-of-contact for us in the future when we need to do releases like this in the future? |
This PR doesn't seem to have made it into the packaged
urdfdom_headers
in bionic. The 1.0.0 tag on this repo doesn't seem to include this PR, and the source code zip for 1.0.0-1 listed at https://packages.ubuntu.com/bionic/amd64/liburdfdom-headers-dev doesn't include thestrToDouble
function, so there doesn't look to have been a new release since.I know that this is released as an upstream ubuntu package, which brings some limitations on what/when it can be released, so there may be reasons that the version in bionic does not include that PR. But from @clalancette's comment referencing getting it in before melodic, I was expecting it to be included.
The issue it addresses is affecting a number of rviz users in a pretty severe way after upgrading to artful/bionic (see ros-visualization/rviz#1249, ros-visualization/rviz#1151), and is quite difficult to track down if you don't know to be looking at locales. Is it possible that it can be pushed to bionic somehow?
The text was updated successfully, but these errors were encountered: