urdfdom_headers
urdfdom_headers copied to clipboard
ros
Release for bionic doesn't include #42
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?
@j-rivero @clalancette Was a new release of urdfdom_headers considered for bionic? Would it be too late now?
@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.
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.
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 +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 :)
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. 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
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
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
@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?
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.
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
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.
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 LC_NUMERIC=C.
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).
Today, the updated urdfdom_headers package has been released to bionic. It includes the fix for #42.
How did you determine this @peci1 ?
@gavanderhoorn probably from the SRU bug. We released the fix into bionic-updates on the 7th thanks to @j-rivero's hard work.
