From 79224910301cfe63a399ae73e3ed1fa855a3868b Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Fri, 15 May 2020 13:55:58 -0400 Subject: [PATCH] go/packages/packagestest: make Export skip tests involving unsupported links Also make Export create all parent directories before all files. If the files are symlinks to directories, the target directory must exist before the symlink is created on Windows. Otherwise, the symlink will be created with the wrong type (and thus broken). Fixes golang/go#46503 Updates golang/go#38772 Updates golang/go#39183 Change-Id: I17864ed66e0464e0ed1f56c55751b384b8c59484 Reviewed-on: https://go-review.googlesource.com/c/tools/+/234112 Trust: Bryan C. Mills gopls-CI: kokoro Run-TryBot: Bryan C. Mills Reviewed-by: Ian Cottrell --- go/packages/packagestest/export.go | 167 ++++++++++++++++++++++-- go/packages/packagestest/export_test.go | 2 +- internal/imports/fix_test.go | 11 -- 3 files changed, 156 insertions(+), 24 deletions(-) diff --git a/go/packages/packagestest/export.go b/go/packages/packagestest/export.go index 6b03926d923..2b93d2c2b95 100644 --- a/go/packages/packagestest/export.go +++ b/go/packages/packagestest/export.go @@ -65,13 +65,16 @@ Running the test with verbose output will print: package packagestest import ( + "errors" "flag" "fmt" "go/token" + "io" "io/ioutil" "log" "os" "path/filepath" + "runtime" "strings" "testing" @@ -79,12 +82,17 @@ import ( "golang.org/x/tools/go/packages" "golang.org/x/tools/internal/span" "golang.org/x/tools/internal/testenv" + "golang.org/x/xerrors" ) var ( skipCleanup = flag.Bool("skip-cleanup", false, "Do not delete the temporary export folders") // for debugging ) +// ErrUnsupported indicates an error due to an operation not supported on the +// current platform. +var ErrUnsupported = errors.New("operation is not supported") + // Module is a representation of a go module. type Module struct { // Name is the base name of the module as it would be in the go.mod file. @@ -180,6 +188,9 @@ func BenchmarkAll(b *testing.B, f func(*testing.B, Exporter)) { // The file deletion in the cleanup can be skipped by setting the skip-cleanup // flag when invoking the test, allowing the temporary directory to be left for // debugging tests. +// +// If the Writer for any file within any module returns an error equivalent to +// ErrUnspported, Export skips the test. func Export(t testing.TB, exporter Exporter, modules []Module) *Exported { t.Helper() if exporter == Modules { @@ -213,6 +224,17 @@ func Export(t testing.TB, exporter Exporter, modules []Module) *Exported { } }() for _, module := range modules { + // Create all parent directories before individual files. If any file is a + // symlink to a directory, that directory must exist before the symlink is + // created or else it may be created with the wrong type on Windows. + // (See https://golang.org/issue/39183.) + for fragment := range module.Files { + fullpath := exporter.Filename(exported, module.Name, filepath.FromSlash(fragment)) + if err := os.MkdirAll(filepath.Dir(fullpath), 0755); err != nil { + t.Fatal(err) + } + } + for fragment, value := range module.Files { fullpath := exporter.Filename(exported, module.Name, filepath.FromSlash(fragment)) written, ok := exported.written[module.Name] @@ -221,12 +243,12 @@ func Export(t testing.TB, exporter Exporter, modules []Module) *Exported { exported.written[module.Name] = written } written[fragment] = fullpath - if err := os.MkdirAll(filepath.Dir(fullpath), 0755); err != nil { - t.Fatal(err) - } switch value := value.(type) { case Writer: if err := value(fullpath); err != nil { + if xerrors.Is(err, ErrUnsupported) { + t.Skip(err) + } t.Fatal(err) } case string: @@ -261,26 +283,132 @@ func Script(contents string) Writer { // Link returns a Writer that creates a hard link from the specified source to // the required file. // This is used to link testdata files into the generated testing tree. +// +// If hard links to source are not supported on the destination filesystem, the +// returned Writer returns an error for which errors.Is(_, ErrUnsupported) +// returns true. func Link(source string) Writer { return func(filename string) error { - return os.Link(source, filename) + linkErr := os.Link(source, filename) + + if linkErr != nil && !builderMustSupportLinks() { + // Probe to figure out whether Link failed because the Link operation + // isn't supported. + if stat, err := openAndStat(source); err == nil { + if err := createEmpty(filename, stat.Mode()); err == nil { + // Successfully opened the source and created the destination, + // but the result is empty and not a hard-link. + return &os.PathError{Op: "Link", Path: filename, Err: ErrUnsupported} + } + } + } + + return linkErr } } // Symlink returns a Writer that creates a symlink from the specified source to the // required file. // This is used to link testdata files into the generated testing tree. +// +// If symlinks to source are not supported on the destination filesystem, the +// returned Writer returns an error for which errors.Is(_, ErrUnsupported) +// returns true. func Symlink(source string) Writer { if !strings.HasPrefix(source, ".") { - if abspath, err := filepath.Abs(source); err == nil { + if absSource, err := filepath.Abs(source); err == nil { if _, err := os.Stat(source); !os.IsNotExist(err) { - source = abspath + source = absSource } } } return func(filename string) error { - return os.Symlink(source, filename) + symlinkErr := os.Symlink(source, filename) + + if symlinkErr != nil && !builderMustSupportLinks() { + // Probe to figure out whether Symlink failed because the Symlink + // operation isn't supported. + fullSource := source + if !filepath.IsAbs(source) { + // Compute the target path relative to the parent of filename, not the + // current working directory. + fullSource = filepath.Join(filename, "..", source) + } + stat, err := openAndStat(fullSource) + mode := os.ModePerm + if err == nil { + mode = stat.Mode() + } else if !xerrors.Is(err, os.ErrNotExist) { + // We couldn't open the source, but it might exist. We don't expect to be + // able to portably create a symlink to a file we can't see. + return symlinkErr + } + + if err := createEmpty(filename, mode|0644); err == nil { + // Successfully opened the source (or verified that it does not exist) and + // created the destination, but we couldn't create it as a symlink. + // Probably the OS just doesn't support symlinks in this context. + return &os.PathError{Op: "Symlink", Path: filename, Err: ErrUnsupported} + } + } + + return symlinkErr + } +} + +// builderMustSupportLinks reports whether we are running on a Go builder +// that is known to support hard and symbolic links. +func builderMustSupportLinks() bool { + if os.Getenv("GO_BUILDER_NAME") == "" { + // Any OS can be configured to mount an exotic filesystem. + // Don't make assumptions about what users are running. + return false + } + + switch runtime.GOOS { + case "windows", "plan9": + // Some versions of Windows and all versions of plan9 do not support + // symlinks by default. + return false + + default: + // All other platforms should support symlinks by default, and our builders + // should not do anything unusual that would violate that. + return true + } +} + +// openAndStat attempts to open source for reading. +func openAndStat(source string) (os.FileInfo, error) { + src, err := os.Open(source) + if err != nil { + return nil, err + } + stat, err := src.Stat() + src.Close() + if err != nil { + return nil, err } + return stat, nil +} + +// createEmpty creates an empty file or directory (depending on mode) +// at dst, with the same permissions as mode. +func createEmpty(dst string, mode os.FileMode) error { + if mode.IsDir() { + return os.Mkdir(dst, mode.Perm()) + } + + f, err := os.OpenFile(dst, os.O_WRONLY|os.O_CREATE|os.O_EXCL, mode.Perm()) + if err != nil { + return err + } + if err := f.Close(); err != nil { + os.Remove(dst) // best-effort + return err + } + + return nil } // Copy returns a Writer that copies a file from the specified source to the @@ -297,12 +425,27 @@ func Copy(source string) Writer { // symlinks, devices, etc.) return fmt.Errorf("cannot copy non regular file %s", source) } - contents, err := ioutil.ReadFile(source) - if err != nil { - return err - } - return ioutil.WriteFile(filename, contents, stat.Mode()) + return copyFile(filename, source, stat.Mode().Perm()) + } +} + +func copyFile(dest, source string, perm os.FileMode) error { + src, err := os.Open(source) + if err != nil { + return err + } + defer src.Close() + + dst, err := os.OpenFile(dest, os.O_WRONLY|os.O_CREATE|os.O_EXCL, perm) + if err != nil { + return err + } + + _, err = io.Copy(dst, src) + if closeErr := dst.Close(); err == nil { + err = closeErr } + return err } // GroupFilesByModules attempts to map directories to the modules within each directory. diff --git a/go/packages/packagestest/export_test.go b/go/packages/packagestest/export_test.go index 36f63975059..356dd4b524d 100644 --- a/go/packages/packagestest/export_test.go +++ b/go/packages/packagestest/export_test.go @@ -15,7 +15,7 @@ import ( var testdata = []packagestest.Module{{ Name: "golang.org/fake1", Files: map[string]interface{}{ - "a.go": packagestest.Symlink("testdata/a.go"), + "a.go": packagestest.Symlink("testdata/a.go"), // broken symlink "b.go": "invalid file contents", }, Overlay: map[string][]byte{ diff --git a/internal/imports/fix_test.go b/internal/imports/fix_test.go index 005bf96e7e6..bfd3cfa1776 100644 --- a/internal/imports/fix_test.go +++ b/internal/imports/fix_test.go @@ -13,7 +13,6 @@ import ( "log" "path/filepath" "reflect" - "runtime" "sort" "strings" "sync" @@ -1307,11 +1306,6 @@ func bar() { // Test support for packages in GOPATH that are actually symlinks. // Also test that a symlink loop does not block the process. func TestImportSymlinks(t *testing.T) { - switch runtime.GOOS { - case "windows", "plan9": - t.Skipf("skipping test on %q as there are no symlinks", runtime.GOOS) - } - const input = `package p var ( @@ -1347,11 +1341,6 @@ var ( } func TestImportSymlinksWithIgnore(t *testing.T) { - switch runtime.GOOS { - case "windows", "plan9": - t.Skipf("skipping test on %q as there are no symlinks", runtime.GOOS) - } - const input = `package p var (