diff --git a/context.go b/context.go index 14b26b1853..f0a80473ab 100644 --- a/context.go +++ b/context.go @@ -105,6 +105,11 @@ func (c *Ctx) LoadProject() (*Project, error) { return nil, err } + err = checkGopkgFilenames(root) + if err != nil { + return nil, err + } + p := new(Project) if err = p.SetRoot(root); err != nil { diff --git a/context_test.go b/context_test.go index c08d1e658d..7e5eda6e25 100644 --- a/context_test.go +++ b/context_test.go @@ -5,6 +5,7 @@ package dep import ( + "fmt" "io/ioutil" "log" "os" @@ -263,6 +264,55 @@ func TestLoadProjectNoSrcDir(t *testing.T) { } } +func TestLoadProjectGopkgFilenames(t *testing.T) { + // We are trying to skip this test on file systems which are case-sensiive. We could + // have used `fs.IsCaseSensitiveFilesystem` for this check. However, the code we are + // testing also relies on `fs.IsCaseSensitiveFilesystem`. So a bug in + // `fs.IsCaseSensitiveFilesystem` could prevent this test from being run. This is the + // only scenario where we prefer the OS heuristic over doing the actual work of + // validating filesystem case sensitivity via `fs.IsCaseSensitiveFilesystem`. + if runtime.GOOS != "windows" && runtime.GOOS != "darwin" { + t.Skip("skip this test on non-Windows, non-macOS") + } + + // Here we test that a manifest filename with incorrect case throws an error. Similar + // error will also be thrown for the lock file as well which has been tested in + // `project_test.go#TestCheckGopkgFilenames`. So not repeating here. + + h := test.NewHelper(t) + defer h.Cleanup() + + invalidMfName := strings.ToLower(ManifestName) + + wd := filepath.Join("src", "test") + h.TempFile(filepath.Join(wd, invalidMfName), "") + + ctx := &Ctx{ + Out: discardLogger(), + Err: discardLogger(), + } + + err := ctx.SetPaths(h.Path(wd), h.Path(".")) + if err != nil { + t.Fatalf("%+v", err) + } + + _, err = ctx.LoadProject() + + if err == nil { + t.Fatal("should have returned 'Manifest Filename' error") + } + + expectedErrMsg := fmt.Sprintf( + "manifest filename %q does not match %q", + invalidMfName, ManifestName, + ) + + if err.Error() != expectedErrMsg { + t.Fatalf("unexpected error: %+v", err) + } +} + // TestCaseInsentitive is test for Windows. This should work even though set // difference letter cases in GOPATH. func TestCaseInsentitiveGOPATH(t *testing.T) { diff --git a/internal/fs/fs.go b/internal/fs/fs.go index 0fb84a8e39..bfacfd95af 100644 --- a/internal/fs/fs.go +++ b/internal/fs/fs.go @@ -35,9 +35,9 @@ func HasFilepathPrefix(path, prefix string) (bool, error) { // handling of volume name/drive letter on Windows. vnPath and vnPrefix // are first compared, and then used to initialize initial values of p and // d which will be appended to for incremental checks using - // isCaseSensitiveFilesystem and then equality. + // IsCaseSensitiveFilesystem and then equality. - // no need to check isCaseSensitiveFilesystem because VolumeName return + // no need to check IsCaseSensitiveFilesystem because VolumeName return // empty string on all non-Windows machines vnPath := strings.ToLower(filepath.VolumeName(path)) vnPrefix := strings.ToLower(filepath.VolumeName(prefix)) @@ -82,7 +82,7 @@ func HasFilepathPrefix(path, prefix string) (bool, error) { // something like ext4 filesystem mounted on FAT // mountpoint, mounted on ext4 filesystem, i.e. the // problematic filesystem is not the last one. - caseSensitive, err := isCaseSensitiveFilesystem(filepath.Join(d, dirs[i])) + caseSensitive, err := IsCaseSensitiveFilesystem(filepath.Join(d, dirs[i])) if err != nil { return false, errors.Wrap(err, "failed to check filepath prefix") } @@ -135,7 +135,7 @@ func EquivalentPaths(p1, p2 string) (bool, error) { } if p1Filename != "" || p2Filename != "" { - caseSensitive, err := isCaseSensitiveFilesystem(filepath.Join(p1, p1Filename)) + caseSensitive, err := IsCaseSensitiveFilesystem(filepath.Join(p1, p1Filename)) if err != nil { return false, errors.Wrap(err, "could not check for filesystem case-sensitivity") } @@ -193,7 +193,7 @@ func renameByCopy(src, dst string) error { return errors.Wrapf(os.RemoveAll(src), "cannot delete %s", src) } -// isCaseSensitiveFilesystem determines if the filesystem where dir +// IsCaseSensitiveFilesystem determines if the filesystem where dir // exists is case sensitive or not. // // CAVEAT: this function works by taking the last component of the given @@ -212,7 +212,7 @@ func renameByCopy(src, dst string) error { // If the input directory is such that the last component is composed // exclusively of case-less codepoints (e.g. numbers), this function will // return false. -func isCaseSensitiveFilesystem(dir string) (bool, error) { +func IsCaseSensitiveFilesystem(dir string) (bool, error) { alt := filepath.Join(filepath.Dir(dir), genTestFilename(filepath.Base(dir))) dInfo, err := os.Stat(dir) @@ -264,6 +264,88 @@ func genTestFilename(str string) string { }, str) } +var errPathNotDir = errors.New("given path is not a directory") + +// ReadActualFilenames is used to determine the actual file names in given directory. +// +// On case sensitive file systems like ext4, it will check if those files exist using +// `os.Stat` and return a map with key and value as filenames which exist in the folder. +// +// Otherwise, it reads the contents of the directory and returns a map which has the +// given file name as the key and actual filename as the value(if it was found). +func ReadActualFilenames(dirPath string, names []string) (map[string]string, error) { + actualFilenames := make(map[string]string, len(names)) + if len(names) == 0 { + // This isn't expected to happen for current usage. Adding edge case handling, + // as it may be useful in future. + return actualFilenames, nil + } + // First, check that the given path is valid and it is a directory + dirStat, err := os.Stat(dirPath) + if err != nil { + return nil, errors.Wrap(err, "failed to read actual filenames") + } + + if !dirStat.IsDir() { + return nil, errPathNotDir + } + + // Ideally, we would use `os.Stat` for getting the actual file names but that returns + // the name we passed in as an argument and not the actual filename. So we are forced + // to list the directory contents and check against that. Since this check is costly, + // we do it only if absolutely necessary. + caseSensitive, err := IsCaseSensitiveFilesystem(dirPath) + if err != nil { + return nil, errors.Wrap(err, "failed to read actual filenames") + } + if caseSensitive { + // There will be no difference between actual filename and given filename. So + // just check if those files exist. + for _, name := range names { + _, err := os.Stat(filepath.Join(dirPath, name)) + if err == nil { + actualFilenames[name] = name + } else if !os.IsNotExist(err) { + // Some unexpected err, wrap and return it. + return nil, errors.Wrap(err, "failed to read actual filenames") + } + } + return actualFilenames, nil + } + + dir, err := os.Open(dirPath) + if err != nil { + return nil, errors.Wrap(err, "failed to read actual filenames") + } + defer dir.Close() + + // Pass -1 to read all filenames in directory + filenames, err := dir.Readdirnames(-1) + if err != nil { + return nil, errors.Wrap(err, "failed to read actual filenames") + } + + // namesMap holds the mapping from lowercase name to search name. Using this, we can + // avoid repeatedly looping through names. + namesMap := make(map[string]string, len(names)) + for _, name := range names { + namesMap[strings.ToLower(name)] = name + } + + for _, filename := range filenames { + searchName, ok := namesMap[strings.ToLower(filename)] + if ok { + // We are interested in this file, case insensitive match successful. + actualFilenames[searchName] = filename + if len(actualFilenames) == len(names) { + // We found all that we were looking for. + return actualFilenames, nil + } + } + } + return actualFilenames, nil +} + var ( errSrcNotDir = errors.New("source is not a directory") errDstExist = errors.New("destination already exists") diff --git a/internal/fs/fs_test.go b/internal/fs/fs_test.go index 920d925bb1..4b9422dd88 100644 --- a/internal/fs/fs_test.go +++ b/internal/fs/fs_test.go @@ -8,11 +8,13 @@ import ( "io/ioutil" "os" "path/filepath" + "reflect" "runtime" "strings" "testing" "github.com/golang/dep/internal/test" + "github.com/pkg/errors" ) // This function tests HadFilepathPrefix. It should test it on both case @@ -169,7 +171,7 @@ func TestEquivalentPaths(t *testing.T) { {strings.ToLower(h.Path("dir")), strings.ToUpper(h.Path("dir")), false, true, true}, } - caseSensitive, err := isCaseSensitiveFilesystem(h.Path("dir")) + caseSensitive, err := IsCaseSensitiveFilesystem(h.Path("dir")) if err != nil { t.Fatal("unexpcted error:", err) } @@ -229,6 +231,119 @@ func TestRenameWithFallback(t *testing.T) { } } +func TestIsCaseSensitiveFilesystem(t *testing.T) { + isLinux := runtime.GOOS == "linux" + isWindows := runtime.GOOS == "windows" + isMacOS := runtime.GOOS == "darwin" + + if !isLinux && !isWindows && !isMacOS { + t.Skip("Run this test on Windows, Linux and macOS only") + } + + dir, err := ioutil.TempDir("", "TestCaseSensitivity") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + var want bool + if isLinux { + want = true + } else { + want = false + } + + got, err := IsCaseSensitiveFilesystem(dir) + + if err != nil { + t.Fatalf("unexpected error message: \n\t(GOT) %+v", err) + } + + if want != got { + t.Fatalf("unexpected value returned: \n\t(GOT) %t\n\t(WNT) %t", got, want) + } +} + +func TestReadActualFilenames(t *testing.T) { + // We are trying to skip this test on file systems which are case-sensiive. We could + // have used `fs.IsCaseSensitiveFilesystem` for this check. However, the code we are + // testing also relies on `fs.IsCaseSensitiveFilesystem`. So a bug in + // `fs.IsCaseSensitiveFilesystem` could prevent this test from being run. This is the + // only scenario where we prefer the OS heuristic over doing the actual work of + // validating filesystem case sensitivity via `fs.IsCaseSensitiveFilesystem`. + if runtime.GOOS != "windows" && runtime.GOOS != "darwin" { + t.Skip("skip this test on non-Windows, non-macOS") + } + + h := test.NewHelper(t) + defer h.Cleanup() + + h.TempDir("") + tmpPath := h.Path(".") + + // First, check the scenarios for which we expect an error. + _, err := ReadActualFilenames(filepath.Join(tmpPath, "does_not_exists"), []string{""}) + switch { + case err == nil: + t.Fatal("expected err for non-existing folder") + // use `errors.Cause` because the error is wrapped and returned + case !os.IsNotExist(errors.Cause(err)): + t.Fatalf("unexpected error: %+v", err) + } + h.TempFile("tmpFile", "") + _, err = ReadActualFilenames(h.Path("tmpFile"), []string{""}) + switch { + case err == nil: + t.Fatal("expected err for passing file instead of directory") + case err != errPathNotDir: + t.Fatalf("unexpected error: %+v", err) + } + + cases := []struct { + createFiles []string + names []string + want map[string]string + }{ + // If we supply no filenames to the function, it should return an empty map. + {nil, nil, map[string]string{}}, + // If the directory contains the given file with different case, it should return + // a map which has the given filename as the key and actual filename as the value. + { + []string{"test1.txt"}, + []string{"Test1.txt"}, + map[string]string{"Test1.txt": "test1.txt"}, + }, + // 1. If the given filename is same as the actual filename, map should have the + // same key and value for the file. + // 2. If the given filename is present with different case for file extension, + // it should return a map which has the given filename as the key and actual + // filename as the value. + // 3. If the given filename is not present even with a different case, the map + // returned should not have an entry for that filename. + { + []string{"test2.txt", "test3.TXT"}, + []string{"test2.txt", "Test3.txt", "Test4.txt"}, + map[string]string{ + "test2.txt": "test2.txt", + "Test3.txt": "test3.TXT", + }, + }, + } + for _, c := range cases { + for _, file := range c.createFiles { + h.TempFile(file, "") + } + got, err := ReadActualFilenames(tmpPath, c.names) + if err != nil { + t.Fatalf("unexpected error: %+v", err) + } + if !reflect.DeepEqual(c.want, got) { + t.Fatalf("returned value does not match expected: \n\t(GOT) %v\n\t(WNT) %v", + got, c.want) + } + } +} + func TestGenTestFilename(t *testing.T) { cases := []struct { str string @@ -254,11 +369,11 @@ func TestGenTestFilename(t *testing.T) { func BenchmarkGenTestFilename(b *testing.B) { cases := []string{ - "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", - "αααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααα", - "11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111", - "⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘", + strings.Repeat("a", 128), + strings.Repeat("A", 128), + strings.Repeat("α", 128), + strings.Repeat("1", 128), + strings.Repeat("⌘", 128), } for i := 0; i < b.N; i++ { @@ -613,6 +728,7 @@ func TestCopyFileLongFilePath(t *testing.T) { h := test.NewHelper(t) h.TempDir(".") + defer h.Cleanup() tmpPath := h.Path(".") diff --git a/project.go b/project.go index 6e5ffaae21..9749a723bf 100644 --- a/project.go +++ b/project.go @@ -11,6 +11,7 @@ import ( "github.com/golang/dep/internal/fs" "github.com/golang/dep/internal/gps" + "github.com/pkg/errors" ) var ( @@ -41,6 +42,54 @@ func findProjectRoot(from string) (string, error) { } } +// checkGopkgFilenames validates filename case for the manifest and lock files. +// +// This is relevant on case-insensitive file systems like the defaults in Windows and +// macOS. +// +// If manifest file is not found, it returns an error indicating the project could not be +// found. If it is found but the case does not match, an error is returned. If a lock +// file is not found, no error is returned as lock file is optional. If it is found but +// the case does not match, an error is returned. +func checkGopkgFilenames(projectRoot string) error { + // ReadActualFilenames is actually costly. Since the check to validate filename case + // for Gopkg filenames is not relevant to case-sensitive filesystems like + // ext4(linux), try for an early return. + caseSensitive, err := fs.IsCaseSensitiveFilesystem(projectRoot) + if err != nil { + return errors.Wrap(err, "could not check validity of configuration filenames") + } + if caseSensitive { + return nil + } + + actualFilenames, err := fs.ReadActualFilenames(projectRoot, []string{ManifestName, LockName}) + + if err != nil { + return errors.Wrap(err, "could not check validity of configuration filenames") + } + + actualMfName, found := actualFilenames[ManifestName] + if !found { + // Ideally this part of the code won't ever be executed if it is called after + // `findProjectRoot`. But be thorough and handle it anyway. + return errProjectNotFound + } + if actualMfName != ManifestName { + return fmt.Errorf("manifest filename %q does not match %q", actualMfName, ManifestName) + } + + // If a file is not found, the string map returned by `fs.ReadActualFilenames` will + // not have an entry for the given filename. Since the lock file is optional, we + // should check for equality only if it was found. + actualLfName, found := actualFilenames[LockName] + if found && actualLfName != LockName { + return fmt.Errorf("lock filename %q does not match %q", actualLfName, LockName) + } + + return nil +} + // A Project holds a Manifest and optional Lock for a project. type Project struct { // AbsRoot is the absolute path to the root directory of the project. diff --git a/project_test.go b/project_test.go index d2dedece7d..c3d1ef00ba 100644 --- a/project_test.go +++ b/project_test.go @@ -5,9 +5,11 @@ package dep import ( + "fmt" "os" "path/filepath" "runtime" + "strings" "testing" "github.com/golang/dep/internal/gps" @@ -61,6 +63,81 @@ func TestFindRoot(t *testing.T) { } } +func TestCheckGopkgFilenames(t *testing.T) { + // We are trying to skip this test on file systems which are case-sensiive. We could + // have used `fs.IsCaseSensitiveFilesystem` for this check. However, the code we are + // testing also relies on `fs.IsCaseSensitiveFilesystem`. So a bug in + // `fs.IsCaseSensitiveFilesystem` could prevent this test from being run. This is the + // only scenario where we prefer the OS heuristic over doing the actual work of + // validating filesystem case sensitivity via `fs.IsCaseSensitiveFilesystem`. + if runtime.GOOS != "windows" && runtime.GOOS != "darwin" { + t.Skip("skip this test on non-Windows, non-macOS") + } + + errMsgFor := func(filetype, filename string) func(string) string { + return func(name string) string { + return fmt.Sprintf("%s filename %q does not match %q", filetype, name, filename) + } + } + + manifestErrMsg := errMsgFor("manifest", ManifestName) + lockErrMsg := errMsgFor("lock", LockName) + + invalidMfName := strings.ToLower(ManifestName) + invalidLfName := strings.ToLower(LockName) + + cases := []struct { + wantErr bool + createFiles []string + wantErrMsg string + }{ + // No error should be returned when the project contains a valid manifest file + // but no lock file. + {false, []string{ManifestName}, ""}, + // No error should be returned when the project contains a valid manifest file as + // well as a valid lock file. + {false, []string{ManifestName, LockName}, ""}, + // Error indicating the project was not found should be returned if a manifest + // file is not found. + {true, nil, errProjectNotFound.Error()}, + // Error should be returned if the project has a manifest file with invalid name + // but no lock file. + {true, []string{invalidMfName}, manifestErrMsg(invalidMfName)}, + // Error should be returned if the project has a valid manifest file and an + // invalid lock file. + {true, []string{ManifestName, invalidLfName}, lockErrMsg(invalidLfName)}, + } + + for _, c := range cases { + h := test.NewHelper(t) + defer h.Cleanup() + + // Create a temporary directory which we will use as the project folder. + h.TempDir("") + tmpPath := h.Path(".") + + // Create any files that are needed for the test before invoking + // `checkGopkgFilenames`. + for _, file := range c.createFiles { + h.TempFile(file, "") + } + err := checkGopkgFilenames(tmpPath) + + if c.wantErr { + if err == nil { + // We were expecting an error but did not get one. + t.Fatalf("unexpected error message: \n\t(GOT) nil\n\t(WNT) %s", c.wantErrMsg) + } else if err.Error() != c.wantErrMsg { + // We got an error but it is not the one we were expecting. + t.Fatalf("unexpected error message: \n\t(GOT) %s\n\t(WNT) %s", err.Error(), c.wantErrMsg) + } + } else if err != nil { + // Error was not expected but still we got one + t.Fatalf("unexpected error message: \n\t(GOT) %+v", err) + } + } +} + func TestProjectMakeParams(t *testing.T) { m := NewManifest() m.Ignored = []string{"ignoring this"}