search

Jayich-Lab / tray-launcher

A launcher for Windows that resides in the taskbar for managing .bat scripts.
MIT License
7 stars 1 forks source link

fix venv bug #70

Closed fanmingyu212 closed 2 years ago

fanmingyu212 commented 2 years ago

With venv on Windows (python 3.10), I cannot run tray-launcher. It turns out that python from subprocess.Popen uses the system python instead of the venv python. I still do not fully understand why, but a patch is attached with also explanation in the README about possible venv issues.

Also fixes a few lint issues and ignores W503 (operators at the start of a line). I am not sure why these issues appear here.

@danhuaxu besides reviewing, can you install this version of tray-launcher on a computer and make sure that it still works with conda? I think it should be fine but just want to make sure.

danhuaxu commented 2 years ago

Will do.

Codes look good to me. You may want to change the Popen line in gui.py as well; I can do that after testing tomorrow.

fanmingyu212 commented 2 years ago

@danhuaxu I delibrately did not change the Popen in child_script.py. I want to leave the choice to the user whether to run the script in the venv. The user can add the activate command in their scripts. Therefore the note in README.

danhuaxu commented 2 years ago

That makes sense. I just tried to run this version in a conda environment and it worked well. Thank you for fixing this bug!

gschaffner commented 2 years ago

Why not just replace "python" -> sys.executable? This will always use the same interpreter as the parent process no matter whether the user is using venv, virtualenv, or conda env. This seems simpler and more robust than special-casing $VIRTUAL_ENV. The current code assumes that the user's shell is set to cmd.exe and uses an activate script for cmd.exe. Is this a safe assumption? Can Windows users change their default shell to e.g. PowerShell, which requires a different activate script?

Somewhat related to this: I am curious why we spawn a shell instead of spawning just the process itself, even prior to this PR.

fanmingyu212 commented 2 years ago

@gschaffner using sys.executable makes sense. Running the command instead of spawning a shell also makes sense...

Although it is no longer related if we change it to the command only, cmd.exe is a safe assumption for a Windows shell as it is shipped with all current and recent shells and it is compatible with all python environments that I know of. PowerShell is not compatible with conda, for example.