buildinspace / peru

a generic package manager, for including other people's code in your projects
MIT License
1.11k stars 69 forks source link

add the PERU_REEXEC_PYTHON heuristic on Windows #211

Closed oconnor663 closed 3 years ago

oconnor663 commented 3 years ago

Normally we prefer to execute plugins directly. This is pretty reliable on Unix, where scripts can specify their interpreter with a shebang line. However, it's not as reliable on Windows, because the extension-interpreter mapping is a global config. In my experience, installing and uninstalling a series of different Python interepreter versions on a Windows machine can break that association, and as a result break peru, in confusing ways. (Tests have also been broken by default for this reason on every Windows CI provider I've ever tried.)

To work around this problem, we apply an extra heuristic on Windows: If a plugin executable filename ends in .py, assume that we should re-execute the current interpreter (sys.executable) to run that file, rather than relying on the system shell to find the interpreter. This fixes peru on systems with broken Python configs, and it makes no difference in the vast majority of other cases.

For users who want to control this heuristic (either to disable it, or to force the same behavior on Unix), we define the PERU_REEXEC_PYTHON env var.

We discussed the previous behavior six years ago as part of be35c14f9986f5481fc6c0e6aff04f08f401309c. At the time, I didn't want to special-case Python plugins over any other language. Since then, repeated problems on Windows have worn me down, particularly related to CI. With the recent switch to GitHub Actions, I was looking at solving this config issue yet again for a new platform, and I just decided I'd rather have a permanent fix.

oconnor663 commented 3 years ago

My first version of this PR forgot to actually check for the .py extension :p It passed tests because we suppress the rsync plugin on Windows. (It's a bash script.)

Any suggestions for tests that we could add here?

olson-sean-k commented 3 years ago

Any suggestions for tests that we could add here?

If we include a non-Python plugin for Windows, then CI would catch this. I suspect that we'd encounter the same issue though: whatever toolchain that plugin uses, it may not execute reliably across various Windows installations. Maybe a VBScript plugin could work. 😝 (Seriously though, VBScript seems to Just Work™ from the shell and desktop even on Windows 10.)

oconnor663 commented 3 years ago

Yeah or maybe just a .bat file plugin. We already disable the tests that depend on rsync when we run tests on Windows, so it wouldn't be any trouble to write a new test that's disabled in the other direction. I'll add something.

oconnor663 commented 3 years ago

Ok, I rebased the new test commit to come first, so you can test it both with and without the new heuristic. Both pass for me locally. And I fixed my mistake with list2cmdline, where I (ironically) forgot that its argument should be a list.

oconnor663 commented 3 years ago

🚢