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

Yet another approach to fixing local plugin installation #1065

Merged
merged 5 commits into from Mar 9, 2023

Conversation

dairiki
Copy link
Contributor

@dairiki dairiki commented Sep 4, 2022

This is another attempt to fix various issues with installing local plugin distributions into our plugin package cache. (See #865 and #1028 for some discussion of the issues. See #877 and #904 for alternative attempts at a fix.)

Issue(s) Resolved

Fixes #865
Fixes #1028

Related Issues / Links

Supersedes #877
Supersedes #904

Documentation updates for this PR are in lektor/lektor-website#350

Description of Changes

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Update the plugin quickstart to use more modern PEP517/PEP660-isms
  • Link to corresponding documentation pull request for getlektor.com
    (Need to update docs to reflect the removal of lektor dev publish-plugin sub-command.)

Currently, we just create a library directory, add it to sys.path, then install distributions into it using pips --target option (or worse, by passing an --install-dir to setup.py.) This causes various sorts of pain. It also relies on local plugins using setuptools, and having a setup.py, so is not generally PEP517 compatible.

The strategy taken here is to create a bone fide virtual environment in which to install distributions (remote or local) required by a Lektor project. Through a bit of a .pth file hack, this virtual environment is "nested" into the execution environment of Lektor. That is: the site-packages directory of the running Lektor process is added to sys.path within the package cache virtual environment.

This allows installation of plugin distribution (both local and remote) via plain old pip install (or pip install --editable), using the pip from the nested virtual environment. This, hopefully, "just works" for any valid distribution or source directory.

Questions

I haven't yet looked at the code to support lektor plugins register and lektor plugins publish[ed: correction the sub-command in question is lektor dev publish-plugin]. I'm pretty sure it's straightforward to update them, but do we really need these going forward? Does anyone use them?

Note that pre-registration is no longer supported by PyPI, so I'm guessing we could just remove lektor plugins register.

My opinion is that it would be better if people just published their distribution in the normal way (e.g. using build and twine). I'm not sure why Lektor needs to be involved.

@yagebu
Copy link
Contributor

yagebu commented Sep 4, 2022

Didn't have a detailed look at the code yet, but: YES! Nice work :) I think our abuse of a plain directory as a pseudo-virtualenv was probably the root cause of all these problems.

My opinion is that it would be better if people just published their distribution in the normal way (e.g. using build and twine). I'm not sure why Lektor needs to be involved.

👍 I'd also be for removing those commands

@yagebu yagebu mentioned this pull request Sep 4, 2022
@dairiki dairiki force-pushed the bug.865-local-plugin-installation branch 7 times, most recently from 8c7b3d8 to 3cfdc34 Compare September 7, 2022 00:33
dairiki added a commit to dairiki/lektor-website that referenced this pull request Sep 7, 2022
@dairiki
Copy link
Contributor Author

dairiki commented Sep 7, 2022

I've gone ahead and removed the lektor dev publish-plugin command.

@dairiki
Copy link
Contributor Author

dairiki commented Sep 7, 2022

I've updated the plugin quickstart template to make it PEP517 compliant.

I moved the setuptools configuration to setup.cfg. I considered it to pyproject.toml but held off since the setuptools docs include an ominous warning about the setuptools-specific parts of pyproject configuration still being beta.

OTOH, I think the only setuptools-specific configuration we're using is for py_modules and for the readme from README.md dynamic metadata. I think we could omit the py_modules specification entirely and let setuptools' automatic discovery do it's thing (but that's marked as beta, too.) I doubt the dynamic metadata spec for reading README.md is going to change enough to break our simple use case.

Maybe we should just go all-out and move all the distribution config into pyproject.toml?

@dairiki dairiki marked this pull request as ready for review September 7, 2022 01:44
@nixjdm
Copy link
Member

nixjdm commented Sep 7, 2022

Not a lot jumped out at me on a first pass of the code. I really like these changes. In particular I explicitly support removing lektor dev publish-plugin, not just deprecating it, and simplifying this environment management as much as we can.

Could we also add the plugin venv path to lektor project-info? The path is printed when plugins are loaded, but that would be nice too. Since it's now a full, standalone virtual environment, a developer may want to cd to it and activate it directly.

I added a note to your docs PR too.

@nixjdm
Copy link
Member

nixjdm commented Sep 7, 2022

I found a related bit of code I think makes some sense to clean up with this PR.

https://github.com/dairiki/lektor/blob/bug.865-local-plugin-installation/lektor/pluginsystem.py#L203

