snok / install-poetry

Github action for installing and configuring Poetry
MIT License
567 stars 53 forks source link

$VENV not set on Poetry installation cache restore #138

Closed emanueljg closed 5 days ago

emanueljg commented 10 months ago

When restoring a Poetry installation from cache, it doesn't seem $VENV gets set.

Steps to reproduce:

  1. Implement https://github.com/snok/install-poetry#caching-the-poetry-installation
  2. use source $VENV in a later step
  3. flow fails due to unset var

...which I guess makes sense since it's the action that sets $VENV, and since the action won't run on a cache hit, the var remains unset. This deserves a mention in the documentation.

sondrelg commented 10 months ago

Yeah this is reasonable. Would you like to contribute a doc update? 🙂

emanueljg commented 10 months ago

Yeah this is reasonable. Would you like to contribute a doc update? 🙂

Sure! But now that I think about it - couldn't we deprecate the use of $VENV altogether?

Why not just tell the user to add this to their matrix instead?

    strategy:
      matrix:
        os: [ubuntu-latest, windows-latest]
        include:
          - VENV: .venv/bin/activate
          - os: windows-latest
            VENV: .venv/scripts/activate

(https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#expanding-or-adding-matrix-configurations)

It potentially breaks on a more exotic matrix configuration (i.e. a windows runner not identified by windows-latest) but that's easily spotted by the user.

sondrelg commented 10 months ago

I'd be open to removing $VENV from the docs, and adding this instead, but I'd prefer not to break existing users 👍

dragondive commented 3 weeks ago

@sondrelg The VENV doesn't seem to be working anyway, or at least not in the way it is currently documented. The code example in the README (below "For context, a full os-matrix using windows-latest could be set up like this:") seems to be broken as well. I can contribute a documentation update and/or implementation fix. But on reading this discussion here, I'd like to first understand what is the expected correct behaviour.

The example code fails with the following error: link to the actions run

/Users/runner/work/_temp/62e0f0dc-5e89-4301-870b-4dd6a410cdd7.sh: line 1: .venv/bin/activate: No such file or directory
sondrelg commented 3 weeks ago

That's surprising. All we're doing in the action, is this: https://github.com/snok/install-poetry/blob/main/main.sh#L59:L66.

How come it's not working in your run? Is the venv not located at .venv, or is source .venv/bin/activate not the right command to enable a venv in this case?

dragondive commented 3 weeks ago

That's surprising. All we're doing in the action, is this: https://github.com/snok/install-poetry/blob/main/main.sh#L59:L66.

How come it's not working in your run? Is the venv not located at .venv, or is source .venv/bin/activate not the right command to enable a venv in this case?

Turns out to be my misunderstanding, and also the result of removing too much relevant config in trying to create a minimal sample. I think I need to give some background on what I originally intended to do.

Creating a venv, but not in the project dir

If you're using the default settings, the venv location changes from .venv to using {cache-dir}/virtualenvs. You can also change the path to whatever you'd like. Generally though, this can make things a little tricky, because the directory will be vary depending on the OS, making it harder to write OS-agnostic workflows.

I want to develop some workflows that addresses these OS-agnostic challenge. These workflows will be mainly used by other people, so I want to make it usable with as little custom configuration as necessary.

To support creating virtualenvs outside the project dir in a OS-agnostic way, I thought I could use the Github context variable ${{ runner.temp }} as the base dir.

I had also looked into the shell script that you pointed to trying to figure out if the $VENV could be "generalized" so that it works regardless of the location of the venv (in project, in default cache, or custom path). Then I came across this discussion where the $VENV deprecation is being discussed, so I was wondering what would be the next steps to take.

sondrelg commented 3 weeks ago

Sorry if this doesn't answer your question; reading it on the go: You can set the $POETRY_HOME environment variable to override the installation directory