Improve Invoke task handling on Windows

[MinchinWeb]

Some code clean-up to the 'tasks.py' file (for invoke) to allow the project to run cleaner on Windows. Most (all?) of these fixes are already in place in the main pelican code.

Also includes some code to create the virtual environment if it doesn't already exist.

@justinmayer : I'm not sure what you want to do for reviewing my PR's... Do you want to review and merge them yourself, or are you ok with me taking the lead on these?

[MinchinWeb]

c.f. #5

[justinmayer]

The way that virtual environments were intended to be handled is (1) use the active virtual environment if present, and if not, (2) create and manage the virtual environment automatically via Poetry. I think allowing Poetry to transparently create and manage virtual environments provides a more seamless solution for folks who don't want/need to manually manage environments, so I'm not sure what benefits would accrue from duplicating that effort in the setup() Invoke task.

[justinmayer]

@MinchinWeb: Thanks for the enhancements. I raised a question about how virtual environments are created & managed, along with a few other suggestions. ✨

Do you want to review and merge them yourself, or are you ok with me taking the lead on these?

I think having the extra pair of eyes can only benefit the project, so if you don't mind, perhaps we can merge PRs after review & consensus that they are ready?

[justinmayer]

@MinchinWeb: Did you see my review comment above? In short, I'm not sure it makes sense to manually create virtual environments when Poetry can probably handle that part much more effectively. What do you think?

[MinchinWeb]

I'm actually working on it right now :)

In short, I'm not sure it makes sense to manually create virtual environments when Poetry can probably handle that part much more effectively. What do you think?

The problem is that's not what the code currently does. The first thing current code tries to upgrade pip in the virtual environment, and when it can't, it fails with the cryptic "The system cannot find the path specified." (Path for what??) I feel like since this is probably the very first thing a new contributor (or me, after being away for a few months) will encounter when trying to contribute to the project, it should at least give a more useful error message.

I haven't used poetry a ton...I know it manages dependencies within a virtual environment, but can it set them up from scratch too?

[MinchinWeb]

@justinmayer Poetry seems to be able to create virtual environments. See https://python-poetry.org/docs/basic-usage/#using-your-virtual-environment

You can get the path to your current poetry virtual environment from the command line: poetry env info --path See https://python-poetry.org/docs/managing-environments/#displaying-the-environment-information

[MinchinWeb]

I guess what I'm trying to say is I like the idea of poetry managing the virtual environments, but the current tasks.py isn't actually set up to let that (entirely) happen. To get it to work, we'd have to:

• tell the user to install poetry if we can't find it on their PATH
• ask poetry for the location of the virtual environment (if it exists)
• create the virtual environment before we start trying to install things into it or upgrading pip in it

[justinmayer]

I feel like since this is probably the very first thing a new contributor (or me, after being away for a few months) will encounter when trying to contribute to the project, it should at least give a more useful error message.

I agree that making things easy for contributors, new and old alike, is a primary goal. That's precisely why I created the setup Invoke task.

I haven't used Poetry a ton...I know it manages dependencies within a virtual environment, but can it set them up from scratch too?

Yes — that's what I was alluding to but apparently never said outright. Poetry will automatically create, activate, and manage virtual environments transparently. Many times users don't even need to interact with the environments at all.

I guess what I'm trying to say is I like the idea of Poetry managing the virtual environments, but the current tasks.py isn't actually set up to let that (entirely) happen.

I'm sure there are potential enhancements to be made, but understand that we assume that would-be contributors will read the documentation thoroughly and follow the provided How to Set Up Your Development Environment instructions. For example, in this case, the documentation suggests that Poetry be installed via:

curl -sSL https://raw.githubusercontent.com/python-poetry/poetry/master/get-poetry.py | python -

Plus, in those same docs, we encourage folks who understand how virtual environments work to manually create and activate them. But folks are free to allow Poetry to handle that chore, since Poetry is quite good at creating and activating environments.

To get it to work, we'd have to:

• tell the user to install Poetry if we can't find it on their PATH

If Poetry isn't installed via the command mentioned above, then we have a bootstrapping Catch-22, and we assume the user will follow our documentation to create their own virtual environment manually, and thus we assume Poetry will be installed into and available from that virtual environment.

• ask Poetry for the location of the virtual environment (if it exists)
• create the virtual environment before we start trying to install things into it or upgrading Pip in it

These steps should not be necessary. If Poetry is installed via the above canonical install command, Poetry should automatically create and manage the virtual environment.

I just ran through some tests and believe the problem you experienced can be rectified by changing one line in the setup task, resulting in:

@task
def setup(c):
    tools(c)
    c.run(f"{POETRY} run pip install -U pip")
    c.run(f"{POETRY} install")
    precommit(c)

