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

Fix bug in cache updates and improve cache logging #3708

Merged
merged 3 commits into from Jul 13, 2017

Conversation

Projects
None yet
2 participants
@gvanrossum
Member

gvanrossum commented Jul 12, 2017

This started out as an attempt to make the logging messages around cache freshness/updates more useful, but I ended up discovering a bug in the code that updates the cache meta file when the mtime or path of the file has changes but size and source hash are the same. It wrote a list instead of a dict! This is a bug in a feature announced in 0.520 so should be included in 0.521 if/when we release it.

I also decided to ignore debug_cache when comparing options (since it only affects the JSON style -- it's still a per-file option but changing it no longer invalidates the cache)

And I made the --skip-version-check option strictly ignore the platform (since it's undocumented I can make it do what I want, and this is more useful for my one use case).

gvanrossum added some commits Jun 28, 2017

Several more changes.
- Fix cache updates (it wrote unusable JSON, dammit).
- Completely ignore platform when --skip-version-check is used.
- Don't compare option setting for debug_cache (so silly).

@ddfisher ddfisher self-requested a review Jul 13, 2017

@ddfisher

Not sure I fully understand this code, but the changes seem reasonable. Feel free to merge after addressing the two nits.

if not os.path.exists(meta_json):
manager.trace('Could not load cache for {}: could not find {}'.format(id, meta_json))
manager.log('Could not load cache for {}: could not find {}'.format(id, meta_json))

This comment has been minimized.

@ddfisher

ddfisher Jul 13, 2017

Collaborator

Why the change from trace to log? And did you intend to change the trace above as well?

@ddfisher

ddfisher Jul 13, 2017

Collaborator

Why the change from trace to log? And did you intend to change the trace above as well?

This comment has been minimized.

@gvanrossum

gvanrossum Jul 13, 2017

Member

So a run with one -v will always show the decision around a particular cache file (every return None from this function, plus specific actions in validate_meta(). The above trace method is not a decision, promoting that would double the number of lines logged per cache file.

@gvanrossum

gvanrossum Jul 13, 2017

Member

So a run with one -v will always show the decision around a particular cache file (every return None from this function, plus specific actions in validate_meta(). The above trace method is not a decision, promoting that would double the number of lines logged per cache file.

Show outdated Hide outdated mypy/build.py
# Optimization: update mtime and path (otherwise, this mismatch will reappear).
meta = meta._replace(mtime=int(st.st_mtime), path=path)
meta = meta._replace(mtime=mtime, path=path)
meta_dict = {

This comment has been minimized.

@ddfisher

ddfisher Jul 13, 2017

Collaborator

It could be worth having a comment explaining the need for this.

@ddfisher

ddfisher Jul 13, 2017

Collaborator

It could be worth having a comment explaining the need for this.

This comment has been minimized.

@gvanrossum

gvanrossum Jul 13, 2017

Member

Good idea, done! Not constructing this dict was the bug here.

@gvanrossum

gvanrossum Jul 13, 2017

Member

Good idea, done! Not constructing this dict was the bug here.

@gvanrossum gvanrossum merged commit 6d75649 into python:master Jul 13, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gvanrossum gvanrossum deleted the gvanrossum:cache-logging branch Jul 13, 2017

gvanrossum added a commit to gvanrossum/mypy that referenced this pull request Jul 18, 2017

Fix bug in cache updates and improve cache logging (#3708)
This started out as an attempt to make the logging messages around cache freshness/updates more useful, but I ended up discovering a bug in the code that updates the cache meta file when the mtime or path of the file has changes but size and source hash are the same. It wrote a list instead of a dict! This is a bug in a feature announced in 0.520 so should be included in 0.521 if/when we release it.

I also decided to ignore debug_cache when comparing options (since it only affects the JSON style -- it's still a per-file option but changing it no longer invalidates the cache)

And I made the --skip-version-check option strictly ignore the platform (since it's undocumented I can make it do what I want, and this is more useful for my one use case).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment