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

Suggest Tox isolated_build w/o poetry install #2416

Closed
wants to merge 1 commit into from
Closed

Suggest Tox isolated_build w/o poetry install #2416

wants to merge 1 commit into from

Conversation

bittner
Copy link

@bittner bittner commented May 13, 2020

Adapts the suggestion about how to integration Poetry with Tox (roughly) following @sinoroc's suggestion. The new way avoids a common anti-pattern (or better: outdated approach) in CI, the imperative installation of prerequisites.

Resolves: #1941

@finswimmer finswimmer requested a review from a team May 14, 2020 18:18
@finswimmer finswimmer added the area/docs Documentation issues/improvements label May 14, 2020
@igor47
Copy link

igor47 commented May 20, 2020

as commented on #1941 , i'm not sure this is the right approach. the currently-documented suggestion allows me to test my application with identical dependencies between local dev, CI, and production. i'm not sure how to do this otherwise

@bittner
Copy link
Author

bittner commented May 20, 2020

@igor47 Sure, this might work for you this way. But there should be a better way to achieve just that. For you and for everyone else.

In a Tox configuration you should specify the dependencies in a declarative way (that's what deps = is for). Then, any deployment strategy Tox supports can install just those dependencies. In other words, Tox can choose which application or procedure it applies to ensure that those dependencies are installed.

By suggesting a foobar install command we're giving up on that clean separation of concerns, and on the flexibility we gain with that. That's bad. And we should make sure people apply the better strategy. Hence this change.

@Secrus
Copy link
Member

Secrus commented May 21, 2022

Hi, @bittner are you still interested in contributing this to Poetry? If so, please describe both options as according to the team, both have their use.

@Secrus Secrus added the status/waiting-on-response Waiting on response from author label May 21, 2022
@bittner
Copy link
Author

bittner commented May 21, 2022

Hi, @bittner are you still interested in contributing this to Poetry? If so, please describe both options as according to the team, both have their use.

I'm not sure I understand. Do you intend, keep the current description and suggest my change only as an alternative way in the docs?

@Secrus
Copy link
Member

Secrus commented May 21, 2022

I'm not sure I understand. Do you intend, keep the current description and suggest my change only as an alternative way in the docs?

Yes, that would be the plan.

@finswimmer
Copy link
Member

tox can be configured in multiple ways. It depends on what should be the code under test and which dependencies should be installed. The docs should include the several use cases.

[tox]
isolated_build = true

[testenv]
deps =
    pytest
commands =
    pytest tests/ --import-mode importlib

tox will create an sdist package of the project and uses pip to install it in a fresh environment. Thus dependencies are resolved by pip.

[tox]
isolated_build = true

[testenv]
whitelist_externals = poetry
commands_pre =
    poetry install --no-root --sync
commands =
    poetry run pytest tests/ --import-mode importlib

tox will create an sdist package of the project and uses pip to install it in a fresh environment. Thus dependencies are resolved by pip in the first place. But afterwards we run Poetry, which will install the locked dependencies into the environment.

[tox]
isolated_build = true

[testenv]
skip_install = true
whitelist_externals = poetry
commands_pre =
    poetry install
commands =
    poetry run pytest tests/ --import-mode importlib

tox will not do any install. Poetry installs all the dependencies and the current package an editable mode. Thus tests are running against the local files and not the builded and installed package.

@bittner
Copy link
Author

bittner commented May 22, 2022

That's all good.

Just to avoid misunderstandings, my point is not what you can do, but what you should do.

Usually, the tool dictates how you should do things. I believe, you should tell Tox what to do and it will do it for you (i.e. no need to write down the installation command with your dependencies; only compile a list of dependencies). If you go against the design of the tool you interfere with some of its features and make changes of the setup more difficult.

As an alternative, if you want to run just sequences of actual commands then write a Makefile or a shell script.

As Poetry is a very popular tool, we have the responsibility to tell developers how to do things the right way. Hence this PR.

@finswimmer
Copy link
Member

Hey @bittner,

the problem with "telling developers how to do things the right way" here, is that there is not a single way. It depends on the goal.

Your suggested changes describes the way how to use tox in the same way, a lot of people are doing it, when developing a package with setuptools.

The current description, describes a way how to use locked dependencies. That's something a lot of poetry user wanted to do.

That's why I think it would be a great benefit for the docs to explain the different options and when to use what.

fin swimmer

@sirosen
Copy link

sirosen commented Jun 27, 2022

In addition to the suggestions above, I believe this one from #1941 is valuable to include.

This gives up on using poetry's --dev/--no-dev behaviors in exchange for having dependencies only listed in pyproject.toml, not in other files, and not requiring the tox caller to have poetry installed.

This appears to be stalled for a while now, are we open to a fresh PR which enumerates the options?
I would like to see the docs show all of the options in a tabbed layout or something similar.

@finswimmer
Copy link
Member

Superseded by #6026

@finswimmer finswimmer closed this Jul 18, 2022
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/docs Documentation issues/improvements status/waiting-on-response Waiting on response from author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FAQ]: The tox answer should recommend poetry install --no-root instead of poetry install
5 participants