Skip to content
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

os: Remove/RemoveAll should remove read-only files on Windows #9606

Closed
adg opened this issue Jan 15, 2015 · 38 comments
Closed

os: Remove/RemoveAll should remove read-only files on Windows #9606

adg opened this issue Jan 15, 2015 · 38 comments

Comments

@adg
Copy link
Contributor

@adg adg commented Jan 15, 2015

On Unix systems, Remove/RemoveAll will work even if the files are chmod 0600. On windows, Remove/RemoveAll will not remove a file marked as read-only. If you want to write a Go program that deletes a file, you need to take special care on windows. That's sad.

Since we determine the semantics of Remove/RemoveAll, we could make this "just work" and save our users some effort.

Discussion started here: https://go-review.googlesource.com/2930

@minux minux added this to the Go1.5 milestone Jan 16, 2015
@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Jan 16, 2015

Code change is easy. Any objections to this idea?

We will need to call out this change in the Go 1.5 release note, but
I don't think it should surprise users.

BTW, once this is in, we can remove the workaround in CL 2930 and
CL 2089 (resetReadOnlyFlagAll function).

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jan 16, 2015

I object. I think there is confusion here about what Windows file "read-only" attribute does. It not just prevents from making changes to the contents of the file. It also prevents from the file to be deleted.

C:\tmp\aaa>echo > a.txt

C:\tmp\aaa>attrib +r a.txt

C:\tmp\aaa>del a.txt
C:\tmp\aaa\a.txt
Access is denied.

C:\tmp\aaa>attrib -r a.txt

C:\tmp\aaa>del a.txt

C:\tmp\aaa>

On linux it would be similar to:

$ ls
$ mkdir a
$ touch a/f1
$ touch a/f2
$ ls -l
total 4
drwxr-xr-x 2 brainman brainman 4096 Jan 16 10:54 a
$ rm a/f1
$ chmod a-w a
$ ls -l
total 4
dr-xr-xr-x 2 brainman brainman 4096 Jan 16 10:54 a
$ rm a/f2
rm: cannot remove `a/f2': Permission denied
$ chmod u+w a
$ ls -l
total 4
drwxr-xr-x 2 brainman brainman 4096 Jan 16 10:54 a
$ rm a/f2
$ rmdir a
$

Basically you have to change the flag before you will be allowed to delete the file.

Also some explanation from http://www.oreilly.com/openbook/samba/book/ch05_03.html

DOS filesystems are not designed for multiple users, and so its designers decided that read-only means "protected against accidental change, including deletion," rather than "protected against some other user on a single-user machine." So the designers of DOS prohibited removal of a read-only file. Even today, Windows file systems exhibit the same behavior.
<<<

I don't see wining this argument, but I am trying.

Alex

@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Jan 16, 2015

You're still talking about Windows file deletion semantics, but
the os package is not bound by Windows file deletion semantics.

To put it another, should the API refrain from deleting any file not
writable by the current user on other platforms?

Why should we have this distinction on the supposedly portable
os package.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jan 16, 2015

Your argument is "os.Remove must make every effort to delete the file". Isn't it?

But, if we assume that premise, then we should do the same on every platform. For example in my example above:

$ rm a/f2
rm: cannot remove `a/f2': Permission denied

should we try and change 'a' directory mode and try to delete a/f2 again? Because that is what you're effectively proposing for windows os.Remove.

To me it is all about being reasonable here. The "read-only" flag is set for a reason. Lets not delete the file until we understand that reason.

To put it another, should the API refrain from deleting any file not
writable by the current user on other platforms?

I say we should leave thing as they are. We use "standard" system API. Whatever it does is most correct for general case scenario.

Alex

@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Jan 16, 2015

We can change the permission of the file itself, but we shouldn't
change the permission of its parent. That's the border line.

@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Jan 16, 2015

I say we should leave thing as they are. We use "standard" system API.
Whatever it does is most correct for general case scenario.

In that case, let's change the documentation for os.Remove to:

Remove just calls the standard os remove (delete) file to remove the file
or directory. It does what the standard OS-specific API does.

Is that acceptable to you? This is essentially your position.

@mattn

This comment has been minimized.

Copy link
Member

@mattn mattn commented Jan 16, 2015

I also do not agree to this change. It's overwork. If you want, let's put makerelease_windows.go and add func removeAll.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jan 16, 2015

I am fine with documentation change. But I think that everyone understands that OS file / permissions might prevent you from removing file. No one complaining about inability to remove opened file on windows. Same for removing current directory.

The problem is us. We assume that os package provides same behaviour everywhere. But sometimes it cannot.

Alex

@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Jan 16, 2015

Why it's overwork?

When I write os.Remove(filename), I truly want it to delete the file.

I don't want to write system specific code for each code, otherwise
what's the point of providing os.Remove?

Why not just force the user to use syscall.Unlink and syscall.DeleteFile
instead? Is providing os.Remove overwork too?

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jan 16, 2015

@minux

When I write os.Remove(filename), I truly want it to delete the file.

I think the real issue we should be discussing here (if we care), is why these files have "read-only" flag. Who set the flag and why? Perhaps, once we understand the reasoning, we can decide what to do with the issue at hand.

If we care.

Alex

@mattn

This comment has been minimized.

Copy link
Member

@mattn mattn commented Jan 16, 2015

In generally, we have to do care for windows about file deletion. For example: when some files are opened by another applications, so the file is locked with opening file, the deletion of file should be fail. This changing spec will solve this issue. But we will get another issue. So I'm thinking we have better not do.

@tv42

This comment has been minimized.

Copy link

@tv42 tv42 commented Jan 16, 2015

Unix chmod a-w on the file itself is irrelevant for removing it; unix controls removing by directory permissions.

The Windows file read-only flag is more like chattr +i (immutable) on Linux, and such a file "cannot be deleted or renamed" (see chattr(1)).

I don't think os.Remove/RemoveAll should do anything extra to make the deletion succeed. What next, change ACLs to allow the delete? That road leads to madness.

@adg

This comment has been minimized.

Copy link
Contributor Author

@adg adg commented Jan 16, 2015

On 16 January 2015 at 12:33, mattn notifications@github.com wrote:

I also do not agree to this change. It's overwork. If you want, let's put
makerelease_windows.go and add func removeAll in makerelease.go.

We already have two instances of work-arounds for this issue in the Go
repositories. That's what motivated this suggestion.

@mattn

This comment has been minimized.

Copy link
Member

@mattn mattn commented Jan 16, 2015

I don't think os.Remove/RemoveAll should do anything extra to make the deletion succeed. What next, change ACLs to allow the delete? That road leads to madness.

I think so. +1

@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Jan 16, 2015

Changing ACL is obviously out-of-question.

However, I don't think windows read-only flag is equivalent to immutable
flag on linux.

For example, why should git label its pack/idx files immutable? Does git do
this on
linux?

When I use os.RemoveAll, should I remember to copy alex's
resetReadOnlyFlagAll
function so that my code will continue to work on Windows?

Addressing this issue will not make os.Remove able to remove file currently
in use
by another process.

@adg

This comment has been minimized.

Copy link
Contributor Author

@adg adg commented Jan 16, 2015

On 16 January 2015 at 13:11, Minux Ma notifications@github.com wrote:

For example, why should git label its pack/idx files immutable? Does git do
this on
linux?

If you look in .git/objects you'll see all the files are 0222.

@mattn

This comment has been minimized.

Copy link
Member

@mattn mattn commented Jan 16, 2015

Addressing this issue will not make os.Remove able to remove file currently in use by another rocess.

There is a possibility that another problem occurs even to solve this problem. So I don't want to break/change os.Remove/RemoveAll. Let's make another function. os/osutil ?

@adg

This comment has been minimized.

Copy link
Contributor Author

@adg adg commented Jan 16, 2015

On 16 January 2015 at 13:13, Andrew Gerrand adg@golang.org wrote:

If you look in .git/objects you'll see all the files are 0222.

And of course I mean 0444.

@adg

This comment has been minimized.

Copy link
Contributor Author

@adg adg commented Jan 16, 2015

On 16 January 2015 at 13:15, mattn notifications@github.com wrote:

Let's make another function. os/osutil ?

That doesn't solve the problem. The problem is that os.Remove deletes
read-only files on Unix but not on Windows.

@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Jan 16, 2015

On Thu, Jan 15, 2015 at 9:14 PM, Andrew Gerrand notifications@github.com
wrote:

On 16 January 2015 at 13:11, Minux Ma notifications@github.com wrote:

For example, why should git label its pack/idx files immutable? Does git
do
this on
linux?
If you look in .git/objects you'll see all the files are 0222.

Right (assuming you meant 0444). I was arguing that the readonly flag on
windows
is not equivalent to immutable on linux. At least developers don't think
they are the
same.

@StefanSchroeder

This comment has been minimized.

Copy link

@StefanSchroeder StefanSchroeder commented Jan 16, 2015

