fix impellerc on windows #33003
fix impellerc on windows #33003
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Sorry, added more changes 😓 WriteFileAtomically also had an issue with a sharing violating while trying to copy the temp file to the intended destination. I debugged this a bit and didn't find any obvious leaked fds but could have missed something. I don't actually think copying a file is atomic in windows, so I removed the temp file copy and impellerc works on windows now |
|
|
||
| auto temp_file = | ||
| OpenFile(temp_file_path.c_str(), true, FilePermission::kReadWrite); | ||
| OpenFile(file_path.c_str(), true, FilePermission::kReadWrite); |
There was a problem hiding this comment.
I think what we want here is to use ReplaceFile instead of MoveFile below.
This does not do an atomic write.
There was a problem hiding this comment.
Adding this back I wasn't able to make ReplaceFile work. Really nothing in this method is atomic for windows though ...
There was a problem hiding this comment.
ReplaceFile is supposed to be atomic.
There was a problem hiding this comment.
I mean this entire method, if anything fails we just leave a temp file laying around. Atomic-ish?
There was a problem hiding this comment.
That said, I'm gonna say we don't need atomic writes for a shader compiler
There was a problem hiding this comment.
Should be called WriteAtomicallyIfItSucceeds
There was a problem hiding this comment.
That's fine - the point is that the final write to the destination file is atomic. As long as that's the case we should be good.
There was a problem hiding this comment.
the point of what though, that never worked. This is a PR to make impellerc work on windows. If you want to fix WriteFileAtomically for windows that is certainly worthwhile at some point but I don't think its a good use of my time
There was a problem hiding this comment.
(talked offline - I misunderstood that this was not used yet - I'm fine if we just file a bug with a TODO here to make this be atomic on Windows)
There was a problem hiding this comment.
|
I'm removing the needs tests tag since my un-disabling a test I have sort of added a test :) |
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
LGTM.
I believe we don't need an exception here because the unit test was updated after the bot requested a test so we're good.
jonahwilliams
added
the
waiting for tree to go green
4 participants
OpenFile seems to mostly do the right thing with absolute and relative paths on windows. Not sure what the other use cases might be? we could always do an ifdef.
Fixes flutter/flutter#102805