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

support version specific install for pip (and others) #694

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

k-okada
Copy link
Contributor

@k-okada k-okada commented Jul 18, 2019

We often have trouble on version problem of pip installed modules, and to avoid this it is very impotant to not to upgrade the python module version automatically (ofcourse do not use pip, is the best solution....)

This PR respect version_eq or other attribute in depend tag when running pip install

  <exec_depend version_lte="5.7">ipython-pip</exec_depend>
  <exec_depend version_eq="4.10">ipykernel-pip</exec_depend>  

@NikolausDemmel
Copy link
Contributor

I'm not maintaining rosdep, nor using it much these days, but just some thoughts from when we had discussed this in the past:

  • What happens if an incompatible version is already installed?
  • What happens if you have different version statements (conflicting or compatible) for the same pip package in multiple ROS packages?

You might argue to just leave all those decisions to pip, and that is probably a good idea if possible, but consider the following: I'm not 100% sure here, I believe rosdep tries to combine multiple packages to a single pip install command, if no other package managers appear in between in the list of resolutions. But what if they get split into separate invocations? So one case would be two different version requirements (compatible or not) for a pip package in the same call to pip install, and another case might be two different version requirements (compatible or not) for a pip package in two subsequent calls pip install. In the latter, might pip uninstall the package from the first invocation and make it silently incompatible?

Ideally, the intended behaviour in such cases would be documented and tested. But maybe it's also ok to keep it simple if it covers >95% of use cases. You should probably wait for some feedback on willingness/bandwidth to include this change from a maintainer.

@k-okada k-okada force-pushed the pip_version branch 3 times, most recently from 9415aee to 604affe Compare July 19, 2019 08:20
@codecov-io
Copy link

codecov-io commented Jul 20, 2019

Codecov Report

Merging #694 into master will increase coverage by 0.30%.
The diff coverage is 90.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #694      +/-   ##
==========================================
+ Coverage   74.85%   75.16%   +0.30%     
==========================================
  Files          41       41              
  Lines        3214     3277      +63     
==========================================
+ Hits         2406     2463      +57     
- Misses        808      814       +6     
Impacted Files Coverage Δ
src/rosdep2/catkin_support.py 46.55% <0.00%> (ø)
src/rosdep2/lookup.py 87.68% <85.71%> (-0.15%) ⬇️
src/rosdep2/platforms/pip.py 73.72% <91.48%> (+10.21%) ⬆️
src/rosdep2/rospkg_loader.py 93.97% <94.11%> (-0.23%) ⬇️
src/rosdep2/installers.py 78.27% <100.00%> (ø)
src/rosdep2/main.py 48.83% <100.00%> (ø)
src/rosdep2/platforms/osx.py 51.93% <100.00%> (ø)
src/rosdep2/platforms/source.py 90.55% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1784da...6d09a34. Read the comment docs.

@130s
Copy link

130s commented Jul 20, 2019

Just raising +1 flag for adding this capability to utilize package.xml being able to define version limitations. We've ended up runningpip manually for pkgs that need to be specific versions, which is obviously tedious and turned out to be prone to human errors. Concerns raised by @NikolausDemmel makes sense though.

@knorth55
Copy link

+1 for this PR, but I can understand @NikolausDemmel 's concern.
But there will be the EOL of python2 soon, so we should move forward for this discussion because some python packages are modified to run only on python3 but carelessly released on python2.
In that case, I want to use version information for rosdep, too.

k-okada added a commit to k-okada/jsk_recognition that referenced this pull request Dec 21, 2019
k-okada added a commit to k-okada/jsk_recognition that referenced this pull request Dec 22, 2019
k-okada added a commit to k-okada/jsk_recognition that referenced this pull request May 9, 2020
k-okada added a commit to k-okada/jsk_recognition that referenced this pull request May 10, 2020
k-okada added a commit to k-okada/jsk_recognition that referenced this pull request May 10, 2020
k-okada added a commit to k-okada/jsk_recognition that referenced this pull request May 10, 2020
@Timple
Copy link

