Release for bionic doesn't include #42

dhood commented 6 years ago

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 the strToDouble 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 https://github.com/ros-visualization/rviz/issues/1249, https://github.com/ros-visualization/rviz/issues/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?

dhood commented 6 years ago

@j-rivero @clalancette Was a new release of urdfdom_headers considered for bionic? Would it be too late now?

j-rivero commented 6 years ago

@j-rivero @clalancette Was a new release of urdfdom_headers considered for bionic? Would it be too late now?

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.

dhood commented 6 years ago

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.

clalancette commented 6 years ago

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.

j-rivero commented 6 years ago

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,

gavanderhoorn commented 6 years ago

I know we're supposed to use Github's +1 feature for this, but going to use a comment to give it extra visiblity: it would be very much appreciated if a patch release could be done that includes #42. I'm in one of those countries where we use commas instead of dots, and although I'm not affected myself (using UTF-8), the nr of times we run into this now makes me come here and ask if something could be done, please :)

simonschmeisser commented 6 years ago

@clalancette are you working on this or anybody else?

clalancette commented 6 years ago

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.

simonschmeisser commented 6 years ago

@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

clalancette commented 6 years ago

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

ldayot commented 6 years ago

RViz doesn't display surfaces when LC_NUMERIC has no value. Solved executing export LC_NUMERIC="en_US.UTF-8"

Source: RViz does not show robot appearance (RViz tutorials: building from scratch) on answers.ros.org

simonschmeisser commented 5 years ago

@scpeters ping?

scpeters commented 5 years ago

Here's a draft of the release notes for a 1.1.0 release:

https://github.com/ros/urdfdom_headers/releases/tag/untagged-1fc2b13e629cb2aa7873

simonschmeisser commented 5 years ago

Thanks for getting to this so quickly after soccer ;) The link doesn't work for me

scpeters commented 5 years ago

sorry I didn't realize that was a secret link

scpeters commented 5 years ago

I just made it as a patch release instead of minor:

https://github.com/ros/urdfdom_headers/releases/tag/1.0.1

I'll ask @j-rivero to cherry-pick e7e0972a4617b8a5df7a274ea3ba3b92e3895a35 into debian and Ubuntu

simonschmeisser commented 5 years ago

@j-rivero any progress?

j-rivero commented 5 years ago

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

simonschmeisser commented 5 years ago

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

j-rivero commented 5 years ago

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.

simonschmeisser commented 5 years ago

looking at https://github.com/ros/urdfdom_headers/commit/9d2b421f3fcbc2a32af40b99ebd9c2cb2d088fb9 there is another issue with float parsing (see #46 )

I will open a PR and then be can start the whole game anew, sorry for not looking closer in the first place

j-rivero commented 5 years ago

I will open a PR and then be can start the whole game anew, sorry for not looking closer in the first place

No problem. We can make a new patch release as soon as the PR is merged and import it to debian.

gavanderhoorn commented 5 years ago

Just realised: #42 is the answer to everything.

simonschmeisser commented 5 years ago

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

traversaro commented 5 years ago

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.

peci1 commented 5 years ago

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 LC_NUMERIC=C.

peci1 commented 4 years ago

Today, the updated urdfdom_headers package has been released to bionic. It includes the fix for #42.

peci1 commented 4 years ago

@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 commented 4 years ago

Today, the updated urdfdom_headers package has been released to bionic. It includes the fix for #42.

How did you determine this @peci1 ?

kyrofa commented 4 years ago

@gavanderhoorn probably from the SRU bug. We released the fix into bionic-updates on the 7th thanks to @j-rivero's hard work.

gavanderhoorn commented 4 years ago

Ok, thanks.

Obviously :100: :+1: :tada: :birthday: :beers: :hamburger: :smile: etc for getting this released, but :frowning_face: :cry: :cloud_with_lightning_and_rain: that it took over 1.5 years.

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.

kyrofa commented 4 years ago

:frowning_face: :cry: :cloud_with_lightning_and_rain: that it took over 1.5 years.

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.

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.

I have a few thoughts to share, if I may.

SRUs are a foreign concept

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

There's no real way to know that process exists unless you've already done it
If you're not an Ubuntu developer I doubt you're familiar with the terminology used there
Despite the process being well defined in theory, it's a bit of a mess in practice and it mostly turns into a game of who you know and how squeaky of a wheel you are (in this case, you know me, and I'm willing to try and help where I can. I have no power but I know other people :wink: )

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 first

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

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

simonschmeisser commented 4 years ago

I'm really glad this journey is over, what an endurance test ...

The bug didn't follow SRU guidelines at first

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! :wink:

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:

you upload a new version to Debian mentors and ask for someone to sponsor it
someone sponsors it, which means it gets uploaded to Debian unstable
after about a week it will automagically get promoted to Debian testing unless it breaks stuff
Ubuntu automatically syncs testing until a certain date
Once that date has passed you need to do the mess observable above to fix anything

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 :disappointed: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=943675 )

kyrofa commented 4 years ago

there seems to be no "default assignee" or "caretaker" looking at and promoting SRUs that go unnoticed

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.

try to align better with Debian and especially Ubuntu release schedule

Yes, that would allow you to skip the SRU process altogether, which I wholeheartedly recommend!

peci1 commented 4 years ago

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.

gavanderhoorn commented 4 years ago

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.

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

gavanderhoorn commented 4 years ago

What we as a comunity (this includes @clalancette and people at OSRF) should do in future is to either stop upstreaming stuff [..]

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.

j-rivero commented 4 years ago

To close this and follow up with the offtopic discussion, I would to do/propose to things:

Thanks everyone in this bug for the work done to solve issues and the information you provided. I know it was extremely slow but Kyle already pointed some of the problems and before that we had other coordination problems among those of us working on the patch. It's done, let's learn from it.
I'll open a topic in discourse with the reasons to upstream the important bug fixes so we can discuss there if we want to change the procedure.

gavanderhoorn commented 4 years ago

2\. I'll open a topic in discourse with the reasons to upstream the important bug fixes so we can discuss there if we want to change the procedure.

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.

clalancette commented 4 years ago

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.

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?

ros / urdfdom_headers