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

Cleanup ephemeral wheel cache properly #968

Merged
merged 1 commit into from
Nov 25, 2019

Conversation

atugushev
Copy link
Member

@atugushev atugushev commented Oct 25, 2019

Attribute self.wheel_cache has been removed in f6de247, thus self.wheel_cache.cleanup() will allways throw:

AttributeError("'PyPIRepository' object has no attribute 'wheel_cache'")

and wheel_cache.cleanup() will never be called as supposed to be.

WheelCache.cleanup() has been introduced in pip==10.0.0. Try-except block has been refactored, for it's better to check the pip version explicitly (LBYL) rather than ask for forgiveness (EAFP).

Changelog-friendly one-liner: Fix dependency resolver to clean up the ephemeral wheel cache.

Contributor checklist
  • Provided the tests for the changes.
  • Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@atugushev atugushev added the refactor Refactoring code label Oct 25, 2019
@atugushev atugushev force-pushed the cleanup-wheel-cache branch from 045c854 to fd8b573 Compare October 25, 2019 08:24
@codecov
Copy link

codecov bot commented Oct 25, 2019

Codecov Report

Merging #968 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #968      +/-   ##
==========================================
+ Coverage    99.1%   99.11%   +<.01%     
==========================================
  Files          34       34              
  Lines        2357     2362       +5     
  Branches      305      306       +1     
==========================================
+ Hits         2336     2341       +5     
  Misses         11       11              
  Partials       10       10
Impacted Files Coverage Δ
piptools/repositories/pypi.py 92.92% <100%> (-0.08%) ⬇️
tests/test_repository_pypi.py 100% <100%> (ø) ⬆️

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 5efb2de...40d74a6. Read the comment docs.

@atugushev atugushev added bug fix and removed refactor Refactoring code labels Oct 25, 2019
@atugushev atugushev changed the title Cleanup wheel cache properly Cleanup ephemeral wheel cache properly Oct 25, 2019
@atugushev atugushev force-pushed the cleanup-wheel-cache branch 3 times, most recently from d830e8b to 0e3ef2c Compare October 25, 2019 09:55
@atugushev atugushev added this to the 4.3.0 milestone Oct 25, 2019
@atugushev atugushev mentioned this pull request Oct 29, 2019
3 tasks
Attribute `self.wheel_cache` has been removed in f6de247,
thus `self.wheel_cache.cleanup()` will allways throw the AttributeError
and `wheel_cache.cleanup()` will never be called as supposed to be.

WheelCache.cleanup() has been introduced in `pip==10.0.0`.
It's better to check pip version explicitly (LBYL) rather than ask for
forgiveness (EAFP).
@atugushev atugushev removed the request for review from blueyed November 16, 2019 16:35
@atugushev
Copy link
Member Author

ℹ️ rebased onto master and resolved conflicts.

Copy link
Contributor

@suutari suutari left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@atugushev atugushev merged commit e1b86a1 into jazzband:master Nov 25, 2019
@atugushev
Copy link
Member Author

@webknjaz, @codingjoe and @suutari, thanks for reviewing this! 🙏

@atugushev atugushev deleted the cleanup-wheel-cache branch November 25, 2019 08:31
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.

4 participants