Skip to content

Don't consider venv as externally managed#999

Merged
nuclearsandwich merged 2 commits into
ros-infrastructure:masterfrom
krisl:venv-not-externally-managed-env
Aug 20, 2025
Merged

Don't consider venv as externally managed#999
nuclearsandwich merged 2 commits into
ros-infrastructure:masterfrom
krisl:venv-not-externally-managed-env

Conversation

@krisl

@krisl krisl commented Apr 26, 2025

Copy link
Copy Markdown
Contributor

Allow installation into virtual environments without requiring --break-system-packages

@cottsay cottsay requested a review from nuclearsandwich April 26, 2025 23:47
@krisl krisl force-pushed the venv-not-externally-managed-env branch from 89ec6fa to ddd5ba0 Compare May 18, 2025 08:21

@nuclearsandwich nuclearsandwich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR. Since you contributed a test for this function in #1000 is it worth rebasing now that it has merged and covering the virtualenv case in the test?



def in_virtual_environment():
return sys.prefix != sys.base_prefix

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you know if this is a sufficiently generic way to detect virtual environments including those created by other tools like pipenv, uv, etc?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great question! I believe so, its from this section docs.python.org

I've used this with venv and uv

@krisl krisl force-pushed the venv-not-externally-managed-env branch from ddd5ba0 to 5ca57d2 Compare July 17, 2025 06:32
@codecov

codecov Bot commented Jul 17, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.10%. Comparing base (ad3a610) to head (5ca57d2).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #999      +/-   ##
==========================================
+ Coverage   71.76%   72.10%   +0.34%     
==========================================
  Files          44       44              
  Lines        3488     3492       +4     
  Branches      686      687       +1     
==========================================
+ Hits         2503     2518      +15     
+ Misses        808      796      -12     
- Partials      177      178       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@krisl

krisl commented Jul 17, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for the PR. Since you contributed a test for this function in #1000 is it worth rebasing now that it has merged and covering the virtualenv case in the test?

cool, I rebased and added a test in there

@nuclearsandwich nuclearsandwich merged commit d691d68 into ros-infrastructure:master Aug 20, 2025
14 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.

2 participants