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

Take Poetry's lock file into account #24

Open
sinoroc opened this issue Sep 29, 2020 · 11 comments
Open

Take Poetry's lock file into account #24

sinoroc opened this issue Sep 29, 2020 · 11 comments
Assignees
Labels
enhancement New feature or request

Comments

@sinoroc
Copy link
Owner

sinoroc commented Sep 29, 2020

There should be a way to instruct Tox to install the pinned versions as read from Poetry's lock file.

UX:

Probably from a UX point of view it could look like this:

[testenv:something]
# ...
poetry_use_lock_file = True

Related:

@sinoroc sinoroc added the enhancement New feature or request label Sep 29, 2020
@sinoroc sinoroc self-assigned this Sep 29, 2020
@sinoroc sinoroc pinned this issue Sep 29, 2020
@sinoroc
Copy link
Owner Author

sinoroc commented Sep 29, 2020

I believe I know more or less how to get started on this. But before I start, there is still one thing that somewhat bothers me about this, to make sure this feature is really worth it and what it should do:

How would that be any better than having commands = poetry install [--no-root]?

From what I understood, reproducibility is very important. The combination of libraries should be the exact same in the testenv as it will be in production. But in Tox we would most likely need to delegate to pip (or whatever the install_command is). There is always a risk of having a slightly different behaviour: meaning a slightly different combination of libraries (hopefully not, if we check the hashes and what not). So how do we mitigate that risk or check that the libraries are exactly the same as in the lock file?

Possible reasons why it would be better than poetry install:

  • we can not guarantee that the poetry binary is available, and we do not want to install it inside the testenv
    • but we still have at least pip (setuptools) in the testenv
    • how is the production environment? does it have pip or poetry inside?
    • how is the application and dependencies installed on the production environment
  • ...

@mikenerone

@mikenerone
Copy link
Contributor

mikenerone commented Sep 29, 2020

