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

Add support for Poetry 1.5 lockfiles #7834

Merged
merged 4 commits into from
Aug 19, 2023
Merged

Conversation

deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Aug 17, 2023

Since we upgraded to Poetry 1.5, we're failing to provide updates to any projects including Poetry 1.5 specific poetry.lock files, which have removed the "category" attribute from locked dependencies.

There's an open PR to fix this at #7418, however, I'm not fully convinced it's the right solution because:

  • It no longer gets right the type (development/production) of sub dependencies.
  • It still keeps the approach of looking into the lockfile directly, so we're subject to suffer from similar changes in the lockfile format in the future.
  • It forces us to keep compatibility code if we don't want to hard fail on lockfiles created by Poetry >1.5, and it's complicated in general.

I think this PR provides a better solution which applies one of the principles we're trying to apply recently: delegate to the package manager as much as possible. So, we let poetry show --only main let us find the list of "production" dependencies.

Also, the logic is moved to the parser. If we get the dependency type right there, then the proper information is already carried out as Dependency#production?.

Fixes #7389.
Fixes #7641.
Supersedes #7418.

@phillipuniverse
Copy link
Contributor

phillipuniverse commented Aug 18, 2023

@deivid-rodriguez I think the email might be the issue with the attribution

Co-authored-by: Phillip Verheyden [email protected]

I think that should be:

Co-authored-by: Phillip Verheyden [email protected]

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/poetry-1.5 branch 2 times, most recently from 78bf3cc to 78c3d48 Compare August 18, 2023 10:31
@deivid-rodriguez
Copy link
Contributor Author

There you go, thanks!

deivid-rodriguez and others added 3 commits August 18, 2023 20:42
Instead of parsing the lockfile directly, use `poetry show --only main`,
and consider the dependencies there as "production", and the others as
"development".

This has the advantage of not having to parse the lockfiles directly,
and as a result fixes an issue where we try to parse a "category"
attribute in the lockfile which has been removed in the latest lockfile
format.

Ideally `poetry show` would support JSON output, but for now this should
be good enough.

Co-authored-by: Phillip Verheyden <[email protected]>
Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me other than possibly the helper can be made more generic. But that can be handled in a follow-on PR, if appropriate.

I'm going to deploy and merge this to unblock all the folks affected by it.

Sorry for the delay here, many thanks to @phillipuniverse for pushing on this repeatedly.

I didn't have bandwidth to look at this til we got through dropping 3.6/3.7, but it does simplify things a bit now that we pin to solely to poetry==1.5.1.

BTW, researching all the background on this issue (I hadn't spent much time looking at poetry before today) resulted in some interesting dinner conversation:

My wife: "Honey, what'd you do today?"
Me: "Read a bunch about poetry"
Her: ???...

File.write(lockfile.name, lockfile.content)

begin
output = Helpers.run_poetry_command("pyenv exec poetry show --only main")
Copy link
Member

@jeffwidman jeffwidman Aug 19, 2023

Choose a reason for hiding this comment

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

👍 for using the poetry CLI rather than inspecting the file directly.

However, the docs for show indicate that --only main and --without dev are both workable here...

At first I was thinking that --without dev might be better here, as main is the default group so there may be other groups, until I saw this note here:

Dependency groups, other than the implicit main group, must only contain dependencies you need in your development process. Installing them is only possible by using Poetry.

So I'm guessing docs/tests, etc are other groups that won't be considered part of main, but aren't part of dev either...

The :dependabot: concept of dep categories isn't quite so sophisticated, we tend to simply have "prod" vs "dev", and in this method solely want prod deps, not "non-dev deps", so --only main makes sense to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, in general I tend to consider "dev", everything that's not production/runtime, that's why I went with --only main.

@jeffwidman jeffwidman merged commit e7e4a7c into main Aug 19, 2023
90 checks passed
@jeffwidman jeffwidman deleted the deivid-rodriguez/poetry-1.5 branch August 19, 2023 08:01
@jeffwidman
Copy link
Member

I tested in our staging environment using this new code against an old-style lockfile with the category key and a new-style without the category key and they both worked flawlessly. So I went ahead and pushed this live to production. If you hit problems, feel free to file an issue and reference this PR in that issue.

@jeffwidman jeffwidman added the L: python:poetry Python packages via poetry label Aug 19, 2023
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Does this affect me?

@nantiferov
Copy link

Hi,

I'm not sure if I should open new issue for this, but seems that even after this released, Dependabot still removes category from lock file.

I tried it today on one of the repos, and PR has all categories removed, example:
image

@phillipuniverse
Copy link
Contributor

phillipuniverse commented Aug 21, 2023

@nantiferov this is expected behavior. Dependabot does not have a way to specify a version of Poetry to use (see #1556) and is currently using 1.5. When poetry.lock is re-created with Poetry 1.5, the category is always removed. The only fix really is to make sure that you are using Poetry 1.5 everywhere (locally, CI, etc) to ensure the category key stays removed.

This PR was about resolving an issue where if that category key was removed, Dependabot could not parse it correctly.

@nantiferov
Copy link

Ah, ok, now it's clear. Thank you!

brettfo pushed a commit to brettfo/dependabot-core that referenced this pull request Oct 11, 2023
…oetry-1.5

Add support for Poetry 1.5 lockfiles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: python:poetry Python packages via poetry L: python
Projects
None yet
4 participants