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

Handle distribution.files being None #6877

Merged
merged 4 commits into from
Nov 3, 2022

Conversation

jace-ys
Copy link
Contributor

@jace-ys jace-ys commented Oct 24, 2022

Pull Request Check List

Resolves: #6788

We discovered that in some unknown situations, distribution.files can be None, causing the assertion to fail. To handle distribution.files being None more gracefully, we treat it as if it were an empty list.

  • Added tests for changed code.
  • Updated documentation for changed code.

Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

I do think this change is a bit clunky, I'd much rather do something like:

if not distribution.files:
    break

Otherwise, the spirit of the thing looks good to me.

@jace-ys
Copy link
Contributor Author

jace-ys commented Oct 24, 2022

Thanks for the suggestion @neersighted, definitely much neater than what I had - fixed it 👍🏻

@neersighted
Copy link
Member

Ah, it looks like I didn't read close enough -- continue is more correct as we still need to finish the outer for-loop. My bad.

@jace-ys
Copy link
Contributor Author

jace-ys commented Oct 24, 2022

Good catch! Just updated it.

@dimbleby
Copy link
Contributor

this looks suspect in the third place you've done it: we now don't execute the code after the for file in distribution.files, which we previously did for an empty list.

fwiw I preferred the original version of this MR that went files = [] if distribution.files is None else distribution.files...

@jace-ys
Copy link
Contributor Author

jace-ys commented Oct 25, 2022

@dimbleby Yeah good point. I think files = [] if distribution.files is None else distribution.files... has the benefit that it would easily solve the problem you highlighted. I also think it carries the meaning of "if distribution.files is None, we treat it as an empty list", which is nice.

I don't write Python much so don't have strong opinions on which approach is preferred - happy to defer to you folks!

@jace-ys
Copy link
Contributor Author

jace-ys commented Oct 25, 2022

@neersighted Do you have any objections if I revert it back to the original files = [] if distribution.files is None else distribution.files version?

Otherwise, I think the other option is to nest the inner for loop:

if distribution.files is not None:
    for file in distribution.files:
...

@neersighted
Copy link
Member

Ah, good catch, I skimmed too quickly there as well 😆

Yeah, I'd rather just indent/nest the for loop than create an empty list, but that's my personal preference.

@jace-ys jace-ys force-pushed the fix-dist-files-none branch 2 times, most recently from 13560ae to 2a09bef Compare October 25, 2022 21:29
@jace-ys
Copy link
Contributor Author

jace-ys commented Oct 25, 2022

I've reverted it back to the original version I had 👍🏻

(I tried nesting the for loop but it made formatting awkward with the linting of line length)

@neersighted neersighted enabled auto-merge (squash) November 3, 2022 04:57
@neersighted neersighted added kind/bug Something isn't working as expected area/installer Related to the dependency installer impact/changelog Requires a changelog entry labels Nov 3, 2022
@neersighted neersighted added this to the 1.3 milestone Nov 3, 2022
@neersighted neersighted merged commit 18c2903 into python-poetry:master Nov 3, 2022
@leocwolter
Copy link

Hey, are there any workarounds we can use before this is released?

@ffreemt
Copy link

ffreemt commented Dec 3, 2022

Yeah, any workarounds? Am also struggling with a similar situation.

@leocwolter
Copy link

leocwolter commented Dec 3, 2022 via email

@neersighted
Copy link
Member

neersighted commented Dec 3, 2022

Hey, are there any workarounds we can use before this is released?

#7137 is under review, we should be releasing shortly.

Hey, I just noticed that this only happens when installing poetry via apt.

Versions of Poetry distributed by distro packagers are unsupported and carry untested patches. Poetry's test suite is stripped by Debian and not run to validate their changes. We can provide no support for distro-packaged versions of Poetry.

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/installer Related to the dependency installer impact/changelog Requires a changelog entry kind/bug Something isn't working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AssertionError from Poetry install
5 participants