os.remove on Windows could fail if files are still in use #554
Conversation
| # unrelated to the test session about to start. | ||
| pass | ||
| yield | ||
| # No cleanup needed after yield anymore |
There was a problem hiding this comment.
That's a great move.
What do you think about adding something like this?
import logging
logger = logging.getLogger(__name__)
...
except PermissionError as e:
logger.warning(f"Could not remove file {f!r}: {e}")
That way, if a file is in use unexpectedly, we’ll at least have breadcrumbs.
We should probably also log the FileNotFoundError.
| @pytest.fixture(scope="session", autouse=True) | ||
| def clean_up_cffi_files(): | ||
| yield | ||
| # Clean up files from previous runs before the session starts | ||
| files = glob.glob(os.path.join(os.getcwd(), "_cpu_obj*")) |
There was a problem hiding this comment.
Because of issues like the Windows cleanup issue, would highly recommend using pytest's builtin temporary directory management fixtures
This will handle all the directory creation and cleanup for us with less fuss
There was a problem hiding this comment.
+1 — if we have the free bandwidth
There was a problem hiding this comment.
Simpler, more reliable cleanup of the test directory will lead to bandwidth savings in the long term ;)
There was a problem hiding this comment.
I've explained this in a past group meeting, repeating here for wider visibility. pytest fixtures like tmp_path etc unfortunately do not work. The temp files are generated by CFFI inside a code sample (here), and we have no way to influence where it places the modules (without making the code sample overly complicated) I am afraid. I'd be happy to be proven wrong!
|
@kshitiz305 unfortunately, at this time we are unable to accept code contributions as mentioned in https://github.com/NVIDIA/cuda-python/blob/main/CONTRIBUTING.md. Based on this I am going to close this PR. We hope to change this in the future. |
Hi @kshitiz305, a quick update: We've recently opened up for external contributions to |
Description
Closes #535
Checklist