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.

Sign up for GitHub

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

Jump to bottom

Resolve pytest pluggy version conflict #1457

Merged
merged 2 commits into from Oct 17, 2018
Merged

Resolve pytest pluggy version conflict #1457

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41a665a
Upgrade pytest to allow it work with pluggy 0.8.0
Oct 16, 2018
2f21ef8
Pin pluggy to 0.8.0
Oct 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion dev-requirements.txt
View file
Open in desktop
Expand Up @@ -7,7 +7,8 @@ tornado==5.0.2
PySocks==1.6.8
pkginfo==1.4.2
pytest-timeout==1.3.1
pytest==3.6.4
pytest==3.8.2
# https://github.com/GoogleCloudPlatform/python-repo-tools/issues/23
pylint<2.0;python_version<="2.7"
gcp-devrel-py-tools==0.0.15
pluggy==0.8.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pluggy==0.8.0

Copy link
Member

Choose a reason for hiding this comment

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

Can we get away with not pinning pluggy here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can get away with not pinning pluggy, but keep in mind that a new version of pluggy is what broke the build yesterday. I saw the other requirements that were pinned to specific versions and inferred that this project likes to have stable dependencies but the build will succeed without it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The commit that upgrades pytest (and fixes the build) is 41a665a and the one that pins pluggy (which is just future-proofing) is 2f21ef8 so if you don't want to pin pluggy you can merge 41a665a but if you do you can merge 2f21ef8.

Copy link
Member

Choose a reason for hiding this comment

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

Arguably that was a bug in pytest 3.6.4, which should have pinned pluggy, just like requests pins urllib3. I think we either want to go the full pip-compile + dependabot route, or just list the dependencies we use, which means we'll get occasional breakage like this. Of course, the maintainers decide. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pytest 3.6.4 did require a range for pluggy (>=0.5,<0.8) but some other module that was loaded sooner caused pluggy 0.8.0 to load so when pytest asked for <0.8 things got crashy. The pain of "DLL hell" is still real.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right. tox 2.9.1 requires pluggy>=0.3.0,<1.0 and is placed before pytest, which is why pluggy 0.8.0 was installed. Really, the only real way to fix this is to use pip-compile, and possibly dependabot to help with the constant updates.

Copy link
Member

Choose a reason for hiding this comment

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

I think merging this as-is is fine for now. We'll have to revisit our whole reproducible build strategy collectively to solve this problem completely :)