Fix crash when checking file in a read only directory - #21496
Conversation
Only applies to new SqliteMetadataStore, which is the default since 1.20; old FilesystemMetadataStore doesn't crash. Fixes python#21495
|
|
||
| @unittest.skipIf( | ||
| sys.platform == "win32", | ||
| "POSIX chmod semantics: os.chmod(dir, 0o555) does not prevent writes on Windows", |
There was a problem hiding this comment.
Wasn't sure what's better -- excluding test altogether, or checking the same thing after store = SqliteMetadataStore(cache_dir) and returning early/skipping in the test body.
I figured excluding it altogether is less confusing.
| os.makedirs(cache_dir_prefix, exist_ok=True) | ||
| try: | ||
| os.makedirs(cache_dir_prefix, exist_ok=True) | ||
| except OSError: |
There was a problem hiding this comment.
Perhaps a bit too broad, but catching all sort of possible FS read-only states can be difficult, and also this is consistent with what FilesystemMetadataStore was doing..
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
As per our policy we don't accept LLM generated PRs from new contributors. |
|
Hmm, I mentioned I just used LLM to bisect the bug and suggest consistency tips. I guess I never contributed actual code to mypy so far, but did participate in issues and discussions since 2019 if it helps to prove that I'm a human :) https://github.com/python/mypy/issues?q=involves%3A%40karlicoss%20sort%3Acreated-asc |
|
OK, I will re-open this PR, but can't guarantee a fast review (as we weren't fast even before the recent PR influx). |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
2 participants
Only applies to new SqliteMetadataStore, which is the default since 1.20; old FilesystemMetadataStore doesn't crash.
Fixes #21495
I added a test for
SqliteMetadataStore-- a bit surprised there weren't any in the first place, perhaps the idea is that this is already covered bycmdlinebased integration tests? I had a stab at implementing one for consistency, but realized that there is no way to issue achmod -wthere, so figured a more specialized unit test is fine. Let me know if there is a better way to test this!I used Claude to bisect when issue started happening and to suggest me best place to place code and make it consistent with existing testbase; but otherwise fully understand it!