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

Error out when poetry -C argument is not a pyproject directory #695

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Cypher1
Copy link

@Cypher1 Cypher1 commented Feb 5, 2024

Poetry's current handling of the --directory (or -C) argument does not protect against incorrectly specified paths (e.g. misspellings and simple mistakes).
In mono repo projects, and other cases where a parent directory has a pyproject.toml this can lead to commands being run in an unexpected package environment, leading to error prone and difficult to catch behaviour in multi package repositories.

This change retains the default behavior of poetry when no -C argument is specified (i.e. walking up the tree from the current directory) but ensures that the -C argument is valid when it is manually specified.

This prevents incorrectly typed directories from breaking builds & CI scripts, while retaining the current default behaviour, minimising change.

Some tests are also updated where they previously pass paths to pyproject.toml files to the cwd arg, (instead of project directories).

Fixes tests that previously passed pyproject.toml paths
as the 'cwd' argument to avoid failures and complexity.
@Cypher1
Copy link
Author

Cypher1 commented Feb 13, 2024

@radoering would you be free to review this?

@radoering
Copy link
Member

I am currently busy preparing Poetry 1.8. I'll take a look after that's done. (The corresponding poetry-core has already been released at the beginning of February so you caught an unlucky timing.)

@Cypher1
Copy link
Author

Cypher1 commented Feb 13, 2024

No problem at all. Thx!

@radoering
Copy link
Member

We have to be careful not to accidentally introduce a breaking change and therefore evaluate all uses of create_poetry() with cwd != None in poetry-core and poetry:

usage evaluation remark
poetry.core.masonry.api (4x) ⚠️ That could be a dangerous one, since it's the API. I'm not sure if we can assume that poetry-core is never called from a subdirectory.
poetry.console.application ⚠️ If directory is not set, we explicitly pass the cwd. I think this PR will introduce an unwanted change in this case. However, we can make a downstream change to just pass None instead.
poetry.console.commands.self.self_command This one should be safe since we pass the directory of the pyproject.toml.
poetry.inspection.info This one should be safe since we pass the directory of the pyproject.toml.
poetry.installation.executor This one should be safe since we pass the directory of the pyproject.toml.

@Cypher1
Copy link
Author

Cypher1 commented Mar 5, 2024

@radoering How can AI help with these concerns? I think the current state of the -C argument is not ideal and leads to significant issues when maintaining a large (i.e. multi-package) mono repo, still I am totally on board with backwards compatibility's important, even when the current implementation does not 100% match the documented (and arguably expected) behaviour.

Looking forward to hear more about the project's needs on this front.
Cheers,
Jay

Cypher1 and others added 3 commits March 5, 2024 11:47
Poetry's current handling of the --directory (or -C) argument does not protect against incorrectly specified paths (e.g. misspellings and simple mistakes).
In mono repo projects, and other cases where a parent directory has a pyproject.toml this can lead to commands being run in an unexpected package environment, leading to error prone and difficult to catch behaviour in multi package repositories.

This change retains the default behavior of poetry when no -C argument is specified (i.e. walking up the tree from the current directory) but ensures that the -C argument is valid when it is manually specified.

This prevents incorrectly typed directories from breaking builds & CI scripts, while retaining the current default behaviour, minimising change.

Some tests are also updated where they previously pass paths to pyproject.toml files to the cwd arg, (instead of project directories).
Copy link

sonarcloud bot commented Mar 5, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@abn
Copy link
Member

abn commented Mar 5, 2024

Don't think this is the correct spot for handling an input validation from the poetry cli.

This should be handled at https://github.com/python-poetry/poetry/blob/d08c75afcc097b3b6a73c704f64f7cebbc2765c1/src/poetry/console/application.py#L123-L131

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants