Fix sharing violations in nob_needs_rebuild on Windows - #272
Open
fartsicle wants to merge 1 commit into
Open
Conversation
This file contains hidden or 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
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.
Edit: The most accurate / updated / recent information is in this comment on a similar PR: #246 (comment) So note that some stuff in this PR is slightly off in understanding / not the absolute best solution, although it would fix the main set of problems. The comment references this PR though in a chain of discussion so it would be best to read this PR anyways.
While submitting this, I noticed there's a similar PR to this here: #269
However I wouldn't use that one, since it changes the semantics of the program. But let me explain.
To start with a familiar / intuitive explanation, is that this code takes our current method of reading the timestamp on Windows to be more like what you'd expect of
staton unix, which is what nob uses there. The fact that it's not like stat is basically the problem. The main problem with the other PR is that it makes it behave likelstatsimply becauseGetFileAttributesExdoesn't follow symlinks and there's no way around it AFAIK.As to the specifics, the problem on Windows is when we open a file we currently pass
0to the third param of CreateFile,dwShareMode, and this essentially asks Windows for exclusive access to the file while we have this handle. What this means is, we need anyone who either already has a handle to this file or will during the duration of our handle, to not have permission to read, write, or delete the file (note that moves require delete access). Which is fairly prohibitive.For example, as a result of this I cannot debug nob on RAD Debugger because for nob to get the timestamp for its own executable, Windows does a shared permissions check and sees RADD's debug handle which needs read access as in conflict with our exclusive ask.
For specifically my example above, just passing
FILE_SHARE_READtodwSharemodeparam should fix it, but I thought we should pass the other available permissions,FILE_SHARE_WRITEandFILE_SHARE_DELETE, as well since it would match the behavior of unix'sstatAFAIK. But doing this just enables more flexibility, since any more "serious" problems, like whether we actually can rebuild, e.g. nob overwriting an exe that is in use/mapped without first moving it in a similar fashion it does to itself, Windows would handle later in the "nob pipeline."Allowing that kind of metadata access even though you later can't actually "build it" personally lets me build some more contrived bullshit on top of nob that does stuff similar to the patented
Go Rebuild Urself™ Technologyfor things other than nob.Thank you for taking a look at this Mr. Tsoding. /salute
--Fart