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: RemoveAll no longer returns *os.PathError #30491

Closed
colincross opened this Issue Mar 1, 2019 · 12 comments

Comments

Projects
None yet
8 participants
@colincross
Copy link
Contributor

colincross commented Mar 1, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"

What did you do?

In go 1.11, os.RemoveAll returned an *os.PathError when it failed to remove something due to a permissions issue. While not documented in the godoc for os.RemoveAll, this behavior was useful to implement a retry loop that attempted to chown the failing path and then call os.RemoveAll again (for example, https://android.googlesource.com/platform/build/soong/+/ef36053829238e24088c578eeac08a1693177757/ui/build/util.go#74). In go 1.12 os.RemoveAll returns a syscall.Errno instead, which doesn't tell the caller which path failed.

This test passes on go 1.11 but fails on go 1.12:

package main

import (
	"io/ioutil"
	"os"
	"path/filepath"
	"testing"
)

func TestRemoveAllPathError(t *testing.T) {
	tmpDir, err := ioutil.TempDir("", "")
	if err != nil {
		t.Fatal(err)
	}
	defer func() {
		err := os.RemoveAll(tmpDir)
		if err != nil {
			t.Errorf("Error removing tmpDir: %v", err)
		}
	}()

	a := filepath.Join(tmpDir, "a")
	b := filepath.Join(a, "b")

	os.Mkdir(a, 0755)
	os.Mkdir(b, 0755)
	os.Chmod(a, 0555)
	defer os.Chmod(a, 0755)

	err = os.RemoveAll(a)
	if pathErr, ok := err.(*os.PathError); ok {
		if pathErr.Path != b {
			t.Errorf("expected pathErr.Path %q, was %q", b, pathErr.Path)
		}
	} else {
		t.Errorf("expected os.PathError, got %T", err)
	}
}

On go 1.11 this test passes, but fails on go 1.12:

--- FAIL: TestRemoveAllPathError (0.00s)
    removeall_test.go:36: expected os.PathError, got syscall.Errno

Should os.RemoveAll always return a *os.PathError?

@ianlancetaylor ianlancetaylor added this to the Go1.12.1 milestone Mar 1, 2019

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 1, 2019

Change https://golang.org/cl/164720 mentions this issue: os: fix os.RemoveAll no longer returns *os.PathError

@oiooj

This comment has been minimized.

Copy link
Member

oiooj commented Mar 1, 2019

I think we need to add a release-blocker label. @ianlancetaylor

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Mar 1, 2019

I don't think this is technically a release blocker.

@bradfitz bradfitz changed the title os.RemoveAll no longer returns *os.PathError os: RemoveAll no longer returns *os.PathError Mar 1, 2019

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Mar 1, 2019

Also, the release is already out. What would it block?

@dgryski

This comment has been minimized.

Copy link
Contributor

dgryski commented Mar 1, 2019

@bradfitz I think the point it be in 1.12.1 or 1.13 on the assumption that changing the returned error type in 1.12 (which the documentation doesn't guarantee anyway, unlike some other calls) is an important API breakage.

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Mar 1, 2019

Oh, if that was a request to cherry-pick the fix to Go 1.12.x, I agree. I'll let @ianlancetaylor approve.

@julieqiu

This comment has been minimized.

Copy link

julieqiu commented Mar 12, 2019

friendly ping @ianlancetaylor - waiting on your approval for this cherry pick candidate

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Mar 12, 2019

@julieqiu There is nothing to cherry pick yet.

I agree that if the fix to master is sufficiently simple and safe then it should be cherry picked to the 1.12 branch. But at least to me it doesn't make sense to approve a cherry pick without knowing what we are cherry picking.

@andybons andybons modified the milestones: Go1.12.1, Go1.12.2 Mar 14, 2019

@gopherbot gopherbot closed this in d039e12 Mar 15, 2019

@oiooj

This comment has been minimized.

Copy link
Member

oiooj commented Mar 15, 2019

@ianlancetaylor It is necessary to cherry picked to the 1.12 branch? And I can send a CL to 1.12 release branch later.

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Mar 15, 2019

@gopherbot, please backport to Go 1.12.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 15, 2019

Backport issue(s) opened: #30859 (for 1.12).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 15, 2019

Change https://golang.org/cl/167739 mentions this issue: [release-branch.go1.12] os: consistently return PathError from RemoveAll

gopherbot pushed a commit that referenced this issue Mar 15, 2019

[release-branch.go1.12] os: consistently return PathError from RemoveAll
Fixes #30859
Updates #30491

Change-Id: If4070e5d39d8649643d7e90f6f3eb499642e25ab
Reviewed-on: https://go-review.googlesource.com/c/go/+/164720
Run-TryBot: Baokun Lee <nototon@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
(cherry picked from commit d039e12)
Reviewed-on: https://go-review.googlesource.com/c/go/+/167739
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Baokun Lee <nototon@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.