-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Harden against windows-style accessdenied bs on other platforms when renaming build output from tmp #25838
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
base: master
Are you sure you want to change the base?
Conversation
|
This code does not work with the C backend or the new linkers as currently written. Therefore, it would be nice to move the entirety of this workaround into linker functions where it can be implemented properly. It would also be nice to defer this workaround until actually getting an error so that filesystems without this issue are not penalized. |
|
@jacobly0 somewhat surprised to hear that, since the workaround code is used as-is on windows already and i wouldnt expect either the C backend or the new linker to be disabled on windows. as for moving the workaround into linker functions and/or deferring it until an error actually occurs, I agree with those in principle and will look into making that change |
|
Ill work on fixing and refactoring this to be properly integrated into the linker implementations sometime this weekend, hopefully removing the workaround in the process |
|
refactored to clean up and hopefully fixed the C backend issue - it also now only uses the workaround if it gets an error the first try at renaming the directory. having some weird errors locally with incremental that im having trouble isolating though, so not quite ready yet |
|
This PR doesnt solve every issue that occurs on WSL, but it does make non-incremental builds work and be resilient to windows-like shenanigans without losing perf on targets that dont have that issue (by trying without the close/reopen nonsense first and only using the workaround if it fails - though on windows the workaround is still dont immediately because windows has this as a documented thing). As for ramining work, unless theres some desire to make WSL a first class target there shouldnt be any so long as incremental isnt used for compiling build scripts, but that issue is so far in the weeds that I dont even know what to think anymore. |
Fixes #24586, fixes #24955.
This pr changes the AccessDenied during renaming output directory workaround to be run on all platforms as a defensive measure, since the overhead of the workaround is very minimal, and we cant be certain that there isnt some other niche case where the issue appears in addition to the one niche target that does - WSL on a ReFS filesystem.
(Ive been trying to get the proprietary ReFS driver for linux to see if this is needed in that case but thats even more niche).
The update to line 3194 is only needed to make it typecheck correctly, the main change of the PR is removing the if checking for windows.