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

(🎁) upgrade warning about inconsistent lock file to an error #8737

Merged
merged 3 commits into from
Feb 10, 2024

Conversation

KotlinIsland
Copy link
Contributor

@KotlinIsland KotlinIsland commented Dec 4, 2023

Pull Request Check List

This is an extremely common issue that I see people running into all the time, there is a warning at the top of the output about the invalid state of the project, but it is instantly hidden by a wall of other output.

Finally poetry says that the version doesn't exist. Confusingly, poetry is only referring to versions in the lock file that don't exist.

  • [🚀] Added tests for changed code.
  • [🚀] Updated documentation for changed code.

@KotlinIsland KotlinIsland marked this pull request as draft December 4, 2023 03:20
@KotlinIsland KotlinIsland changed the title (🎁) specify lock file in error message (🎁) upgrade warning about inconsistent lock file to an error Dec 4, 2023
@KotlinIsland KotlinIsland marked this pull request as ready for review December 4, 2023 06:40
@ilyagr
Copy link
Contributor

ilyagr commented Jan 12, 2024

Will this allow running Poetry 1.7 (or 1.8) on a lockfile generated by Poetry 1.6? Currently, that generates a warning since the lockfile doesn't match exactly what Poetry 1.7 would generate; I'd hate for that to become an error.

BTW, I agree that erroring out when the lockfile is incompatible with pyproject.toml is a good idea.

@radoering
Copy link
Member

that generates a warning since the lockfile doesn't match exactly what Poetry 1.7 would generate

It does not generate this warning, does it? Actually, I'm not aware that it generates a warning at all. The last update of the lockfile version was in Poetry 1.3.

@ilyagr
Copy link
Contributor

ilyagr commented Jan 13, 2024

Actually, I'm not aware that it generates a warning at all.

Hmm... I was sure I saw it, but I couldn't actually reproduce it. I will let you know if I find it again.

For reference, here is what the diff looks like between a lockfile generated by Poetry 1.3 and Poetry 1.7.1. The differences are slight, but this is what I thought (seemingly incorrectly?) Poetry was complaining about.

diff --git a/poetry.lock b/poetry.lock
index e933ba2ee6...955471c81b 100644
--- a/poetry.lock
+++ b/poetry.lock
@@ -1,10 +1,9 @@
-# This file is automatically @generated by Poetry and should not be changed by hand.
+# This file is automatically @generated by Poetry 1.7.1 and should not be changed by hand.

 [[package]]
 name = "babel"
 version = "2.14.0"
 description = "Internationalization utilities"
