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

[10171] Fix coverage run random failures on PYPY due to SQLite error. #1581

Merged
merged 2 commits into from Apr 10, 2021

Conversation

adiroiban
Copy link
Member

@adiroiban adiroiban commented Apr 6, 2021

Scope and purpose

Coverage runs on PYPY fails

https://github.com/twisted/twisted/pull/1577/checks?check_run_id=2274177993#step:9:751

Experimental fix discussed here nedbat/coveragepy#1010

This is the Twisted integration branch.

Contributor Checklist:

  • The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/10171
  • I ran tox -e lint to format my patch to meet the Twisted Coding Standard
  • I have created a newsfragment in src/twisted/newsfragments/ (see: News files)
  • The title of the PR starts with the associated Trac ticket number (without the # character).
  • [NA] I have updated the automated tests and checked that all checks for the PR are green.
  • I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.
  • The merge commit will use the below format
    The first line is automatically generated by GitHub based on PR ID and branch name.
    The other lines generated by GitHub should be replaced.
Merge pull request #123 from twisted/4356-branch-name-with-trac-id

Author: adiroiban
Reviewer: 
Fixes: ticket:10171

Fix coverage run on PYPY due to SQLite error.

@adiroiban adiroiban marked this pull request as ready for review April 7, 2021 10:40
@adiroiban
Copy link
Member Author

My plan is to have this is trunk for a while... 1 month. If we no longer observer pypy coverage errors. we can consider it fixed. report upstream and use the released version with the fix, when ready,

Thanks!

@adiroiban adiroiban requested a review from a team April 7, 2021 10:44
coverage ~= 5.5
# To be pinned to a minor version once PYPY `coverage run` error is fixed.
# See: https://twistedmatrix.com/trac/ticket/10171
coverage
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 drop the tox.ini change if we do this here?

Suggested change
coverage
coverage @ https://github.com/nedbat/coveragepy/archive/refs/heads/nedbat/another-1010.zip

Copy link
Member

Choose a reason for hiding this comment

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

Hum, actually I suppose...

Suggested change
coverage
coverage ~= 5.5 @ https://github.com/nedbat/coveragepy/archive/refs/heads/nedbat/another-1010.zip

Or describe the version it presently lists, though this is going a bit far probably. The first suggestion in the previous comment is likely sufficient and useful since it isolates the change to only needing to be undone in one place.

https://github.com/nedbat/coveragepy/blob/nedbat/another-1010/coverage/version.py#L8

version_info = (5, 5, 1, "alpha", 0)
Suggested change
coverage
coverage >= 5.5.1, < 6 @ https://github.com/nedbat/coveragepy/archive/refs/heads/nedbat/another-1010.zip

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that we can drop it.

I tried your last suggestion and I got

  pkg_resources.extern.packaging.requirements.InvalidRequirement: Parse error at "'@ https:'": Expected stringEnd

I think that a bit of duplication is ok and there is not much gain by "doing it right(tm)"

I think that this is no longer supported...and it required an extra dependency_links

https://setuptools.readthedocs.io/en/latest/userguide/dependency_management.html?highlight=dependency_links#dependencies-that-aren-t-in-pypi

Copy link
Member

Choose a reason for hiding this comment

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

@ is a new syntax but apparently it doesn't work there... I'll have to go learn. We can move on.

https://www.python.org/dev/peps/pep-0508/#examples

Copy link
Member

Choose a reason for hiding this comment

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

(this is not in setup.cfg, just an example in a place without dependency_links.)

$ venv/bin/pip install 'coverage @ https://github.com/nedbat/coveragepy/archive/refs/heads/nedbat/another-1010.zip'
Collecting coverage@ https://github.com/nedbat/coveragepy/archive/refs/heads/nedbat/another-1010.zip
  Downloading https://github.com/nedbat/coveragepy/archive/refs/heads/nedbat/another-1010.zip
     / 1.0 MB 1.5 MB/s
Building wheels for collected packages: coverage
  Building wheel for coverage (setup.py) ... done
  Created wheel for coverage: filename=coverage-5.5.1a0-cp39-cp39-linux_x86_64.whl size=243403 sha256=de379aeb239ebb6321429001d7085a07748ffb5277bd9692e50c72bf692b885a
  Stored in directory: /home/altendky/.cache/pip/wheels/3e/b1/2f/368d0a88f96d28bf2f5f1e74203b32ac575b293fe0d40f4bcd
Successfully built coverage
Installing collected packages: coverage
Successfully installed coverage-5.5.1a0

Copy link
Member

Choose a reason for hiding this comment

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

Mm, looks like maybe just the version spec is the issue. Either version or @ but not both. Which seems reasonable.

@adiroiban adiroiban merged commit 8e23448 into trunk Apr 10, 2021
@adiroiban adiroiban deleted the 10171-pypy-coverage-run branch April 10, 2021 08:46
@adiroiban
Copy link
Member Author

Thanks for the review. I hope a new coverage release will be made soon and we can then remove this.

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

2 participants