That doesn't seem to be a great test / function. distributions there is a list of strings, and normally of length 1. But it could be of length 2 if the same plugin is installed from more than one source. Maybe more somehow. From a little testing, I think this'll come up if there is a plugin is installed in the plugin's venv (through either packages/ or the projectfile) and another from a pip install in the primary lektor venv. I'm thinking it might be best to allow that to work, because:

  1. It seems like a reasonable use to rely on the projectfile for a plugin, then want to develop on it with an editable install.
  2. The metadata lib seems to have no trouble deciding which distribution's entry point to use, so why should we? It chooses the primary lektor env's copy, btw. This is just as lektor's copy of requests would be used if a plugin also installed a different version of requests.

@dairiki
Copy link
Contributor Author

dairiki commented Sep 8, 2022

Could we also add the plugin venv path to lektor project-info? The path is printed when plugins are loaded, but that would be nice too. Since it's now a full, standalone virtual environment, a developer may want to cd to it and activate it directly.

That is a good idea. I had thought of it at one point (as I was copy-and-pasting the venv path from a traceback that Lektor had spewed to the console), but then, of course, forgot about it...

@dairiki
Copy link
Contributor Author

dairiki commented Sep 8, 2022

I found a related bit of code I think makes some sense to clean up with this PR.

https://github.com/dairiki/lektor/blob/bug.865-local-plugin-installation/lektor/pluginsystem.py#L203

That doesn't seem to be a great test / function. distributions there is a list of strings, and normally of length 1. But it could be of length 2 if the same plugin is installed from more than one source. Maybe more somehow.

I agree it's not great. I'd rather find a better way to map module to distribution. As your point out, Python knows unambiguously (I think) which distribution the module is from; there must be a cleaner way to get at that.

(At the time I wrote that I was thinking about the possibility of namespace packages, and punting by disallowing Lektor plugins in namespace packages...)

@dairiki
Copy link
Contributor Author

dairiki commented Sep 21, 2022

I found a related bit of code I think makes some sense to clean up with this PR.

https://github.com/dairiki/lektor/blob/bug.865-local-plugin-installation/lektor/pluginsystem.py#L203

PR #1073 does away with this use of packages_distributions altogether. I think it's much cleaner.

@dairiki
Copy link
Contributor Author

dairiki commented Sep 21, 2022

Could we also add the plugin venv path to lektor project-info? The path is printed when plugins are loaded, but that would be nice too. Since it's now a full, standalone virtual environment, a developer may want to cd to it and activate it directly.

I've just pushed a commit to this PR that does this.


I'm not sure how useful activating that venv directly will be. Note that (with this PR), lektor itself does not run "in" the package cache venv — it just adds the package cache venv's site-packages directory to the end of it's sys.path. Normally, the only thing that gets runs "in" the package cache venv is the pip used to install the plugin distributions.

@nixjdm
Copy link
Member

nixjdm commented Oct 6, 2022

Just thinking about this issue again...

A plugin's install_requires=["Lektor"], can be problematic because it can cause a reinstallation of Lektor. Surely a conundrum we'd all love to see resolved! Especially since, really, plugins do rely on Lektor being present generally in order to subclass Plugin from from lektor.pluginsystem import Plugin.

@dairiki
Copy link
Contributor Author

dairiki commented Oct 7, 2022

A plugin's install_requires=["Lektor"], can be problematic because it can cause a reinstallation of Lektor. Surely a conundrum we'd all love to see resolved! Especially since, really, plugins do rely on Lektor being present generally in order to subclass Plugin from from lektor.pluginsystem import Plugin.

Our docs currently recommend against listing Lektor as a dependency of plugin packages — I suspect for this reason. As you say, it does make logical sense to list Lektor as a dependency.

Currently (pre- this PR) I'm pretty sure that if a plugin does list Lektor as a dependency, it causes Lektor to be installed in the package cache, however, that second copy of Lektor (in the package cache) will never get imported within an executing Lektor process. (Imports of any sub-packages and sub-modules of lektor will be resolved via lektor.__path__ which is set when lektor module is first imported, so submodules of lektor are guaranteed to be imported from the same location as the top-level module was imported from, even if we've messed with sys.path in the interim.)

So, while triggering the installation of Lektor into the package cache is inefficient (since it's unnecessary and unused), I don't think it really screws anything up. (Modulo version requirement conflicts.)

I think this PR mostly fixes this. Since it keeps the original sys.path and just adds the package-cache virtualenv to it, and since we now run pip in its normal install mode, rather than with using its --target parameter, pip finds the existing installation of Lektor and does not install a second copy. (Though it still will do so if the plugin requires a different version of Lektor than is currently running. Also, no error message will be generated regarding the version conflict — at least, not at plugin-install-time.)


I've been playing with disusing Lektor’s package management entirely in a couple of my projects. Instead, I run Lektor from a per-project virtualenv. Any required plugins (including any project-local custom plugins) are installed into that same virtualenv. Since no plugins are configured in the .lektorproject file, Lektor does not even create its local package cache.

I've now tried using both Poetry and hatch to manage the project’s virtualenv. Both work fine. @yagebu did this some time ago, using pipenv in lektor/lektor-website#327. Even plain old setup.py, or pip with a requirements.txt, or tox would probably do the job.

The only friction I've encountered doing things this way is that Lektor's pedantic requirements on a plugin’s distribution name make installing project-local plugins a bit of a pain. See this comment on #875 for more details on that.

Anyhow, this approach fixes all these issues with Lektor’s package management, since it avoids all the funny business with the package cache. My current thoughts are that we probably ought to move our docs in this direction (i.e. pick one of Poetry, tox, pipenv, or poetry, and document how to use it to manage plugin installation) and get Lektor out of the package-management business altogether.

of python running within the virtual environment.
"""
with Path(self.site_packages, "_lektor.pth").open("a", encoding="utf-8") as fp:
fp.write(f"import site; site.addsitedir({sitedir!r})\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fp.write(f"import site; site.addsitedir({sitedir!r})\n")
fp.write(sitedir)

Simply writing the path to the .pth file would work.

A path configuration file is a file whose name has the form name.pth and exists in one of the four directories mentioned above; its contents are additional items (one per line) to be added to sys.path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition to adding the directory to sys.path, site.addsitedir also processes any .pth files found therein.
We need that here, and I'm pretty sure that doesn't happen if we just list the directory in the .pth file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed I couldn't get the point adding lektor site-packages to the venv. However, the venv packages must be loaded back into lektor's sys.path.

@nixjdm
Copy link
Member

nixjdm commented Nov 3, 2022

@dairiki I want to thank you for the work you've put in on this web of issues. This PR is great progress.

Anyhow, this approach fixes all these issues with Lektor’s package management, since it avoids all the funny business with the package cache. My current thoughts are that we probably ought to move our docs in this direction (i.e. pick one of Poetry, tox, pipenv, or poetry, and document how to use it to manage plugin installation) and get Lektor out of the package-management business altogether.

My mind keeps coming back to this. I want to fully support getting out of the package management business, and I won't be a stick in the mud about which dedicated tool to use, especially as you're putting in most of the work. I do have two opinions though:

  • I think it's best that Lektor itself and what Lektor uses to manage plugins be the same tool.
  • We should pick Poetry.

Poetry seems to be pretty widely adopted and has consistently favorable sentiment behind it, and that's very important in my opinion. It also seems to be fairly mature and capable in my own estimation, from other use I've had. If you're becoming a strong supporter of Hatch (#1082) (or other tool) over Poetry, that's probably ok. I concede that we all may not agree on which tool to migrate to, but we should migrate to something.

I think I'll make an issue for this specifically to clear the air on it.

@dairiki
Copy link
Contributor Author

dairiki commented Nov 3, 2022

  • I think it's best that Lektor itself and what Lektor uses to manage plugins be the same tool.

I'm not convinced (yet) that that's of much importance.

For Lektor itself, the end goal is to create installable distributions. The ability to manage developer environments is a secondary "nice-to-have" concern. Some things that matter for this, in no particular order:

  • The ability to hook the frontend esbuild into the PEP517 build process (so that, e.g., Lektor can be installed from a git repo.)
  • Ease of test (and test dependency) configuration. (And we have a lot of tests — including the pytest, mypy, linting/pre-commit and javascript tests — to configure!)

For a Lektor project, the needs are quite different. In that case, we care about dependency management, reproducibility, dependency lock files, and that sort of thing — the sorts of things that are (it appears) Poetry's forte. Note that most Lektor projects will never be distributed as a wheel (or sdist), so we don't care at all about the dist-building capabilities of the tool here.

Since what it means to build the Lektor distribution is so different from what it means to "build" a Lektor project (i.e. website), I don't really see a strong reason to use the same tool for both.
Since the Lektor source itself does not reference any particular Lektor plugins, it is not as if the Lektor source would serve as a particularly useful example as to how to configure a Lektor project anyway.

(As a side note, while we should probably pick one anointed dependency-management tool to document for use in Lektor projects, in the end, it's just a matter of getting the right python distributions installed in the environment. It's not rocket science. Almost any tool will work: setuptools, plain old requirements.txt, pip-tools, pipenv, poetry, pdm, hatch, etc.)

  • We should pick Poetry.

So, in #1082 I went with hatch because the hatch docs made it fairly obvious how to hook our frontend build process into the PEP517 build process. I did play around with Poetry a bit, but not long enough to determine whether its plugin system (or some other mechanism) would allow us to do something similar. I don't think hatch can compete with Poetry in terms of package management (but, again, for Lektor itself, we don't care too much about that — our primary concern is to be able to build the distribution.)

I have converted one of my Lektor projects to use Poetry for package management. It works fine!

I think PDM is worth a look too. (I haven't looked at it in depth — maybe @frostming can give us a pitch.) It appears to have package management abilities on par with Poetry, and be more standards (PEP)-compliant at the same time. It also has the nice feature of decoupling the package management frontend from the build backend — reportedly one can use any PEP517 build backend with PDM — i.e. we could continue to use hatchling, hatch's build backend for building the distribution while using PDM for package management.

@frostming
Copy link
Contributor

frostming commented Nov 4, 2022

PDM author here. There are three different tasks here:

  1. Dependency management, which is a user flavor and lektor source does not need to care too much.
  2. Build backend - ability to generate the frontend files when building the source.
  3. Environment management, for different tasks like testing and building docs

PDM and poetry are good at (1). It would be good to recommend one or both in the guide on how to manage a lektor project(website). For lektor source itself, it is up to the maintainers(@dairiki ) on which one is preferred. Forcing them to use the same doesn't make sense to me. PDM gives additional ability to not lock-in with any build backend(2).

For the second task, PDM, poetry, and hatchling all ship with a backend. Frankly speaking, among them, hatchling is the most flexible and customizable build backend which I recommend over others.

As for the third task, that's tox and nox's area of expertise. Hatch(the frontend, not hatchling) also supports this feature.

@dairiki dairiki force-pushed the bug.865-local-plugin-installation branch from 5052255 to 0374be1 Compare November 4, 2022 03:49
@nixjdm
Copy link
Member

nixjdm commented Nov 7, 2022

I suppose the reason I was thinking they should be the same tool stems from the ambiguity of the sentiment that Lektor should get out of package management. How much should it, exactly? If the answer is less than getting rid of it as much as we can, there may be benefit to only having to understand nuances of one tool. But, I'm pretty sure the prevailing sentiment is to really minimize Lektor's job here, and then it really doesn't even know what tool is used in a project. In that case, I'll remove the request to consolidate on Poetry, and we could use anything to maintain the repo as developers. I agree though that some guidance should be given in the docs, wherever we land.

None-the-less, it should be discussed to make sure there's enough consensus and buy-in. I'll make an issue today for it. Edit: #1087

Here we create a "nested" virtual environment in which to install our
plugins.  By "nested" we mean that it keeps the `site-packages`
directory of the "containing" virtual environment (or the system, if
there is no containing virtual environment on sys.path. (It does this
via some `.pth` file magic.)

This allows us to use plain old `pip install` to install plugins,
rather than having to resort to `pip install --target <dir>` which
was causing all sorts of grief.

This also allows us to generalize our plugin installation machinery to
support any PEP517 / PEP660 compatible distribution.

We've also cleaned up detection of changed requirements.

Previously, the package cache was created whenever:

- Any new plugin (local or remote) was added to the project
- Any plugin (local or remote) was removed from the project
- The specified version for any remote plugin was changed

Now that we are using a real virtualenv to manage installation, we
could probably loosen these requirements, and let pip deal with, e.g,
reconciling changes in requested package version.

For now, we keep the existing behavior, but clean up the logic for
detecting when requirements have changed.
This appears not ever to have been referenced since the initial commit
in this repo.
(Hopefully no one is using this.)  For publishing to PyPI, there's no
reason not to use the standard tools (e.g. `build` and `twine`) for
this.
@dairiki dairiki force-pushed the bug.865-local-plugin-installation branch from 0374be1 to c6bbf29 Compare February 28, 2023 18:39
@dairiki
Copy link
Contributor Author

dairiki commented Feb 28, 2023

I've just rebased this onto the current master branch. If no one objects (soon), I'm going to merge it.

(The doc PR lektor/lektor-website#350 still could use some work before we release 3.4.0, but I think this work is complete.)

@dairiki dairiki merged commit 2b5a3cd into lektor:master Mar 9, 2023
@dairiki dairiki deleted the bug.865-local-plugin-installation branch March 9, 2023 00:22
dairiki added a commit to dairiki/lektor-website that referenced this pull request Mar 11, 2023
dairiki added a commit to dairiki/lektor that referenced this pull request Apr 16, 2023
Here we create a "nested" virtual environment in which to install our
plugins.  By "nested" we mean that it keeps the `site-packages`
directory of the "containing" virtual environment (or the system, if
there is no containing virtual environment on sys.path. (It does this
via some `.pth` file magic.)

This allows us to use plain old `pip install` to install plugins,
rather than having to resort to `pip install --target <dir>` which
was causing all sorts of grief.

This also allows us to generalize our plugin installation machinery to
support any PEP517 / PEP660 compatible distribution.

We've also cleaned up detection of changed requirements.

Previously, the package cache was created whenever:

- Any new plugin (local or remote) was added to the project
- Any plugin (local or remote) was removed from the project
- The specified version for any remote plugin was changed

Now that we are using a real virtualenv to manage installation, we
could probably loosen these requirements, and let pip deal with, e.g,
reconciling changes in requested package version.

For now, we keep the existing behavior, but clean up the logic for
detecting when requirements have changed.
dairiki added a commit to dairiki/lektor that referenced this pull request Apr 16, 2023
Here we create a "nested" virtual environment in which to install our
plugins.  By "nested" we mean that it keeps the `site-packages`
directory of the "containing" virtual environment (or the system, if
there is no containing virtual environment on sys.path. (It does this
via some `.pth` file magic.)

This allows us to use plain old `pip install` to install plugins,
rather than having to resort to `pip install --target <dir>` which
was causing all sorts of grief.

This also allows us to generalize our plugin installation machinery to
support any PEP517 / PEP660 compatible distribution.

We've also cleaned up detection of changed requirements.

Previously, the package cache was created whenever:

- Any new plugin (local or remote) was added to the project
- Any plugin (local or remote) was removed from the project
- The specified version for any remote plugin was changed

Now that we are using a real virtualenv to manage installation, we
could probably loosen these requirements, and let pip deal with, e.g,
reconciling changes in requested package version.

For now, we keep the existing behavior, but clean up the logic for
detecting when requirements have changed.
dairiki added a commit to dairiki/lektor that referenced this pull request Apr 16, 2023
Here we create a "nested" virtual environment in which to install our
plugins.  By "nested" we mean that it keeps the `site-packages`
directory of the "containing" virtual environment (or the system, if
there is no containing virtual environment on sys.path. (It does this
via some `.pth` file magic.)

This allows us to use plain old `pip install` to install plugins,
rather than having to resort to `pip install --target <dir>` which
was causing all sorts of grief.

This also allows us to generalize our plugin installation machinery to
support any PEP517 / PEP660 compatible distribution.

We've also cleaned up detection of changed requirements.

Previously, the package cache was created whenever:

- Any new plugin (local or remote) was added to the project
- Any plugin (local or remote) was removed from the project
- The specified version for any remote plugin was changed

Now that we are using a real virtualenv to manage installation, we
could probably loosen these requirements, and let pip deal with, e.g,
reconciling changes in requested package version.

For now, we keep the existing behavior, but clean up the logic for
detecting when requirements have changed.
dairiki added a commit to dairiki/lektor that referenced this pull request Sep 11, 2023
* An alternative fix for installing local plugins.

Here we create a "nested" virtual environment in which to install our
plugins.  By "nested" we mean that it keeps the `site-packages`
directory of the "containing" virtual environment (or the system, if
there is no containing virtual environment on sys.path. (It does this
via some `.pth` file magic.)

This allows us to use plain old `pip install` to install plugins,
rather than having to resort to `pip install --target <dir>` which
was causing all sorts of grief.

This also allows us to generalize our plugin installation machinery to
support any PEP517 / PEP660 compatible distribution.

We've also cleaned up detection of changed requirements.

Previously, the package cache was created whenever:

- Any new plugin (local or remote) was added to the project
- Any plugin (local or remote) was removed from the project
- The specified version for any remote plugin was changed

Now that we are using a real virtualenv to manage installation, we
could probably loosen these requirements, and let pip deal with, e.g,
reconciling changes in requested package version.

For now, we keep the existing behavior, but clean up the logic for
detecting when requirements have changed.

* Delete unused lektor.packages.PackageException

This appears not ever to have been referenced since the initial commit
in this repo.

* Remove the `lektor dev publish-plugin` sub-command

(Hopefully no one is using this.)  For publishing to PyPI, there's no
reason not to use the standard tools (e.g. `build` and `twine`) for
this.

* PEP517-ize the plugin quickstart template

* Add package cache (venv) path to the output of `lektor project-info`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weird behavior of installing local packages Wrong way to install dependencies of plugins
4 participants