@sinoroc You may well be right that it's not better - my thinking has been shifting on this for the last week or so. Out of necessity, I've already switched back to a tox config that does it that way. The very visible problem with this approach was that devs needed to explicitly include the separate poetry testenv in the tox command line, but I thought of a variation that cleaned that up (putting poetry it in tox's own env with the global requires setting) so that's not an issue anymore, and it seems to be working well enough. :|

@fredrikaverpil
Copy link

fredrikaverpil commented Sep 29, 2020

I agree with @mikenerone that it works quite well with {toxworkdir}/.poetry/bin/poetry install --no-root. However, this solution is not cross platform compatible.

I went and added a pull request towards tox so to add a keyword for the "bin" folder (which would then get replaced with "bin" on posix and "Scripts" on Windows). However, one of the maintainers did not like this. They also disapproved this entire approach of mixing in the delegation of installing the venv to poetry like this, basically because accessing binaries outside of the tox testenv was not ideal in their mind.

You can read the full thread here. And why not get in touch with the maintainers to receive feedback, perhaps?

Either way, I would rather use your plugin than call {toxworkdir}/.poetry/bin/poetry install --no-root. I presume you could make this cross platform quite easily too. 😋

@sinoroc
Copy link
Owner Author

sinoroc commented Sep 30, 2020

The very visible problem with this approach was that devs needed to explicitly include the separate poetry testenv in the tox command line

@mikenerone You mean you used to run tox -e poetry,somethingelse or tox -e poetry && tox? So poetry is not already installed before starting to run tox? So you only install tox (globally?), but not poetry? I see...

@sinoroc
Copy link
Owner Author

sinoroc commented Sep 30, 2020

They also disapproved this entire approach of mixing in the delegation of installing the venv to poetry like this, basically because accessing binaries outside of the tox testenv was not ideal in their mind.

@fredrikaverpil I had read most of that discussion already. I must say I agree with them, mixing the testenvs feels weird. Although I see your point. There has to be a better way to solve this.

On the other hand, it is strange when they say that they consider allowlist_externals a red flag. Honestly I do not think that installing the test tools (or poetry, or even pip) in the testenv is a better idea. I'd much rather have the tools isolated, and point them at the environment that should be acted upon (tested), so we need allowlist_externals, but obviously for most of the tools there is no other way than installing them inside the testenv. For static tools (those that only read the code without executing it), it should be no problem, but for the others such as pytest (that do need to run the code) it seems impossible.

So on that front, poetry is on the right track. It doesn't need to be in the env, it can act on any env that is activated. Also the work in progress that would remove the need to install pip, setuptools, etc. in the env is very welcome.

@mikenerone
Copy link
Contributor

mikenerone commented Oct 2, 2020

@mikenerone You mean you used to run tox -e poetry,somethingelse or tox -e poetry && tox? So poetry is not already installed before starting to run tox? So you only install tox (globally?), but not poetry? I see...

Yes, we used to have to do e.g. tox -e poetry,py36.

To answer your second question, devs install both Poetry and tox globally on their systems. The tox requirements = approach is intended primarily for the build servers, and provides at least four benefits there (all closely related):

  1. Simply less for DevOps to maintain in terms of installed software on those servers. Only tox >= 3.8 is required.
  2. Also less knowledge of package specifics required by DevOps. All they have to know is that, for Python projects, all Jenkins needs to do to make a package is tox -e wheel, we we devs will make sure that does the right thing for whatever build system that project happens to be using.
  3. Even if all projects used the same build system (Poetry, obviously :P, but this isn't the case right now), it's conceivable that different projects might need different versions of Poetry. Keeping the Poetry installs isolated to tox envs allow us to define Poetry version constraints on a per-project basis.
  4. Similarly, it allows us to define required poetry plugins (with version constraints) or other utility packages on a per-project basis. (Granted, there's no formal plugin system yet, so such plugins are hacks at the moment, but in any case, a real plugin system is on the roadmap.)

sinoroc added a commit that referenced this issue Oct 3, 2020
sinoroc added a commit that referenced this issue Oct 12, 2020
sinoroc added a commit that referenced this issue Oct 12, 2020
sinoroc added a commit that referenced this issue Oct 12, 2020
sinoroc added a commit that referenced this issue Oct 12, 2020
sinoroc added a commit that referenced this issue Oct 12, 2020
sinoroc added a commit that referenced this issue Oct 12, 2020
sinoroc added a commit that referenced this issue Oct 12, 2020
sinoroc added a commit that referenced this issue Oct 12, 2020
@sinoroc
Copy link
Owner Author

sinoroc commented Oct 13, 2020

An experimental implementation based on reading the TOML in poetry.lock directly has been released in v0.0.4.

The issue stays open until hopefully the behaviour is proved to be conform to expectations, and/or the implementation can be rewritten to rely directly on poetry-core instead.

@fredrikaverpil
Copy link

Where will tox expect me to define poetry_experimental_add_locked_dependencies?
Is it in the [tox] section or in each testenv?

@sinoroc
Copy link
Owner Author

sinoroc commented Oct 13, 2020

Where will tox expect me to define poetry_experimental_add_locked_dependencies?
Is it in the [tox] section or in each testenv?

It is a per-env setting:

[testenv:something]
poetry_experimental_add_locked_dependencies = True

Of course it can also be set as default for all environments in the nameless [testenv]:

[testenv]
poetry_experimental_add_locked_dependencies = True

@fredrikaverpil
Copy link

Thanks, great. And I should still add the poetry_add_dev_dependencies in each testenv as well, right?

@sinoroc
Copy link
Owner Author

sinoroc commented Oct 13, 2020

Thanks, great. And I should still add the poetry_add_dev_dependencies in each testenv as well, right?

The 2 settings are independent and can be used in combination.

You can choose to have:

  • unpinned mandatory dependencies without development dependencies
  • pinned mandatory dependencies without development dependencies
  • unpinned mandatory and development dependencies
  • pinned mandatory and development dependencies

deps *_add_locked_dependencies *_add_dev_dependencies
unpinned main (no dev) False False
pinned main (no dev) True False
unpinned main and dev False True
pinned main and dev True True

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants