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

Check for pip configuration when an externally managed environment is detected. #979

Merged
merged 24 commits into from
Aug 15, 2024

Conversation

nuclearsandwich
Copy link
Contributor

@nuclearsandwich nuclearsandwich commented Aug 8, 2024

rosdep is designed to treat pip like an alternative system-level package manager. Deviating from this approach is not easily achievable without a significant rethinking of how pip packages are managed. In the meantime, we can at least instruct users how to restore the prior functionality.

Rather than inject the environment variable / config on behalf of the user, this change instructs them to make the necessary config changes themself, keeping them informed of the change their making to the system's new default.

I added a documentation page specific to this issue with additional configuration details and recipes for CI.

Seeks to reduce friction related to #978

rosdep is designed to treat pip like an alternative system-level package
manager. Deviating from this approach is not easily achievable without a
significant rethinking of how pip packages are managed. In the meantime,
we can at least instruct users how to restore the prior functionality.

Rather than inject the environment variable / config on behalf of the
user, this change instructs them to make the necessary config changes
themself, keeping them informed of the change their making to the
system's new default.
@nuclearsandwich
Copy link
Contributor Author

In addition to the test failures, this actually doesn't work as intended because sudo is stripping the environment. I'm hesitant to recommend the pip global config file as a default because the default location is variable. Alternatively, we could override the sudo_command for the pip installer to pass that environment variable through.

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/rosdep-for-pip-is-broken-on-jazzy/38981/5

@nuclearsandwich
Copy link
Contributor Author

In addition to the test failures, this actually doesn't work as intended because sudo is stripping the environment. I'm hesitant to recommend the pip global config file as a default because the default location is variable. Alternatively, we could override the sudo_command for the pip installer to pass that environment variable through.

As of 2c76686 the installer is configured to pass PIP_BREAK_SYSTEM_PACKAGES through from the user environment. This works by default on each of my tested sudo environments (Archlinux, Ubuntu, Fedora) but a locked down sudo that disallows environment preservation or has an explicit allowlist will still create problems. My hope was to detect that the check passed but the install failed due to the externally managed target anyway and give a hint about sudo to suggest an alternative configuration method. But without changes, the pip installer never gets control back from install_resolved() so we'd have to add pip-specific hooks elsewhere in the code and I'm hesitant. I think I'm content to leave it here but with more robust documentation.

@nuclearsandwich nuclearsandwich changed the title Raise an error when an externally managed environment is detected. Check for pip configuration when an externally managed environment is detected. Aug 9, 2024
PIP_BREAK_SYSTEM_PACKAGES=1
in your environment.

For more information refer to http://docs.ros.org/en/independent/api/rosdep/html/pip_and_pep_668.html
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this documentation won't actually be live until the PR is merged and docs rebuild the next day. This will only affect those using rosdep from mainline or this branch. I'll wait until the docs rebuild to make a new rosdep release with this change.

Copy link
Member

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

Minor comments!

doc/pip_and_pep_668.rst Outdated Show resolved Hide resolved
doc/pip_and_pep_668.rst Outdated Show resolved Hide resolved
doc/pip_and_pep_668.rst Outdated Show resolved Hide resolved
doc/pip_and_pep_668.rst Outdated Show resolved Hide resolved
src/rosdep2/platforms/pip.py Outdated Show resolved Hide resolved
src/rosdep2/platforms/pip.py Outdated Show resolved Hide resolved
src/rosdep2/platforms/pip.py Outdated Show resolved Hide resolved
test/test_rosdep_pip.py Show resolved Hide resolved
Use monospace formatting when referring to the `rosdep` command /
executable name and simply 'rosdep' when referring to the project.

The preferred capitalization of rosdep is rosdep not Rosdep or ROSdep
(and certainly not ROSDep).
nuclearsandwich and others added 7 commits August 9, 2024 16:19
Co-authored-by: Christophe Bedard <[email protected]>
Co-authored-by: Christophe Bedard <[email protected]>
Co-authored-by: Christophe Bedard <[email protected]>
This is a fixup after two earlier changes inverted the conditional and
reformatted an internal check.
Comment on lines +72 to +73
from rosdep2 import InstallFailed
from rosdep2.platforms.pip import EXTERNALLY_MANAGED_EXPLAINER, PipInstaller
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not super up on why imports are done per test function here but I am propagating the convention until I understand it better.

Comment on lines +77 to +81
try:
installer.get_install_command(['whatever'])
assert False, 'should have raised'
except InstallFailed as e:
assert e.failures == [('pip', EXTERNALLY_MANAGED_EXPLAINER)]
Copy link
Member

Choose a reason for hiding this comment

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

If you want, a more Pythonic way could be:

Suggested change
try:
installer.get_install_command(['whatever'])
assert False, 'should have raised'
except InstallFailed as e:
assert e.failures == [('pip', EXTERNALLY_MANAGED_EXPLAINER)]
with pytest.raises(InstallFailed) as exception_info:
installer.get_install_command(['whatever'])
assert exception_info.value.failures == [('pip', EXTERNALLY_MANAGED_EXPLAINER)]

and make sure to import pytest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I'm actually going to keep this as is for now, because I don't believe the infrastructure packages ever explicitly require pytest. I think pytest is supposed to be compatible with stdlib unittest as well and utilizing that may be the preferred approach.

Copy link
Member

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

Aside from my last minor suggestions, this looks good to me!

Using the dict access method will raise a KeyError when the config file
is present but this section or value is missing.
@nuclearsandwich nuclearsandwich merged commit 55f5b39 into master Aug 15, 2024
13 checks passed
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.

3 participants