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.

Sign up for GitHub

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

Jump to bottom

os: RemoveAll no longer returns *os.PathError #30491

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

os: RemoveAll no longer returns *os.PathError #30491

colincross opened this issue Mar 1, 2019 · 12 comments
Labels
CherryPickCandidate Used during the release process for point releases FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.12.2

Comments

@colincross
Copy link
Contributor

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
@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 1, 2019
@gopherbot
Copy link

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

@oiooj
Copy link
Member

oiooj commented Mar 1, 2019

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

@ianlancetaylor
Copy link
Contributor

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
Copy link
Contributor

bradfitz commented Mar 1, 2019

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

@dgryski
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 bradfitz added the CherryPickCandidate Used during the release process for point releases label Mar 1, 2019
@bradfitz
Copy link
Contributor

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
Copy link
Member

julieqiu commented Mar 12, 2019 •
edited

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

@ianlancetaylor
Copy link
Contributor

@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
@oiooj
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
Copy link
Contributor

@gopherbot, please backport to Go 1.12.

@gopherbot gopherbot mentioned this issue Mar 15, 2019
Closed
@gopherbot
Copy link

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
Copy link

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
@oiooj @bradfitz
[release-branch.go1.12] os: consistently return PathError from RemoveAll
ff04803 
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>
@golang golang locked and limited conversation to collaborators Mar 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
CherryPickCandidate Used during the release process for point releases FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.12.2
Development

No branches or pull requests
8 participants
@bradfitz @andybons @dgryski @colincross @oiooj @ianlancetaylor @julieqiu @gopherbot