Timple commented Feb 16, 2021

some python packages are modified to run only on python3 but carelessly released on python2.

In that case I think it would be okay to just pin the version in the rosdistro file because everybody on python2 will suffer from this.

@@ -278,8 +278,9 @@ def get_depends(self, rosdep_args):
"""
return [] # Default return empty list

def resolve(self, rosdep_args_dict):
def resolve(self, rosdep, rosdep_args_dict):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please move this argument to after the rosdep_args_dict and add a default value for it so it doesn't unnecessarily break other projects that rely on resolve?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@russkel thanks for comment, this makes much simpler, fixed the code

src/rosdep2/installers.py Outdated Show resolved Hide resolved
@@ -433,13 +438,13 @@ def resolve_all(self, resources, installer_context, implicit=False):

return resolutions_flat, errors

def resolve(self, rosdep_key, resource_name, installer_context):
def resolve(self, rosdep, resource_name, installer_context):
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not new argument, just to rename variable name, since we have changed from list of keys to catkin_pkg.package.Dependency object
:param rosdep_key: rosdep key to resolve :param rosdeps: rosdep key (catkin_pkg.package.Dependency object) to resolve

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep understood.

@russkel
Copy link
Contributor

russkel commented Jun 15, 2022

Alternatively, could you store the Dependency object in the rosdep_args_dict instead of changing the arguments of the resolve method?

Edit: disregard above comment. I investigated it and it wasn't that simple.

@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2022

Codecov Report

Merging #694 (6d09a34) into master (b2ee64f) will increase coverage by 0.33%.
The diff coverage is 90.32%.

❗ Current head 6d09a34 differs from pull request most recent head 4749394. Consider uploading reports for the commit 4749394 to get more accurate results

@@            Coverage Diff             @@
##           master     #694      +/-   ##
==========================================
+ Coverage   74.82%   75.16%   +0.33%     
==========================================
  Files          44       41       -3     
  Lines        3360     3277      -83     
==========================================
- Hits         2514     2463      -51     
+ Misses        846      814      -32     
Impacted Files Coverage Δ
src/rosdep2/catkin_support.py 46.55% <0.00%> (ø)
src/rosdep2/lookup.py 87.68% <85.71%> (-0.50%) ⬇️
src/rosdep2/platforms/pip.py 73.72% <91.48%> (+9.72%) ⬆️
src/rosdep2/rospkg_loader.py 93.97% <94.11%> (-0.47%) ⬇️
src/rosdep2/installers.py 78.27% <100.00%> (ø)
src/rosdep2/main.py 48.83% <100.00%> (+0.22%) ⬆️
src/rosdep2/platforms/osx.py 51.93% <100.00%> (ø)
src/rosdep2/platforms/source.py 90.55% <100.00%> (+0.10%) ⬆️
src/rosdep2/platforms/gem.py 78.37% <0.00%> (-6.24%) ⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2ee64f...4749394. Read the comment docs.

@@ -343,26 +343,26 @@ def test_RosdepLookup_get_rosdeps():
pass

print(lookup.get_rosdeps('stack1_p1'))
assert set([d.name for d in lookup.get_rosdeps('stack1_p1')]) == set(['stack1_dep1', 'stack1_p1_dep1', 'stack1_p1_dep2'])
assert set([d.name for d in lookup.get_rosdeps('stack1_p1', implicit=False)]) == set(['stack1_dep1', 'stack1_p1_dep1', 'stack1_p1_dep2'])
assert set({d.name for d in lookup.get_rosdeps('stack1_p1')}) == set(['stack1_dep1', 'stack1_p1_dep1', 'stack1_p1_dep2'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Set comprehension doesn't need the outer set()

@JWhitleyWork
Copy link

@k-okada Would you mind taking a look at my new draft #901 which covers a small portion of what you've done here but on the install-verification side? This at least allows you to specify pip entries in YAML files which have a version requirement and verify that they were installed with that specific version.

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.

9 participants