Bootstrapping is hard. If the user (1) doesn't have Poetry or Invoke globally-available and (2) has not created and activated a virtual environment, it's an indication that the instructions have not done the job somehow (or haven't been followed). That said, I think we could add some checks to tasks.py for these conditions and print a more user-friendly message, telling the user what needs to be rectified.

[justinmayer]

Regarding the latter, this might do the trick:

def setup(c):
    if which("poetry") or ACTIVE_VENV:
        tools(c)
        c.run(f"{POETRY} run pip install -U pip")
        c.run(f"{POETRY} install")
        precommit(c)
    else:
        sys.exit(
            """Poetry is not installed, and there is no active virtual environment
            available. You can either manually create and activate a virtual
            environment, or you can install Poetry via:

            curl -sSL https://raw.githubusercontent.com/python-poetry/poetry/master/get-poetry.py | python -

            Once you have taken one of the above two steps, run `invoke setup` again.
            """
        )

[MinchinWeb]

@justinmayer I used the code you suggested, and changed some other paths to lean more heavily on poetry.

One issue I run into locally is the poetry-created venv has a name nothing like what the script assumes: it was created at C:\Users\<username>\AppData\Local\pypoetry\Cache\virtualenvs\pelican-jinja-filters-pib10Hwz-py3.9. I know you can get that out of poetry, but I'm not sure how to programmatically at the moment to then feed into other parts of the script. So my changes are designed to sidestep that as much as possible. I've tested it locally, and it seems to work.

[justinmayer]

Since Poetry's environment handling is meant to be transparent, you have the right idea in not even attempting to retrieve and use its environment path. If someone is using Poetry to manage virtual environments, we should let Poetry handle them altogether if we can.

I took slightly different approach to this question of whether there is an active virtual environment, or whether Poetry is handling that: https://github.com/getpelican/cookiecutter-pelican-plugin/compare/tasks-enhanced?expand=1

In short, I added a dynamic prefix to the various tools, which changes to poetry run for folks who prefer to use Poetry to manage environments, and to {VENV_BIN}/ for folks who don't rely on Poetry's environment management. That way, for the latter group, Poetry isn't being invoked when it arguably doesn't need to be.

What do you think?

[MinchinWeb]

@justinmayer I like the approach you took. The one change I made was the call the variable BIN_PREFIX rather than "just" PREFIX; hopefully this makes it a little more clear. I think this is ready! (cross fingers...)

[MinchinWeb]

One question: should we update the default VENV location? Here, it's ~/virtualenvs, which in the cookie-cutter project it's ~/.local/share/virtualenvs.

[justinmayer]

The one change I made was the call the variable BIN_PREFIX rather than "just" PREFIX; hopefully this makes it a little more clear.

I thought about that, but it felt strange to use that name in the case that the prefix ends up being poetry run […]. How about CMD_PREFIX?

Should we update the default VENV location? Here, it's ~/virtualenvs, which in the cookie-cutter project it's ~/.local/share/virtualenvs.

Ultimately this is a matter of personal preference, since there isn't a super-common convention. While I personally keep mine in ~/virtualenvs for easy accessibility, I think ~/.local/share/virtualenvs might be a better default for most users, since it tidily tucks away environments, out-of-sight, in a location that seems logically suited for these kinds of files. Plus, despite my distaste for Pipenv, ~/.local/share/virtualenvs is the location that it and some other tools use, so there is at least some common precedent for that location.

[MinchinWeb]

Ok, both are updated!

I am working on a couple new filters to support my theme, so in terms of releases, it may make sense to wait for those (before pushing out a new release). I'll create a pull request with them as soon as I've confirmed they're working.

[justinmayer]

Looks good. Do you want to squash any of the irrelevant commits here (e.g,. via interactive rebase) before merge? Not strictly necessary, of course. Up to you.

[MinchinWeb]

...I'm working to figure out how to do that...

[justinmayer]

No worries. I have taken the liberty of squashing most of the commits together, as there was a bit of fix-up churn, and they were quite related to one another.

It took me a while to figure out how to use Git's interactive rebase feature. This guide from a few years ago shows the basics: https://thoughtbot.com/blog/git-interactive-rebase-squash-amend-rewriting-history

As an aside, if I were you, the following is probably how I would have handled this:

1. When I make changes that essentially constitute a fix-up for a previous commit, I look for a word in that previous commit message (say, "invoke"), and then I use: git commit --fixup :/invoke
2. I then use git log and count how many recent commits I want to act upon. In this case, there were 12 commits in this PR, so: git rebase -i HEAD~12
3. I use the interactive rebase tool to re-order commits, mark many of them as fix-up commits, mark others as reword so I can change their commit messages, etc.
4. Once I'm happy with the commits in the feature branch, I rebase the branch onto master via: git rebase master
5. Finally, I would force-push the rebased branch via: git push --force origin invoke-windows

By the way, one should only do this when feeling confident that nobody else has cloned the branch and collaborated on it. In this case, I might have cloned the invoke-windows branch and worked on it, so you wouldn't want to do the above without checking with other potential collaborators first. It's often a workflow that is better suited for forked clones, but it can also work well when, as in this case, we are essentially done and just trying to tidy things up.

By the way, rather than cherry-pick my commit from master into this PR, the above git rebase master step would have achieved the same result without duplicating the commit in this branch.

Hopefully the above is somewhat helpful. Looking forward to the next collaboration! 😁