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

use pip install --user when --as_root pip:false, rather than pip install #693

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

Conversation

k-okada
Copy link
Contributor

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

Current implementation runs pip install command when --as-root pip:false is set.
In most cases, it does not work because it try to install the pip package under /usr/local without sudo command
So when we set --as-root, I think we should use pip install --user command.

@codecov-io
Copy link

codecov-io commented Jul 18, 2019

Codecov Report

Merging #693 into master will increase coverage by 0.16%.
The diff coverage is 96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #693      +/-   ##
==========================================
+ Coverage   75.47%   75.63%   +0.16%     
==========================================
  Files          32       32              
  Lines        2968     2996      +28     
==========================================
+ Hits         2240     2266      +26     
- Misses        728      730       +2
Impacted Files Coverage Δ
src/rosdep2/platforms/pip.py 67.24% <100%> (+2.42%) ⬆️
src/rosdep2/installers.py 78.36% <100%> (+0.08%) ⬆️
src/rosdep2/main.py 50.36% <95.55%> (+1.77%) ⬆️

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 ab1f06e...87f90ea. Read the comment docs.

@NikolausDemmel
Copy link
Contributor

The original use case of adding this was on macos, where the system-wide install in /usr/local for brewed python shouldn't be done with root. So I don't think we should change the default.

Maybe instead some syntax like --installer-args "pip:--user" could be used.

@k-okada
Copy link
Contributor Author

k-okada commented Jul 18, 2019 via email

@NikolausDemmel
Copy link
Contributor

Yes, exactly, it means the python installed with homebrew (as opposed to the one shipped with macos). You can still do pip install --user, and maybe that is even a better idea if you are the only user for the ROS setup on that machine. I personally don't mind the change too much, but maybe some users rely on the current behaviour.

@NikolausDemmel
Copy link
Contributor

Oh look, there was already discussion on supporting --user back when --as-root was added, but nothing came of it in the end: #307 (comment)

@k-okada
Copy link
Contributor Author

k-okada commented Jul 23, 2019

@NikolausDemmel thanks for comment, I have implemented —installer-options and add rosdep/config.yaml to support —user

@nuclearsandwich
Copy link
Contributor

@k-okada in addition to needing a rebase there are quite a few changes here which I think could be split out. I'm especially interested in the optparse -> argparse commits and I think the introduction of a config file to persist options will come with its own discussion and would benefit from a breakout PR as well. Would you be willing to open either or both of those?

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

Successfully merging this pull request may close these issues.

4 participants