Pull request changed default behaviour

[slivingston]

Pull request #2 does indeed change default behaviour. In summary, it is possible to declare a machine to be the default in a roslaunch file, which propagates through included roslaunch files. The change merged in by pull request #2 overrides this default, requiring the user to pass in an argument named "machine" to use anything other than localhost.

As an example of how this breaks default behaviour, I have a roslaunch file that includes 3dsensor.launch from the turtlebot_bringup package. This roslaunch file is invoked on a machine that is not a turtlebot, but by making use of the default attribute for the machine tag, I am able to have everything created by 3dsensor.launch happen on a turtlebot that is accessible on my local network. My code worked in ROS Groovy, and it should work in ROS Hydro, unchanged. However, the nodelet manager is invoked on my local machine, rather than the desired turtlebot. When I manually undo the changes from that merge, my roslaunch file completes as expected.

Could you roll back the changes caused by pull request #2?

[piyushk]

You're correct that #2 changes default behavior. I was unaware the machine tag had a default attribute.

I'll merge this change in Indigo.

[piyushk]

@slivingston I'm preparing the Indigo release right now, but I'm slightly wary of reverting this change. While the initial merge broke code for using the machine tag while moving from groovy->hydro, reverting this change might cause a similar issue for hydro->indigo.

If you can achieve the functionality you had in Groovy through the Hydro launch file by setting the machine argument, I would prefer to leave that change in place. Please let me know in case you cannot achieve the same functionality.

[slivingston]

I cannot achieve the same functionality. You should have considered the changeset introduced by that pull request as a bug and fixed it accordingly.

To be sure that I understand the situation: there is the bug described by this issue, and since it was not fixed for hydro, you are afraid that it has become regarded as the (new) default behavior and thus do not want to fix it for indigo.

I originally focused on how the pull request changed default behavior because you used that as justification for merging: "As far as I can tell, this does not change the default behaviour. I'll merge it in." quote from #2. Aside from the usefulness of preserving previous behavior (i.e., backwards compatibility), it is possible to launch on a remote machine without the machine parameter introduced in #2, i.e., default behavior already supported the use-case that #2 was supposed to provide. In other words, merging #2 restricted what is possible without providing something new (aside from an alternative method for something already possible), and therefore that changeset introduced a bug.

[piyushk]

@slivingston You've understood my point correctly. And you're absolutely right that my analysis at the time of merging the PR was flawed.

However the change was made when this package was first released, and around the time of hydro feature freeze. Since the Groovy -> Hydro update does not necessarily need to maintain backwards compatibility, as well as the fact that rgbd_launch was released with major version revision, introducing that change is not a bug. It's just annoying, and I hate breaking backwards compatibility.

#2 provides additional functionality that was not possible before. For instance, you can now launch openni_launch on multiple remote machine in the same launch file, which was not possible before #2 was merged. Additionally, it is still possible to achieve what you want, albeit by the slightly cumbersome way of supplying the machine argument.

[piyushk]

As per http://wiki.ros.org/roslaunch/XML/machine, the default settings only applies to the same scope, so my argument for additional functionality is not valid.

I'm fine reverting the change for Indigo.

[jack-oquin]

What about Hydro? Shouldn't it be consistent with Groovy and Indigo?

[piyushk]

It shouldn't be released into Hydro as it is not a necessary bug fix.
On Apr 30, 2014 1:07 PM, "jack-oquin" notifications@github.com wrote:

What about Hydro? Shouldn't it be consistent with Groovy and Indigo?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/5#issuecomment-41829202
.

[jack-oquin]

The original report was as a bug in Hydro.

[piyushk]

I don't consider it as a bug in Hydro. Its just a poor way to achieve the functionality that was requested in #2. As I mentioned before, #2 was merged into rgbd_launch around the time of the Hydro freeze (might have been a bit after it), and became part of the API for Hydro. I don't intend on releasing a change that breaks the API for anyone using a released Hydro deb.