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

chore: re-lock Poetry's dependencies #6950

Merged
merged 2 commits into from
Dec 1, 2022

Conversation

neersighted
Copy link
Member

@neersighted neersighted commented Nov 3, 2022

We can now lock using the new lock file format as Poetry 1.2.2 has spent sufficient time in the wild. A re-lock is also advisable as the full release of Python 3.11 is here and we want to make sure to use any new wheels in CI.

@Secrus
Copy link
Member

Secrus commented Nov 21, 2022

please rebase and resolve, this is easily mergeable

@dimbleby
Copy link
Contributor

suggest that you rebase this and at the same time take poetry-core 1.4.0?

Happy then to rebase #6747 on that.

while you're there it might be sensible to change

trove-classifiers = "^2022.5.19"

to use a >= specifier, this is clearly not semantic versioning.

@neersighted neersighted force-pushed the relock_3.11_1.2.2 branch 2 times, most recently from 6c228b7 to 0330ae3 Compare November 22, 2022 16:21
radoering
radoering previously approved these changes Nov 22, 2022
@neersighted
Copy link
Member Author

Hmm, this change is still seeing/causing non-spurious failures on Python 3.9 on Windows. @dimbleby @radoering do you have time in to dig in and diagnose them (I do not, for the next 24 hours or so)? This looks like a real breakage that is affecting installs in the wild right now, due to some dependency shifting under us...

@dimbleby
Copy link
Contributor

dimbleby commented Nov 22, 2022

I'm not well-placed to do anything with windows-only problems

edit but fwiw I'd bet on the virtualenv bump as the most likely starting place for investigation, maybe try a pipeline that holds that back but allows the rest to float up

@radoering
Copy link
Member

It's virtualenv. The last "good" version is 20.16.5. I don't have time to investigate this further right now. Maybe in the next few days...

@dimbleby
Copy link
Contributor

so my next guess is that this is related to the embedded pip upgrade at pypa/virtualenv#2434.

And while I'm speculating wildly, the next step in this imaginary trail is maybe the pep517 upgrade at pypa/pip#11504.

And after that, in this flight of fancy, would be that it's something to do with the change in that project from a homegrown tempdir() to tempfile.TemporaryDirectory(): because the pipeline failures here look vaguely related to temporary directories, and I know that windows temporary directory has been problematic in the past.

But I still am not well placed to test whether any of this is actually true on a windows machine, and there are enough leaps in this that it... probably isn't true. "Long variation, wrong variation", as they say.

@radoering
Copy link
Member

Quite impressive guess: It is the embedded pip and it is the pep517 upgrade. In pep517, it is pypa/pyproject-hooks#144
If I revert this change in the vendored pep517 of the embedded pip, the error is gone.

@dimbleby
Copy link
Contributor

dimbleby commented Nov 23, 2022

python/cpython#22915 maybe?? this looks as though it's in the right ballpark and is in python 3.10 but not 3.9. I am of course guessing again...

Edit: or python/cpython#93780. Both suggestions derived from https://github.com/python/cpython/commits/3.10/Lib/importlib/_common.py

@radoering
Copy link
Member

I can't reproduce the issue even with 3.10.0a1. I can fix the issue in 3.9 by replacing its importlib with the version from 3.10.a1 and patching importlib._common.from_package() to use a importlib.readers.ZipReader. (For some reason I can't explain, replacing importlib and zipimport.py does not work because importlib._common.from_package() still tries to use _ZipImportResourceReader which is included in the original zipimport.py but not in the replacement.)

I'd say it's probably python/cpython#20576 what's missing in 3.9.

I doubt whether it's worth digging further. In the end only Windows is affected and it's fixed in Python 3.10, so restricting virtualenv for Python 3.9 on Windows should be the way to go.

@dimbleby
Copy link
Contributor

do you have a way to reproduce a problem more conveniently than by running poetry's test suite? is this worth a report at virtualenv - python3.9 is several years away from eol, if it's going to be stuck at old virtualenv forever then that will bring its own problems.

@radoering
Copy link
Member

Installing virtualenv (20.16.5)
Installing virtualenv (20.16.7)

Seems we encountered another bug...

@radoering
Copy link
Member

do you have a way to reproduce a problem more conveniently than by running poetry's test suite?

python pip-22.3-py3-none-any.whl\pip install tomli-2.0.1.tar.gz

is this worth a report at virtualenv

I'm wondering if that's something for virtualenv, pip or pep517?

@dimbleby
Copy link
Contributor

dimbleby commented Nov 25, 2022

Seems we encountered another bug...

I would expect that your two entries for virtualenv in pyproject.toml ought to be mutually exclusive ie I mean one for windows 3.9, one explicitly excluding windows 3.9

I'm wondering if that's something for virtualenv, pip or pep517?

I'd start at virtualenv, showing them the trail. But since they're delivering an embedded pip, all of those things are that project's responsibility in the end.

@dimbleby
Copy link
Contributor

I'd start at virtualenv

but now paying more attention: I see that you have a reproduction that just involves running pip and nothing else. So perhaps pip makes more sense as the place to report this

@radoering
Copy link
Member

I would expect that your two entries for virtualenv in pyproject.toml ought to be mutually exclusive ie I mean one for windows 3.9, one explicitly excluding windows 3.9

That would work. However, it shouldn't be required if one dependency has no markers. See #4590. I think I have a fix and just need to add a test.

We can now lock using the new lock file format as Poetry 1.2.2 has spent
sufficient time in the wild. A re-lock is also advisable as the full
release of Python 3.11 is here and we want to make sure to use any new
wheels in CI.
@dimbleby
Copy link
Contributor

dimbleby commented Dec 1, 2022

awkward, looks as though poetry still installs two virtualenv versions on windows / py3.9.

I'd again suggest updating the markers in this pull request so that they do not overlap - ie explicitly exclude windows / py3.9 in the one that's currently un-markered.

Hopefully that unblocks this and therefore the 1.3.0 release, and fixing the bug can happen more at leisure.

@radoering
Copy link
Member

Arrgh, that's because we use the latest released version to install dependencies. So yeah, I'll make the markers mutually exclusive for now.

@radoering
Copy link
Member

@dimbleby Can you review the constraints/markers please?

@dimbleby
Copy link
Contributor

dimbleby commented Dec 1, 2022

looks good to me.

I might have been tempted to tidy up the virtualenv constraint so that it just cleanly wants some more recent version - and perhaps tomlkit too come to think of it. But that can wait for another day

@radoering radoering merged commit b2e2045 into python-poetry:master Dec 1, 2022
@neersighted neersighted deleted the relock_3.11_1.2.2 branch December 1, 2022 19:27
@neersighted neersighted added this to the 1.3 milestone Dec 21, 2022
@neersighted neersighted added the area/project/deps Related to Poetry's own dependencies label Dec 21, 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/project/deps Related to Poetry's own dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants