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

Always write the cache (unless cache_dir is /dev/null) #3133

Merged
merged 6 commits into from
Apr 19, 2017

Conversation

gvanrossum
Copy link
Member

Fixes #3045

(How to test? I tested manually and it seems to work. It's also pretty straightforward.)

Copy link
Contributor

@ambv ambv left a comment

Choose a reason for hiding this comment

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

LGTM, I'd improve the error handling a bit.

self.manager.log("Cached module {} has changed interface".format(self.id))
self.mark_interface_stale()
self.interface_hash = new_interface_hash
if not self.path or self.options.cache_dir == os.devnull:
Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, we can still override writing the cache by setting /dev/null.

👍

@@ -891,7 +891,12 @@ def write_cache(id: str, path: str, tree: MypyFile,
manager.trace("Interface for {} has changed".format(id))
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the replace in :890 be only done if there's no OSError in :895?

mypy/build.py Outdated
try:
st = manager.get_stat(path)
except OSError:
manager.log("Cannot write {}".format(path))
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. We should try to delete the nonced file here (and the data file; it's invalidated by the mismatched data_mtime anyway).
  2. Would also be good to at least stringify the OSError to get an idea why the stat failed.
  3. The error message is misleading. It's a source stat issue, not a cache write issue.

@gvanrossum
Copy link
Member Author

OK, I think I've made all the changes you suggested. I moved the get_stat() call up earlier so we don't even write anything if we can't stat() the source; and I also delete the cache files in that case (not sure if it's even needed now).

@gvanrossum
Copy link
Member Author

A downside is that it slows down regular runs somewhat. (5-10%? I have to collect more serious data.)

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

LGTM

mypy/build.py Outdated
try:
st = manager.get_stat(path)
except OSError as err:
manager.log("Cannot get stat for {}: %s".format(path, err))
Copy link
Member

Choose a reason for hiding this comment

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

Something is wrong with formatting here, this does not fail, but %s is never replaced (at least in Python 3.4 I just tried).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good eye, I am still not in the habit of using {} for format()...

Guido van Rossum added 3 commits April 19, 2017 13:24
There was a Travis-CI test failure, I presume it was due to
runtests.py parallelizing jobs that share the cache?
mypy/build.py Outdated
os.makedirs(parent)
except os.error:
pass # Assume it already exists.
Copy link
Member

Choose a reason for hiding this comment

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

You can just call os.makedirs(parent, exist_ok=True) instead (as of 3.2, https://docs.python.org/3/library/os.html#os.makedirs).

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh thanks! Why this isn't the default behavior I'll never understand (it's Eric Raymond's fault :-).

@gvanrossum gvanrossum merged commit 264715e into master Apr 19, 2017
@gvanrossum gvanrossum deleted the always-write-cache branch April 19, 2017 21:51
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.

5 participants