-
Notifications
You must be signed in to change notification settings - Fork 1k
check Gopkg filenames case on case insensitive systems #1114
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
CLAs look good, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
But since most of the tests are set to run on windows only and appveyor doesn't seem to be running for some reason, I'm not sure of the test results. Both Windows & macOS are case-insensitive (by default), so all those windows-only tests can be enabled to run on macOS, for which we have machine setup in travis.
Also, added some inline suggestions for changes. Looks pretty good. 👍
internal/fs/fs.go
Outdated
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we prefer writing os.Stat
.
internal/fs/fs.go
Outdated
return nil, errPathNotDir | ||
} | ||
|
||
// Ideally, we would use `os#Stat` for getting the actual file names |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.Stat
project.go
Outdated
} | ||
|
||
actualLfName := actualFilenames[LockName] | ||
if len(actualLfName) > 0 && actualLfName != LockName { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for this length check? I think just inequality check would be enough. Just curious 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a file is not found, the string map returned by ReadActualFilenames
will not have an entry for the given filename. Since lock file is optional, we should check for equality only if it was found.
Edit: Will add a code comment explaining this.
project_test.go
Outdated
invalidLfName := strings.ToLower(LockName) | ||
|
||
cases := []struct { | ||
errExpected bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's follow our "want" naming style and change this to wantErr
.
project_test.go
Outdated
cases := []struct { | ||
errExpected bool | ||
createFiles []string | ||
errMsg string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errMsg
-> wantErrMsg
project_test.go
Outdated
if err == nil { | ||
t.Fatalf("did not get expected error - '%s'", c.errMsg) | ||
} else if err.Error() != c.errMsg { | ||
t.Fatalf("expected message was '%s' but got '%s'", c.errMsg, err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make these GOT/WNT style failure messages.
Example:
("unexpected error message: \n\t(GOT) %s\n\t(WNT) %s", err.Error(), c.errMsg)
internal/fs/fs_test.go
Outdated
func TestIsCaseSensitiveFilesystem(t *testing.T) { | ||
if runtime.GOOS != "windows" && runtime.GOOS != "linux" { | ||
t.Skip("skip this test on non-Windows, non-Linux") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
darwin
? We run macOS specific tests on travis and by default macOS is also case insensitive, like windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are many OS recognised by Go runtime - https://github.com/golang/go/blob/master/src/go/build/syslist.go#L7. I thought that since I wasn't sure about the rest of them, it would be sufficient to test for 1 case sensitive and 1 case insensitive system. But I agree that it makes sense to check for macOS as it is among the most popular ones.
internal/fs/fs_test.go
Outdated
func TestReadActualFilenames(t *testing.T) { | ||
if runtime.GOOS != "windows" { | ||
t.Skip("skip this test on non-Windows") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And darwin
?
internal/fs/fs_test.go
Outdated
if err != nil { | ||
t.Fatalf("unexpected error: %+v", err) | ||
} | ||
reflect.DeepEqual(c.want, got) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a conditional check and GOT/WNT style failure message here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies. Missed this one.
oh! appveyor isn't reporting here but it's working fine https://ci.appveyor.com/project/golang/dep/build/1963 and tests are passing 😊 |
You should be able to simplify this a bit by using |
@sudo-suhas @sdboyer : Just to clarify, we are trying to prevent users from changing the letter-casing of I would suggest using a different approach and erroring only when using a case-senstive filesystem. If dep fails to find
instead of:
|
e24eef4
to
7d0e19a
Compare
@darkowlzz I have implemented the requested changes while rebasing for now.
@ibrasho I am not sure I follow. On case insensitive system, when
This would mean that @sdboyer Could you please weigh in on this? |
2d40f42
to
723f1bf
Compare
internal/fs/fs.go
Outdated
return nil, errPathNotDir | ||
} | ||
|
||
// Ideally, we would use `os.Stat` for getting the actual file names |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: i'm not sure what line length rule you're following here, and we're not really stringent about following any particular one in this project. but, at least within a particular comment block, let's be consistent 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Wrapping at 90 chars.
project.go
Outdated
// But error message will be a tad inaccurate. | ||
actualMfName := actualFilenames[ManifestName] | ||
if actualMfName != ManifestName { | ||
return fmt.Errorf("manifest filename '%s' does not match '%s'", actualMfName, ManifestName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: %q
instead of '%s'
internal/fs/fs.go
Outdated
// Otherwise, it reads the contents of the directory | ||
func ReadActualFilenames(dirPath string, names []string) (map[string]string, error) { | ||
actualFilenames := make(map[string]string, len(names)) | ||
if len(names) <= 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
len
can't actually be less then zero, afaik - let's make it the clearer check len(names) == 0
internal/fs/fs.go
Outdated
defer dir.Close() | ||
|
||
// Pass -1 to read all files in directory | ||
files, err := dir.Readdir(-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be quite a bit cheaper to use Readdirnames()
instead of Readdir()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it will be cheaper. I skimmed through the source - https://golang.org/src/os/dir_windows.go. It actually uses dir.Readdir
under the hood and only returns the names. By using dir.Readdir
, we can avoid file name comparisons for non-regular(file.Mode().IsRegular()
) files. Also, 1 less loop 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's the windows implementation, but the unix implementation is the reverse - Readdir
relies on Readdirnames
https://golang.org/src/os/dir_unix.go#L24. when we do it this way, both platforms are slow; when we do it with Readdirnames
, only windows is slow.
we could benchmark it, but i imagine that constant factors of retrieving full file stat info for each node will drastically outweigh the reduction in N from skipping a some subset of the names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. Okay. I saw https://golang.org/src/os/dir_plan9.go which was same as windows. Didn't know unix was different. I am guessing the unix one is used on macOS as well. Changing this to use Readdirnames
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still, i do appreciate the thoughtfulness!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha ha.. That was because all the code I saw was super through. Very very good code comments etc.
the goal is to make sure that we only consider files with the literal name so, i think @sudo-suhas is correct here in his approach - we don't actually need to do work on case-sensitive filesystems, because we're already guaranteed that addressing the file with the exact name we expect will succeed only if the file exists with that exact name. |
context_test.go
Outdated
func TestLoadProjectCfgFileCase(t *testing.T) { | ||
if runtime.GOOS != "windows" && runtime.GOOS != "darwin" { | ||
t.Skip("skip this test on non-Windows, non-macOS") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this to use fs.IsCaseSensitiveFilesystem
instead? Since that's what we are testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am slightly unsure about this. The function checkCfgFilenames
uses fs.IsCaseSensitiveFilesystem
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that's fine since checkCfgFilenames
really depends on fs.IsCaseSensitiveFilesystem
being correct to function properly.
(i.e. a bug in fs.IsCaseSensitiveFilesystem
will most likely result in checkCfgFilenames
not behaving properly.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But a bug in fs.IsCaseSensitiveFilesystem
could prevent the test from being run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would vote to change this to fs.IsCaseSensitiveFilesystem
, but I'll leave the final call to @sdboyer . 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think, for this specific case (and the other test doing this check in this PR), we should probably prefer to have the test results for fs.IsCaseSensitiveFilesystem()
and this test vary independently, so it's preferable to use the OS heuristic.
however, we need a comment explaining that this is the specific justification, as this is (or should be) the only place that we prefer the heuristic over doing the actual work of validating fs case sensitivity via fs.IsCaseSensitiveFilesystem()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @sdboyer, Could you please help me phrase the comment? Is this good enough - This is the only place that we prefer the heuristic over doing the actual work of validating fs case sensitivity via fs.IsCaseSensitiveFilesystem()
? While I get the gist, I am wondering if it can be phrased in simpler words.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i struggle with such phrasing things, as well :)
that seems good enough, but it also needs a bit that explains why we're avoiding it here - the reasons you gave earlier, about ensuring that our testing outcomes aren't correlated.
@@ -229,6 +231,112 @@ func TestRenameWithFallback(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestIsCaseSensitiveFilesystem(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit inaccurate. You can mount a case-sensitive filesystem on Linux, a case-insensitive one on Linux/macOS...
Can we think of a better way to check this rather than relying on the OS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. But I cannot think of a better way. To some extent, I think we can make some assumptions about the test environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not work on Windows Subsystem for Linux. dF -Th
will give you the actual file system type if the OS is linux. I'm not sure how it works with samba shares, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colek42 Is Windows Subsystem really something that the tests should handle? Isn't it an edge case we can safely ignore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sudo-suhas it is easy enough to test for. See microsoft/WSL#423, can you just skip the test if it is WSL? Some of us are forced to use Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of us are forced to use Windows.
Ha ha.. Don't worry I am on windows too. I will look into this. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sudo-suhas let me know if you need me to test any changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what's the best way to test this tbh. It's not easy to find a cross-platform solution for this as far as I know...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdboyer Do I need to check for BashOnWindows? How would I check this in golang? Is it as simple as a file read from /proc/version
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sudo-suhas not here - this is just a test. unless we have an actual reason to believe the real logic (not the test logic) fails on WSL, then it's fine to omit here.
internal/fs/fs_test.go
Outdated
} | ||
|
||
func TestReadActualFilenames(t *testing.T) { | ||
if runtime.GOOS != "windows" && runtime.GOOS != "darwin" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this to use fs.IsCaseSensitiveFilesystem instead?
project.go
Outdated
// 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 checkCfgFilenames(projectRoot string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would prefer to name this checkGopkgFilenames
. 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, let's make that change.
project_test.go
Outdated
@@ -61,6 +63,75 @@ func TestFindRoot(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestCheckCfgFilenames(t *testing.T) { | |||
if runtime.GOOS != "windows" && runtime.GOOS != "darwin" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this to use fs.IsCaseSensitiveFilesystem
instead?
{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)}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when the project has an invalidMfName
and a valid LockName
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check is sequential. So if the manifest file name is invalid an error is returned and we do not check the lock file.
@carolynvs i'm not entirely sure where the logic paths are for root vs. injected analyzer now, so could you have a quick look at this just to make sure that we're also rejecting improperly cased metadata file names in the injected analyzer? (might take a minute for that, as she's at a conference this week) |
There are lots of places in dep where we stat Lines 19 to 23 in 28fb6b0
Also the importers (for external config files): Lines 71 to 79 in 28fb6b0
Init also stats that filename to check if dep has already been run before: Lines 102 to 112 in beb6bb1
If all these need the same logic in |
@carolynvs Are all of these mentioned files in independent execution paths? For example, init is. What I am trying to say is, if the different file paths are part of the same command(ex:
Do we really need to enforce case sensitive file name checks for external config files?
Correct me if I am wrong here, but |
Each of the code snippets above could be the first check for a particular file in an execution path:
I'm not sure about enforcing rather ensuring consistent behavior on both filesystems. However consider the following scenario: When I am on a mac and have a dependency on a package that uses glide, but the package has "GLIDE.LOCK", dep checks for "glide.lock", finds that and uses it during solve. But on linux, it wouldn't because the case is not what dep expected. Getting different solve results due to case-sensitive/insensitive filesystems is undesired.
I was suggesting something that wraps I'm not saying any of this feedback must be addressed this PR (I'll leave that up to the original reviewers). I am just sharing context on where else in init/ensure dep does file checks that may be impacted by case-sensitive file systems. |
There is no need to check if the filesystem is case-sensitive before calling |
This is a comment from the source code of this PR 😀
None of my comments are changes requested for this PR, just context. I've provided it and will leave it up to the main reviewers from here. |
@carolynvs My apologies 😅. I will update the comment. Btw, just a thought. All the checks you mentioned, they are for the same folder(project root) right? So how about I define a helper struct which can be reused in all places? The struct would hold the actual filename for all files in the project root and would be created only once. @sdboyer @ibrasho @darkowlzz I really don't mind doing any changes including the changes suggested by @carolynvs. But I would really appreciate it if you could finalise the requested changes so that I can tackle all of them. |
The checks will be different directories once we start checking for external config in depndencies during solve. |
project.go
Outdated
@@ -43,10 +43,16 @@ func findProjectRoot(from string) (string, error) { | |||
} | |||
|
|||
// checkCfgFilenames validates filename case for the manifest and lock files. | |||
// This is relevant on case-insensitive systems like Windows. | |||
// | |||
// This is relevant on case-insensitive file systems like Windows and macOS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/like Windows/like the defaults in Windows/
project.go
Outdated
// 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 checkCfgFilenames(projectRoot string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, let's make that change.
context_test.go
Outdated
func TestLoadProjectCfgFileCase(t *testing.T) { | ||
if runtime.GOOS != "windows" && runtime.GOOS != "darwin" { | ||
t.Skip("skip this test on non-Windows, non-macOS") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think, for this specific case (and the other test doing this check in this PR), we should probably prefer to have the test results for fs.IsCaseSensitiveFilesystem()
and this test vary independently, so it's preferable to use the OS heuristic.
however, we need a comment explaining that this is the specific justification, as this is (or should be) the only place that we prefer the heuristic over doing the actual work of validating fs case sensitivity via fs.IsCaseSensitiveFilesystem()
.
d2eef8e
to
277ba75
Compare
- `fs` - Export `IsCaseSensitiveFilesystem`. Add and run test on windows, linux and macOS. - Add function `ReadActualFilenames` to read actual file names of given string slice. Add tests to be run on windows and macOS. - `project` - Add function `checkCfgFilenames` to check the filenames for manifest and lock have the expected case. Use `fs#IsCaseSensitiveFilesystem` for an early return as the check is costly. Add test to be run on windows and macOS. - `context` - Call `project.go#checkCfgFilenames` after resolving project root. Add test for invalid manifest file name to be run on windows and macOS.
- `project` - `checkCfgFilenames` - Improve function and code comments - Use boolean value indicating whether value was found in actual filenames. If manifest file was not found, return `errProjectNotFound`. Use boolean to determine if lock file was not found instead of length check. - `TestCheckCfgFilenames` - Add code comments for test cases explaining the expected behavior - `fs` - `ReadActualFilenames` - Use cleaner check(`<=` ➡ `==`) to see if `names` parameter for `ReadActualFilenames` actually has any values. - Use `Readdirnames` instead of `Readdir`. This is expected to perform better in most cases. - `TestReadActualFilenames` - Add code comments for test cases explaining the expected behavior - general - Make line length for code comments consistent(90), add periods where appropriate. - String formatting, use %q instead of '%s'
- `project.go` - Rename method {checkCfgFilenames => checkGopkgFilenames} - Improve funciton comment as suggested by @sdboyer - Fix ambigious comment explaining rationale behind early return. - Add comment explaining why we do not use `fs.IsCaseSensitiveFilesystem` for skipping following tests: - context_test.go#TestLoadProjectGopkgFilenames - project_test.go#TestCheckGopkgFilenames - fs_test.go#TestReadActualFilenames
277ba75
to
d451fa0
Compare
@sdboyer I have done the requested changes. AFAIK the CI failure is not related to any changes I made. Please take a look. |
yeah, the issue there was unrelated - #1200. kicking the job just for cleanliness' sake |
idk why we have a fail from travis, everything passed. well, anyway - in we go, thanks! 🎉 |
What does this do / why do we need it?
This checks the manifest and lock file name on case insensitive systems to ensure that the case exactly matches
Gopkg.toml
andGopkg.lock
.Do you need help or clarification on anything?
Looking for feedback on the tests and added functions.
Which issue(s) does this PR fix?
fixes #1035
Changelog
fs
IsCaseSensitiveFilesystem
. Add and run test on windows, linux.ReadActualFilenames
to read actual file names of givenstring slice. Add tests to be run on windows.
project
checkCfgFilenames
to check the filenames for manifestand lock have the expected case. Use
fs#IsCaseSensitiveFilesystem
for an early return as the check is costly. Add test to be run on windows.
context
project#checkCfgFilenames
after resolving project root. Add testfor invalid manifest file name to be run on windows.