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

Run venv Creation in Isolated Mode (#1091) #1115

Closed
wants to merge 3 commits into from
Closed

Run venv Creation in Isolated Mode (#1091) #1115

wants to merge 3 commits into from

Conversation

ehlewis
Copy link
Contributor

@ehlewis ehlewis commented Dec 1, 2023

  • I have added an entry to docs/changelog.md

Summary of changes

Addressing #1091
Created venv using isolated mode by passing the -I flag in order to resolve a bug where python files in the cwd could break the venv creation.
Also a slight security improvement since by ignoring the python files in the cwd which could potentially override files on the sys.path

Test plan

Manually tested resolution of bug by creating a file in cwd
echo "blah blah" > logging.py
then running the current release of pipx
pipx install frogmouth
we get the expected error

File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/venv/__init__.py", line 7, in <module>
  import logging
  File "/Users/ehlewis/test/logging.py", line 1
    blah blah
         ^^^^
SyntaxError: invalid syntax

'/Library/Frameworks/Python.framework/Versions/3.12/bin/python3.12 -m venv --without-pip
/Users/ehlewis/.local/pipx/venvs/frogmouth' failed

running the same command with the modified create_venv shows a successful install and the installed package functions as expected

python3 src/pipx install frogmouth                                                 
  installed package frogmouth 0.9.2, installed using Python 3.12.0
  These apps are now globally available
    - frogmouth
done! ✨ 🌟 ✨

@uranusjr
Copy link
Member

uranusjr commented Dec 1, 2023

The isolated mode turns off a lot more than just sys.path. I’m not strictly against this (it’s fundamentally a good idea), but we need to communicate the change very carefully with users, including a major release and visible warnings to help migration.

@ehlewis
Copy link
Contributor Author

ehlewis commented Dec 1, 2023

Going to spend some time to resolve test issues and return to this

@ehlewis ehlewis closed this Dec 1, 2023
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