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

pyquaternion: 0.9.6-3 in 'kinetic/distribution.yaml' [bloom] #23794

Merged
merged 3 commits into from Feb 24, 2020

Conversation

Achllle
Copy link
Contributor

@Achllle Achllle commented Feb 17, 2020

Increasing version of package(s) in repository pyquaternion to 0.9.6-3:

pyquaternion

Post ROS package conversion
* Change exec depend naming from numpy to python-numpy
* Add numpy dependency to package.xml
* Create catkin package, rename and move some files
* Fix casting error in trace_method
* Add setter for vector

@Achllle Achllle requested a review from tfoote as a code owner February 17, 2020 20:56
@Achllle
Copy link
Contributor Author

Achllle commented Feb 17, 2020

This package is a requirement for the dual-quaternions and dual-quaternions-ros packages, see also #23261

@Achllle Achllle changed the title pyquaternion: 0.9.6-2 in 'kinetic/distribution.yaml' [bloom] pyquaternion: 0.9.6-3 in 'kinetic/distribution.yaml' [bloom] Feb 18, 2020
pyquaternion:
doc:
type: git
url: https://github.com/Achllle/pyquaternion.git
Copy link
Contributor

Choose a reason for hiding this comment

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

So how does this relate to the pip key we have called python-pyquaternion-pip?

Copy link
Contributor Author

@Achllle Achllle Feb 19, 2020

Choose a reason for hiding this comment

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

Same package.
My reasoning was based on @tfoote 's comment on #23261 that pip was not the right way to release and follow-up comment that everything has to be in apt. I also don't like the default behavior of rosdep that it runs pip install as root, which doesn't play well with virtualenv. Finally, I am considering releasing a cpp version, along with the cpp version of dual_quaternions, although this is not a near-term thing.
If the pip package is preferred or if there only can be one, happy to withdraw the PR.

Copy link
Member

Choose a reason for hiding this comment

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

If we're going to package it here, we should just pull the pip version so that we don't get conflicts. Can you update this PR to remove the pip rosdep entry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. I do want to note for full transparency that along with the additions to turn the package into a ROS package, I made some minor changes that are not yet in the original package.

Copy link
Member

Choose a reason for hiding this comment

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

That should be fine. Just be careful doing that with too much. We've done that in the past and been bitten when the upstream didn't take the change and then we were stuck maintaining it in our fork. If it's mostly packaging changes those are more common, the API changes are the most at risk of being a maintenance burden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Achllle Achllle requested a review from a team as a code owner February 20, 2020 18:47
@clalancette clalancette merged commit e41ae04 into ros:master Feb 24, 2020
@PymZoR
Copy link

PymZoR commented Feb 24, 2020

This is a breaking change in rosdep as the key as been changed. That broke our CI, how can we avoid this in the future ?

@clalancette
Copy link
Contributor

This is a breaking change in rosdep as the key as been changed. That broke our CI, how can we avoid this in the future ?

Arg, sorry about that. I'm actually going to revert this for now, as I don't really know the way forward here.

FYI @tfoote @Achllle

@tfoote
Copy link
Member

tfoote commented Feb 25, 2020

With this change it is expected that the pip key stops resolving. The solution is to switch to the ROS package that was provided in it's place that will now be available as a ROS package. And this will allow other ROS packages to be released.

All global pip rules like this are going to cause potential regressions when we replace them with the preferred native packages or in this case ROS packages. Unfortunately we don't have any mechanism for communicating deprecation. This is part of why I'd like to discourage using the PIP rules in our main repositories they are subject to disappearing and being replaced when a better packaged solution arrives.

We should go ahead with this change and not revert it.

@Achllle please make sure to also release a version for melodic and noetic to get better coverage.
@PymZoR If you're not covered by the above list. We can add the pip rule back for other platforms but it should be nulled for any supported platform for kinetic and newer that would conflict with the released ROS package.

at-wat pushed a commit to alpine-ros/rosdistro that referenced this pull request Mar 12, 2020
* pyquaternion: 0.9.6-2 in 'kinetic/distribution.yaml' [bloom]

* Bump release version to 0.9.6.3

* Remove pyquaternion pip rosdep entry
at-wat pushed a commit to alpine-ros/rosdistro that referenced this pull request Mar 12, 2020
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.

None yet

4 participants