-category = "dev"
 optional = false
 python-versions = ">=3.7"
 files = [

(there are then many repeats of the -category = "dev" line).

@radoering
Copy link
Member

category is not written anymore because it has not been read even longer. This should not produce any warning since we decided not to bump the lockfile version. See #7637 for details.

The only risk in this PR is that users edited the pyproject.toml, did not lock because they knew that their change did not affect the lockfile except for the hash and deliberately ignored this warning. This workflow will not work anymore after this change. I suppose we can/should take that risk to improve the feedback for the majority of users.

@ilyagr
Copy link
Contributor

ilyagr commented Jan 14, 2024

The only risk in this PR is that users edited the pyproject.toml, did not lock because they knew that their change did not affect the lockfile except for the hash and deliberately ignored this warning.

I did not realize that poetry.lock contained a hash of the pyproject.toml, so changing a comment in the latter invalidates the former. This might be the warning that confused me.

I don't think this behavior is inherently problematic, but see below for a suggestion about wording.

I suppose we can/should take that risk to improve the feedback for the majority of users.

This seems like a reasonable tradeoff.

One way to make it friendlier is to mention this possibility in the error message, saying something "pyproject.toml changed since poetry.lock was generated". When it says that "poetry.lock is not consistent with pyproject.toml", I assumed it checked that the versions of packages in poetry.lock don't satisfy the version specifiers in pyproject.toml.

P.S. I do appreciate that the same lockfile can be used with a wide variety of versions of poetry. It's nice when people who do sudo apt install poetry and get Poetry 1.3 can work with my project.

@radoering
Copy link
Member

I did not realize that poetry.lock contained a hash of the pyproject.toml, so changing a comment in the latter invalidates the former. This might be the warning that confused me.

It's not a hash of the entire pyproject.toml. Only fields that are relevant for locking are considered.

"pyproject.toml changed since poetry.lock was generated"

Maybe we should say "relevant fields of pyproject.toml changed ..."?

@KotlinIsland
Copy link
Contributor Author

P.S. I do appreciate that the same lockfile can be used with a wide variety of versions of poetry. It's nice when people who do sudo apt install poetry and get Poetry 1.3 can work with my project.

If I ever saw anyone that had been tricked into using apt for installing poetry, I would direct them towards the actual install instructions, or pyprojectx

Also, it's sudo apt install python3-poetry, which will be 1.1.12 if you're on the LTS.

@ilyagr
Copy link
Contributor

ilyagr commented Jan 15, 2024

Maybe we should say "relevant fields of pyproject.toml changed ..."?

Maybe "Dependencies changed in pyproject.toml since poetry.lock was generated"?

@radoering radoering merged commit 9ba2b30 into python-poetry:master Feb 10, 2024
20 checks passed
ilyagr added a commit to ilyagr/poetry that referenced this pull request Feb 10, 2024
ilyagr added a commit to ilyagr/poetry that referenced this pull request Feb 10, 2024
ilyagr added a commit to ilyagr/poetry that referenced this pull request Feb 10, 2024
ilyagr added a commit to ilyagr/poetry that referenced this pull request Feb 10, 2024
ilyagr added a commit to ilyagr/poetry that referenced this pull request Feb 10, 2024
@KotlinIsland KotlinIsland deleted the message branch February 11, 2024 23:50
radoering pushed a commit to ilyagr/poetry that referenced this pull request Feb 13, 2024
maxrake added a commit to phylum-dev/phylum-ci that referenced this pull request Feb 26, 2024
Poetry is the workflow management tool used for this project and forms
the root of all other actions taken when working with this repository.
It is also used to manage dependencies and therefore should be treated
very carefully, with updates to newer versions taken deliberately.

This PR updates `poetry` to the latest version of v1.8.1 to account for
the [changes introduced](https://python-poetry.org/history) in both
v1.8.0 and v1.8.1, with these actions taken:

* Bump all instances of `poetry` to the new version
  * Installs in workflows
  * pre-commit hook revision
  * Dockerfiles
* Update the lockfile with the new version of `poetry`

None of the changes or new features in these new versions required any
updates to the use of `poetry` in this project. Interestingly, a change
to "Upgrade the warning about an inconsistent lockfile to an error"
([#8737](python-poetry/poetry#8737)) still does
not address the lockfile injection attack outlined in the
["Bad Beat Poetry"](https://blog.phylum.io/bad-beat-poetry/) blog post.
Therefore, it is still recommended to check and refresh the lockfile
every time before using it to install an environment:

```
poetry check --lock
poetry lock --no-update --no-cache
poetry install ...
```

A review of the latest `poetry-core` release
([v1.9.0](https://github.com/python-poetry/poetry-core/releases/tag/1.9.0))
did not prove that an upgrade to that version in the `phylum-ci` project
is needed at this time.
maxrake added a commit to phylum-dev/phylum-ci that referenced this pull request Feb 26, 2024
Poetry is the workflow management tool used for this project and forms
the root of all other actions taken when working with this repository.
It is also used to manage dependencies and therefore should be treated
very carefully, with updates to newer versions taken deliberately.

This PR updates `poetry` to the latest version of v1.8.1 to account for
the [changes introduced](https://python-poetry.org/history) in both
v1.8.0 and v1.8.1, with these actions taken:

* Bump all instances of `poetry` to the new version
  * Installs in workflows
  * pre-commit hook revision
  * Dockerfiles
* Update the lockfile with the new version of `poetry`

None of the changes or new features in these new versions required any
updates to the use of `poetry` in this project. Interestingly, a change
to "Upgrade the warning about an inconsistent lockfile to an error"
([#8737](python-poetry/poetry#8737)) still does
not address the lockfile injection attack outlined in the ["Bad Beat
Poetry"](https://blog.phylum.io/bad-beat-poetry/) blog post. Therefore,
it is still recommended to check and refresh the lockfile every time
before using it to install an environment:

```
poetry check --lock
poetry lock --no-update --no-cache
poetry install ...
```

A review of the latest `poetry-core` release
([v1.9.0](https://github.com/python-poetry/poetry-core/releases/tag/1.9.0))
did not prove that an upgrade to that version in the `phylum-ci` project
is needed at this time.
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 Mar 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

option to fail if poetry.lock doesn't match pyproject.toml
4 participants