Fix CacheDir initialization so works on 3.7 also #4695
Merged
+56
−29
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Although there is no indication of a change in this area of the tempfile module, it seems if the temporary directory has been renamed so the original no longer exists, the context manager fails on 3.7, though not on any later Python versions (maybe a bugfix rather than a planned behavior change?). Now use mkdtemp instead, and clean it up by hand if the rename failed (also use os.replace instead of os.rename, so it will actually fail if the directory existed and is nonempty, on Linux os.rename doesn't fail in this case). Fixes SCons#4694 Signed-off-by: Mats Wichmann <[email protected]>
|
FWIW: all 30 CacheDir related tests that failed in #4694 pass for all python versions 3.7 to 3.13 in both linux and Windows with the first commit of this PR locally. |
|
Thanks for that update! |
Mainly restores VS 2022 image builds, which were commented out. If they're still broken, we can revert. Signed-off-by: Mats Wichmann <[email protected]>
bdbaddog
reviewed
Mar 12, 2025
The failure on Python 3.7 caused a rewrite. Redo that so the version released with SCons 4.9.0 is still present, but commented out, so it's easier to restore it in future. tempfile.TemporaryDirectory has better cleanup logic, so we'll want to flip to that when Python 3.7 support is retired. Signed-off-by: Mats Wichmann <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Although there is no indication of a change in this area of the
tempfilemodule, it seems if the temporary directory has been renamed so the original no longer exists, the context manager fails on 3.7, though not on any later Python versions (maybe a bugfix rather than a planned behavior change?). Now usemkdtempinstead, and clean it up by hand if the rename failed (also useos.replaceinstead ofos.rename, so it will actually fail if the destination directory existed and is nonempty, on Linuxos.renamedoesn't fail in this case).Fixes #4694
Contributor Checklist:
CHANGES.txtandRELEASE.txt(and read theREADME.rst).