Skip to content
This repository has been archived by the owner on Aug 16, 2022. It is now read-only.

fix(setup): remove sudo for pip #306

Merged
merged 3 commits into from
May 18, 2022
Merged

fix(setup): remove sudo for pip #306

merged 3 commits into from
May 18, 2022

Conversation

kenji-miyake
Copy link
Contributor

@kenji-miyake kenji-miyake commented May 17, 2022

Description

We've been using sudo for pip because, as written in ~/.profile, $HOME/.local/bin isn't in the $PATH right after OS installation.

# set PATH so it includes user's private bin if it exists
if [ -d "$HOME/.local/bin" ] ; then
    PATH="$HOME/.local/bin:$PATH"
fi

However, using sudo for pip isn't recommended.

$ sudo pip install clang-format
[sudo] password for kenji: 
Requirement already satisfied: clang-format in /usr/local/lib/python3.10/dist-packages (14.0.1)
WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv

As I thought sudo can be removed if I add export PATH="$HOME/.local/bin:$PATH" in the setup script, I changed so.

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

Kenji Miyake added 3 commits May 17, 2022 21:21
@kenji-miyake
Copy link
Contributor Author

Since I haven't tested this well, I'll mark this as a draft.

@esteve
Copy link
Contributor

esteve commented May 17, 2022

@kenji-miyake alternatively, what do you think of virtualenv? virtualenvwrapper lets you have multiple versions of Python workspaces and are stored in a central place (https://virtualenvwrapper.readthedocs.io/en/latest/)

@kenji-miyake
Copy link
Contributor Author

@esteve I think it's good! But there are some problems:

  • I'm not familiar with it.
  • I'm not sure if it works well with ROS 2.

Do you know about it?

@esteve
Copy link
Contributor

esteve commented May 17, 2022

@kenji-miyake yeah, we used virtualenv quite a lot during the development of ROS2 at Open Robotics. It's in the documentation as one of the options for working with Python packages with ROS2 https://docs.ros.org/en/galactic/How-To-Guides/Using-Python-Packages.html#installing-via-a-virtual-environment

@kenji-miyake
Copy link
Contributor Author

@esteve Thank you for the link. 🙏

Seeing this, after the user run source ./venv/bin/activate, it seems the same as the normal user mode of pip.

If so, I guess this PR also works with virtualenv. Am I right? 🤔

@esteve
Copy link
Contributor

esteve commented May 17, 2022

@kenji-miyake yes, it should work the same. But this was just to ask you if you think it's a good idea, perhaps we don't need to add this to the development process. One nice thing is that any dependencies we install are in the virtualenv, so if we want to remove all the dependencies with installed, it's as easy as removing the virualenv

@kenji-miyake
Copy link
Contributor Author

@esteve I see. 👍
Do you think it's good to merge this now (even though it may break something) and will observe it for a while? (And will fix bugs if issues are reported.)
If it's okay, I'll make this ready for review.

Currently, it's not good because it obviously won't work with virtualenv.

@esteve
Copy link
Contributor

esteve commented May 17, 2022

@kenji-miyake I think it should be ok to merge this 👍

@kenji-miyake
Copy link
Contributor Author

Okay, then I'll set reviewers from TIER IV who might be familiar with this issue!

@kenji-miyake kenji-miyake marked this pull request as ready for review May 17, 2022 12:52
Copy link
Contributor

@yukkysaito yukkysaito left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@wep21 wep21 left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

None yet

4 participants