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

Support pyenv on Windows #94

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

nre-ableton
Copy link
Contributor

@nre-ableton nre-ableton commented Jun 29, 2022

This PR adds support for Windows in pyenv.createVirtualEnv() and virtualenv.create(). I've bumped the minor version since this is technically a breaking change, since these functions previously resulted in an error being thrown when run on Windows.

@nre-ableton nre-ableton requested a review from a team as a code owner June 29, 2022 15:53
@nre-ableton nre-ableton requested review from ala-ableton and removed request for a team June 29, 2022 15:53
src/com/ableton/VirtualEnv.groovy Outdated Show resolved Hide resolved
src/com/ableton/VirtualEnv.groovy Outdated Show resolved Hide resolved
src/com/ableton/VirtualEnv.groovy Outdated Show resolved Hide resolved
src/com/ableton/VirtualEnv.groovy Outdated Show resolved Hide resolved
src/com/ableton/VirtualEnv.groovy Outdated Show resolved Hide resolved
@nre-ableton
Copy link
Contributor Author

@ala-ableton my changes should squash cleanly. Let me know when I should squash.

Copy link
Contributor

@ala-ableton ala-ableton left a comment

Choose a reason for hiding this comment

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

Please fix 31ed1db and we're almost done.

Copy link
Contributor

@ala-ableton ala-ableton left a comment

Choose a reason for hiding this comment

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

In 43d3b35, we now have export PATH=... twice.

@nre-ableton
Copy link
Contributor Author

In 43d3b35, we now have export PATH=... twice.

Sorry about that. Fixed in b5b1c96.

Copy link
Contributor

@ala-ableton ala-ableton left a comment

Choose a reason for hiding this comment

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

Please rebase/squash, then re-rebase and edit 43d3b35 to make sure that export PYENV_ROOT=... keeps on coming before export PATH=..., instead of having export PATH=... move around.

src/com/ableton/VirtualEnv.groovy Outdated Show resolved Hide resolved
src/com/ableton/VirtualEnv.groovy Outdated Show resolved Hide resolved
@nre-ableton nre-ableton force-pushed the nre/master/2497-pyenv-windows branch 2 times, most recently from 732bd30 to 6381850 Compare July 28, 2022 08:44
@nre-ableton
Copy link
Contributor Author

So I've rebased this PR, and also added Windows support for the newly-added integration tests, but they keep timing out after 1 hour.

The first time, the build failed with a different error (not a timeout):

[2022-07-28T08:33:57.000Z] F:/jenkins-node/workspace@tmp/durable-6cb19c97/script.sh: line 4: /f/jenkins-node/workspace/.venv/venv-95957963/Scripts/activate: No such file or directory

I've no idea why this is happening. 😞

@nre-ableton nre-ableton force-pushed the nre/master/2497-pyenv-windows branch 5 times, most recently from 161f29f to 8c50355 Compare September 14, 2023 16:53
@nre-ableton nre-ableton marked this pull request as draft September 14, 2023 16:54
@nre-ableton
Copy link
Contributor Author

Ok! This PR is finally ready for review, since our CI nodes now have the updated version of Pyenv needed for Windows support.

src/com/ableton/Pyenv.groovy Outdated Show resolved Hide resolved
src/com/ableton/VirtualEnv.groovy Outdated Show resolved Hide resolved
src/com/ableton/Pyenv.groovy Outdated Show resolved Hide resolved
vars/pyenv.groovy Outdated Show resolved Hide resolved
src/com/ableton/Pyenv.groovy Show resolved Hide resolved
src/com/ableton/Pyenv.groovy Show resolved Hide resolved
@nre-ableton
Copy link
Contributor Author

@ala-ableton Please let me know when I should rebase.

@ala-ableton
Copy link
Contributor

@nre-ableton Feel free to rebase whenever you want.

@nre-ableton
Copy link
Contributor Author

@nre-ableton Feel free to rebase whenever you want.

Done. Feel free to review. 😊 Note that I pushed a dropme commit just to make sure that installing fresh (uncached) Python versions still works.

src/com/ableton/Pyenv.groovy Outdated Show resolved Hide resolved
src/com/ableton/Pyenv.groovy Outdated Show resolved Hide resolved
src/com/ableton/Pyenv.groovy Outdated Show resolved Hide resolved
In future commits, we'll need to modify the commands used depending on
the platform (and some other factors as well). Storing them in a list
will make this easier to manipulate.
This will facilitate Windows support, which will be added in a
future commit.
This is needed to support pyenv on Windows.
This will be needed to fully support Windows, which will happen in a
future commit.
This is needed to support pyenv on Windows, which will be added in a
future commit.
This will be needed to support pyenv on Windows, which will happen in a
subsequent commit.
To support Windows (which will be done in a subsequent commit), we'll
need to translate Windows-style paths to Cygwin-style paths (for
example, C:\foo\bar -> /c/foo/bar). The fileExists step only supports
Windows-style paths when run on Windows, which means that we need to
actually run pyenv to assert that it exists.
@nre-ableton
Copy link
Contributor Author

Forgive the rebase/force-push, I just noticed a vim-related typo in a commit message and decided to just drop the experiment commits regarding pyenv shell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants