Skip to content
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

changes between 1.11.20 and 1.12.7 for backporting #1008

Merged
merged 21 commits into from
Mar 6, 2017

Conversation

dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Mar 1, 2017

The following list of changes has been integrated into ros_comm 1.12.7 (Kinetic) since the last Indigo / Jade release (1.11.20).

Backported: (these changes are part of this PR)

Not backported:

@ros/ros_team @ros/ros_comm-maintainers Please comment on the decision which changes to (not) backport. Especially for the three backported changes marked with a "?" I wasn't sure about the regression risk.

jasonimercer and others added 18 commits March 1, 2017 14:59
rospy.Time -> rospy.Duration for period.
Don't rely on transitive header inclusion to declare std::vector as building with GCC-6 fails due to no '#Include <vector>' statement.
Handles issues with many simultaneous connections to XmlRPC Server in
OSX 10.11
Take the ._type using [0], because the arguments passed to migrate_raw
are tuples.

This was throwing another exception because of that bug: AttributeError,
saying that tuple has no attribute _type.
… confusing.

For example, [this warning msg](https://travis-ci.org/wg-perception/people/jobs/202019288#L3737) (in this [PR](wg-perception/people#49)):

 ```
 * WARN: unrecognized 'group' tag in <node> tag. Node xml is <test name="$(arg testnode_name)" pkg="rostest" test-name="hztest_test" type="hztest">
    <group unless="$(arg expected_success)">
      <node args="-r 0.5 $(find face_detector)/test/face_detector_noface_test_diamondback.bag" name="play" pkg="rosbag" type="play"/>
      <param name="hz" value="0.0"/>
    </group>
```

This is confusing because the tag in question is actually `<test>`, not `<node>`. Although the warning message refers to the exact <test> tag, I didn't try to read it because I was told the issue is with <node>, not <test>.

With this PR the message becomes a bit verbose but hope the readers get better idea.
Jannik Abbenseth and others added 3 commits March 2, 2017 11:21
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, I especially looked at the "low risk of regression?" labeled ones.

@dirk-thomas
Copy link
Member Author

@wjwwood Thank you for going through this lengthy list as well as all the other backport PRs. 🙇‍♂️

@gavanderhoorn
Copy link
Contributor

@dirk-thomas: I found the list extremely informative - not only because it provides an overview of bugs that were fixed, but also because it shows what is not backported. +10.

Do you intend to provide such as list for future backport PRs?

Would it be an idea to link to these PRs somewhere or post the list to a more visible location?

@dirk-thomas
Copy link
Member Author

Do you intend to provide such as list for future backport PRs?

Due to the amount of patches to be considered and the advantage of having the backports tested by CI, yes, I think I will do it the same way in the future in this repo.

Would it be an idea to link to these PRs somewhere or post the list to a more visible location?

Why not. What do you have in mind?

@tfoote
Copy link
Member

tfoote commented Jun 6, 2017

It would be great to queue #962 for backport. I have ros/geometry/pull/145 against geometry to use the new path. However since I haven't forked development I'm holding the PR until #962 is backported. Since the goal is for downstream packages to use the new path it would be great to let that happen sooner rather than waiting for indigo to EOL or development to fork.

@dirk-thomas
Copy link
Member Author

#962 is intentionally not backported as described above since it is not necessary for older distributions. So I don't think any of the rational had changed to justify backporting it.

@tfoote
Copy link
Member

tfoote commented Jun 6, 2017

Indeed #962 is "not necessary" for older distributions. It's also "not necessary" on any distribution. It's a style cleanup flagged by @jspricke as a potential conflict on Debian. And the goal of fixing this way was that downstream code should be easy to fix. The benefit of this change is not achieved until everyone has switched.

Looking at the comment stating that it should be removed in lunar @mightyCelu helpfully submitted ros/geometry#145 to update it, but I cannot merge it because the development is not forked for the new distro(s). And especially as geometry is now mostly a backwards compatibility place holder I don't plan on forking it since there's no new feature development going on. Not backporting this change would force forking the repo as soon as the old header locations are removed. I can see that it's the same situation for diagnostics. And if there is any other moderately stable package out there they will have the same issue.

Thus I'm asking that this be queued as it will be convenient for downstream developers, such as myself, as well as anyone else using the xmlrpcpp headers directly in a package not under active development but still being maintained. Alternatively tocking it can wait until the Indigo rosdistro is EOL, but that's quite a long way out still.

@gavanderhoorn
Copy link
Contributor

@dirk-thomas wrote:

Would it be an idea to link to these PRs somewhere or post the list to a more visible location?

Why not. What do you have in mind?

I only stumbled on this PR and the description of the backports by accident. Perhaps we could create some page on the wiki where these kind of PRs are listed chronologically?

I was first thinking of perhaps adding a link to the repo sync mails / posts that @tfoote sends out, but that would possibly be hard to correlate with a particular backport PR.

@mikepurvis
Copy link
Member

In terms of overall visibility/discoverability, I'm not sure that another wiki page to have to maintain would be any better than linking to a Github search, eg:

https://github.com/ros/ros_comm/pulls?q=backport%20in%3Atitle

@gavanderhoorn
Copy link
Contributor

I don't really care how or where, as long these backport PRs receive some more attention. Keeping up-to-speed with all the changes in (core) packages is quite involved, so having a single overview of backports like this would seem like a valuable resource, that seems to deserve a bit more exposure.

@mikepurvis
Copy link
Member

Is the concern primarily about breakages, or new features, or something else? These are intentionally selected on the basis of being low-risk changes/fixes that have been specifically requested by users of the LTS release.

@gavanderhoorn
Copy link
Contributor

gavanderhoorn commented Jun 14, 2017

I'm not too worried about breakages: the really important pkgs seem to have sufficient unit tests to catch regressions most of the time.

Keeping up with backports of new features is definitely nice, but knowing what has changed (even if it was supposed to be an orthogonal change that shouldn't have introduced any changes to existing behaviour) is invaluable to me when trying to debug something.

Also: being able to point users on ROS Answers to PRs that have either changed something that they were not aware of or introduced features they thought / assumed they could not use on their version of ROS would be another rationale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.