What about having a Remove() function that respects the readonly/immutable flag and a RemoveForce() that does every effort to delete it.

@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Jan 16, 2015

If we provide two functions to do roughly the same thing, which should the
user choose?

If I say os.Remove(file), I really want file to be removed, so if we provide
two functions, I will have to change all my os.Remove to os.RemoveForce.
Then what's the point of providing two?

I really don't understand why someone would say os.Remove and expect
the OS to prevent it. If you want to preserve read-only files on Windows,
you should do that yourselves, just as you would have to do on other OSes.

I think most of uses of os.Remove is to remove temporary files. And we
have got at least one example where external application not under our
control marks its files as read-only. Why should the programmer care about
that when trying to delete temporary files created by such applications?
(you might argue that those applications are using the readonly flag wrong,
but that's beyond the scope of this issue, and I see no clear alternatives
that they can change to.)

Anyway, we control the definition of os.Remove, and it doesn't say it will
conform to what the OS-specific remove file API would do, so I believe
we can fix this issue.

@tv42

This comment has been minimized.

Copy link

@tv42 tv42 commented Jan 16, 2015

"Why should the programmer care" -- because the people who designed the OS thought that the distinction matters? http://msdn.microsoft.com/en-us/library/windows/desktop/aa363915(v=vs.85).aspx

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jan 16, 2015

because the people who designed the OS thought that the distinction matters

That's a weak argument. That would disallow all portable interfaces and languages which abstracted over per-CPU and per-OS differences.

The whole point of having a high-level language and package like "os" is to let me do things consistently over many operating systems and architectures and knowing it'll work the same everywhere.

If I wanted Windows' DeleteFile semantics, I'd use syscall.DeleteFile directly. But I don't. I want os.Remove semantics, like I'm used to everywhere else.

@jonnenauha

This comment has been minimized.

Copy link

@jonnenauha jonnenauha commented Jul 2, 2015

No, I've made my own golang/js repo build server from scratch. Just added the work around and can finally run builds on windows with proper disk cleanup at the end :) I don't really run the server on windows except when developing it, where disk cleanup is not that important. Lucky thing that I was looking at 1.5 milestone issues for no real reason, just to find this.

Notes if anyone gets this same one in the futures: I ended up using the cmd.exe work around. "del" did not work as I wanted to remove directory that had 1-N git repos inside it down the tree. The dir recursion to change the read flag is potentially heavy if you have big repos.

cmd := exec.Command("cmd.exe", "/C", "rmdir", "/S", "/Q", dirPathWithWindowsSeparators)

Ended up using that. Just make sure the folder exists or you'll get an error from cmd.Run().

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jul 2, 2015

There is an example in 3b7841b

Alex

@frederich

This comment has been minimized.

Copy link
Contributor

@frederich frederich commented Oct 27, 2015

1+

This issue makes github.com/google/codesearch unusable on Window.

@rsc rsc modified the milestones: Proposal, Go1.6Early Nov 24, 2015
@rsc rsc changed the title os: Remove/RemoveAll should remove read-only files on Windows proposal: os: Remove/RemoveAll should remove read-only files on Windows Nov 24, 2015
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 24, 2015

Moving to proposal process, hopefully for Go 1.7.

@rsc rsc added the Proposal label Nov 24, 2015
@haliphax

This comment has been minimized.

Copy link

@haliphax haliphax commented Dec 11, 2015

+1 - This is preventing Go programs from removing directory structures created by Git (as an example), since many of the files are RO.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jan 4, 2016

@maruel, if you are still willing to cook up a CL, I'd like to see it. No rush - we won't start comitting Go 1.7 CLs until February, but if you get it queued up I'll be happy to review it sooner.

@maruel

This comment has been minimized.

Copy link
Contributor

@maruel maruel commented Jan 4, 2016

Will look this week.

@maruel

This comment has been minimized.

Copy link
Contributor

@maruel maruel commented Jan 5, 2016

https://go-review.googlesource.com/#/c/18235 can be reviewed once the merge window is open again.

unknwon referenced this issue in gogs/gogs Feb 8, 2016
Workaround delete folder on Windows.
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 18, 2016

CL https://golang.org/cl/18235 mentions this issue.

@bradfitz bradfitz changed the title proposal: os: Remove/RemoveAll should remove read-only files on Windows os: Remove/RemoveAll should remove read-only files on Windows May 11, 2016
@gopherbot gopherbot closed this in 2ffb3e5 May 11, 2016
@golang golang locked and limited conversation to collaborators May 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.