-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 mypy test temporary config file creation #13620
base: main
Are you sure you want to change the base?
Fix mypy test temporary config file creation #13620
Conversation
tests/mypy_test.py
Outdated
@@ -214,7 +214,7 @@ def run_mypy( | |||
env_vars = dict(os.environ) | |||
if mypypath is not None: | |||
env_vars["MYPYPATH"] = mypypath | |||
with tempfile.NamedTemporaryFile("w+") as temp: | |||
with tempfile.NamedTemporaryFile("w+", delete_on_close=False) as temp: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://docs.python.org/3/library/tempfile.html#tempfile.NamedTemporaryFile
[...] in case the temporary file is created in a with statement, set the delete_on_close parameter to be false. The latter approach is recommended as it provides assistance in automatic cleaning of the temporary file upon the context manager exit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's not usable until Python 3.12 . Hopefully delete=False
is fine...
I think it should be fine as long as the mypy call closes the file after reading it:
Opening the temporary file again by its name while it is still open works as follows:
- On POSIX the file can always be opened again.
- On Windows, make sure that at least one of the following conditions are fulfilled:
- delete is false
- additional open shares delete access (e.g. by calling os.open() with the flag O_TEMPORARY)
- delete is true but delete_on_close is false. Note, that in this case the additional opens that do not share delete access (e.g. created via builtin open()) must be closed before exiting the context manager, else the os.unlink() call on context manager exit will fail with a PermissionError.
tests/mypy_test.py
Outdated
@@ -214,7 +214,8 @@ def run_mypy( | |||
env_vars = dict(os.environ) | |||
if mypypath is not None: | |||
env_vars["MYPYPATH"] = mypypath | |||
with tempfile.NamedTemporaryFile("w+") as temp: | |||
# TODO: Use delete_on_close on Python 3.12 | |||
with tempfile.NamedTemporaryFile("w+", delete=False) as temp: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will pollute the developer's temp folder with mypy config files.
Are you sure that this change is needed? As far as I can tell, the file is only needed while mypy is running (i.e. we are inside the with
block), so there's no need to pass delete=False
.
I can't reproduce your No [mypy] section in config file
error locally. I added x = asoijfasoif
to the end of stubs/setuptools/setuptools/monkey.pyi
, and this is what I get:
akuli@akuli-desktop:~/typeshed$ python3 tests/mypy_test.py stubs/setuptools -p 3.13
*** Testing Python 3.13 on linux
Testing third-party packages...
testing setuptools (104 files)... failure (exit code 1)
stubs/setuptools/setuptools/monkey.pyi:17: error: Name "asoijfasoif" is not defined [name-defined]
--- 1 package with errors, 104 files checked ---
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a Windows-only issue, I don't get it on WSL either. Likely due to those quirks of tempfile.NamedTemporaryFile
mentioned in the documentation.
And yeah that leaves the file behind, maybe I mixed up my tests because I thought I checks for that...
Edit: delete_on_close
doesn't leave files behind, it just changes when the deletion happens.
Here's the relevant CPython PR: python/cpython#97015 and original issue: python/cpython#58451
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe try the following:
- Add a manual
os.remove()
or similar to clean up the file, make sure it works - Move the
os.remove()
as close to the end of thewith
statement as possible - Figure out why it cannot be simply at the end of the
with
statement (default behavior ofNamedTemporaryFile
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember getting this issue when I used Windows as well FWIW; can confirm that it's not an issue specific to Avasam's environment :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could place a regular file inside the td
folder, created in the main()
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I know why it's a windows issue (after reading the docs): To read the tempfile, you need to first close the tempfile. But the tempfile is immediately deleted on close. Unless you "call os.open() with the flag O_TEMPORARY
", but mypy doesn't do that.
So on Pyton >= 3.12, I could just use delete_on_close=False
. Before that on Windows specifically I have to pass delete=False
and manually delete the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think TemporaryDirectory()
has much more reliable cross-platform behaviour than the other tempfile
functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah those comments loaded after I wrote my last answer.
I wrapped NamedTemporaryFile
, but it's true there's already a temporary directory that I could try to pass further along. (test_third_party_stubs
doesn't pass tempdir
to test_third_party_distribution
, which is what eventually calls run_mypy
)
# We need to work around a limitation of tempfile.NamedTemporaryFile on Windows | ||
# For details, see https://github.com/python/typeshed/pull/13620#discussion_r1990185997 | ||
# Python 3.12 added a workaround with `tempfile.NamedTemporaryFile("w+", delete_on_close=False)` | ||
if sys.platform != "win32": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If any contributor runs this on win, it will be a NameError
to access _named_temporary_file
. I guess we have to explicitly say that win32
platform is not supported for running tests.
For a long while now I noticed that when the mypy test fails,
No [mypy] section in config file
is printed, but I never really paid attention to it. Turns our the temporary mypy config file was never actually being written to.I also noticed an additional bug where the same config section would be repeated a lot.
I have an upcoming experimental PR that depends on the
[mypy-tests]
feature, so here you go with some bugfixes and removing a bogus error line that affects current tests.When there's a mypy error:
Before:
After:
On top of the above fix, with the following in METADATA.toml:
Before (content written to temp file):
After (content written to temp file):