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

Resolve pytest pluggy version conflict #1457

Merged

Conversation

caboteria
Copy link
Contributor

@caboteria caboteria commented Oct 16, 2018

This resolves issue #1455 .

toby cabot added 2 commits Oct 16, 2018
Pluggy released a new version today but pytest 3.6.4 explicitly
forbade it, so the urllib3 builds started to fail.  Pytest 3.8.2
allows pluggy 0.8.0 so they will work with one another.
This isn't strictly necessary, but it looks as if this project prefers
to pin its dev dependencies to an exact version (probably to prevent
what happened today when a pluggy release caused the urllib3 build to
start failing).
@codecov-io
Copy link

codecov-io commented Oct 16, 2018

Codecov Report

Merging #1457 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1457   +/-   ##
=======================================
  Coverage   82.09%   82.09%           
=======================================
  Files          22       22           
  Lines        2312     2312           
=======================================
  Hits         1898     1898           
  Misses        414      414

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b548abc...2f21ef8. Read the comment docs.

@sethmlarson sethmlarson reopened this Oct 17, 2018
# 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

@sethmlarson sethmlarson Oct 17, 2018

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

@sethmlarson sethmlarson Oct 17, 2018

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

@caboteria caboteria Oct 17, 2018

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

@caboteria caboteria Oct 17, 2018

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

@pquentin pquentin Oct 17, 2018

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

@caboteria caboteria Oct 17, 2018

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

@pquentin pquentin Oct 17, 2018

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

@sethmlarson sethmlarson Oct 17, 2018

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 :)

@sethmlarson sethmlarson merged commit cd7cfa6 into urllib3:master Oct 17, 2018
Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants