Fix CacheDir initialization so works on 3.7 also#4695
Conversation
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 <mats@linux.com>
|
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 <mats@linux.com>
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 <mats@linux.com>
|
@mwichmann Stupid question follows... The following CacheDir tests fail on a windows box with no compiler installed:
Is that the expected behavior? |
|
@jcbrill - Is that failure new after this PR? Or was it always failing when no compiler installed? |
I'd say yes, that would be expected. Both tests have a place where they call |
|
Looking at the code for one of the tests, it would have always failed when no compiler is installed. The final test in If there are no known compilers, msvc is assumed under windows. I'm not sure there is an easy way to skip a test if there are no compilers installed at least under windows. |
|
I have a few Windows VMs with only windows SDKs installed that I usually run just the msvs/msvc tests to make sure that the msvc compilers are not detected. The CacheDir tests were setup in some test batch files that I was using to update python virtual environments and failed the two tests above. It's not a big deal. |
There's an |
Unfortunately that doesn't tell you if mingw (or other valid windows compiler) is installed. It would have to be no msvc and the default compiler is msvc. Although, then you would have to know if it was requested by a user and just doesn't exist. |
|
No, it's not trivial to guess beforehand what SCons is going to find once it's actually called to run the test. Various tests have all kinds of tricks to cheat some of that, but it's never ideal. |
|
I'm ok with "if you want all the tests to pass, you need to have a compiler installed". As described above, in the VMs with no compiler, I'm usually running a limited set of tests for msvs/msvc just to verify that a usable msvc compiler is not detected. Sorry for the bother. |
|
Curious, what happens to Not really ideal to be conducting a chat in a merged/closed PR, but I guess it works... |
On windows without a compiler, msvc is forced to be the default compiler.
Yeah. At it's core, this didn't seem to be a bug. Just call me the "Necromancer". Edit: execution times are unreliable due to being run in a VM hosted on an external drive and with windows installing updates. |
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).