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 Subprocesses in Destination dir #1127

Closed
wants to merge 1 commit into from
Closed

Conversation

ehlewis
Copy link
Contributor

@ehlewis ehlewis commented Dec 2, 2023

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

Summary of changes

Addressing #1091
Set venv creation and install_package subprocesses to run inside the destination directory (self.root) 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! ✨ 🌟 ✨

Repo tests were also passed successfully
https://github.com/ehlewis/pipx/actions/runs/7072389139

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Test please.

@gaborbernat gaborbernat marked this pull request as draft December 3, 2023 02:56
@gaborbernat
Copy link
Contributor

Seems stalled, we can reopen if you pick it up again.

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