Force removing read-only files on Windows with FilePath.remove()
#3588
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
On Windows, `_unlink()` cannot remove a file if it is read-only. This change calls `_chmod(..., _S_IWrite)` on files that are about to be removed so as to make sure they are removable. This fixes an issue with temp directories being left around from tests in Corral, as Git often makes some of its files read-only.
chalcolith
added
the
changelog - fixed
SeanTAllen
reviewed
Jul 10, 2020
packages/files/_test.pony
Outdated
| @@ -473,7 +474,7 @@ class iso _TestFileCreateExistsNotWriteable is _NonRootTest | |||
| mode.owner_read = true | |||
| mode.owner_write = true // required on Windows to delete the file | |||
| filepath.chmod(mode) | |||
| filepath.remove() | |||
| h.assert_true(filepath.remove()) | |||
There was a problem hiding this comment.
seems like this test should be updated to match _TestFileRemoveReadOnly in terms of the try/then and asserting both chmod and remove.
There was a problem hiding this comment.
I've updated it a bit.
SeanTAllen
reviewed
Jul 10, 2020
packages/files/file_path.pony
Outdated
| @@ -223,6 +223,9 @@ class val FilePath | |||
| if info.directory and not info.symlink then | |||
| 0 == @_rmdir[I32](path.cstring()) | |||
| else | |||
| // _unlink() on Windows can't remove readonly files | |||
| // so we change mode to _S_IWRITE just in case | |||
| @_chmod[I32](path.cstring(), I32(0x0080)) | |||
There was a problem hiding this comment.
does this need to be done before line 224 for directories as well?
There was a problem hiding this comment.
Oops, I put the chmod() in the wrong spot. Fixed.
|
@kulibali drop me a note when you are ready for me to give this another look. |
|
@SeanTAllen should be good to go now. |
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.
On Windows,
_unlink()cannot remove a file if it is read-only. This change calls_chmod(..., _S_IWrite)on files that are about to be removed so as to make sure they are removable.This fixes an issue with temp directories being left around from tests in Corral, as Git often makes some of its files read-only.