Guard against errs in stripVendor WalkFunc #1026
Conversation
| err := filepath.Walk(to, stripVendor) | ||
| if err != nil { | ||
| errCh <- errors.Wrapf(err, "failed to strip vendor from %s", p.Ident().ProjectRoot) | ||
| } |
There was a problem hiding this comment.
Now the errCh buffer is too small since each routine could send 2 errors. Does that even make sense? Or should the first error terminate the goroutine? (maybe defer the wg.Done() so we can return from there)
|
of course there are windows errors 😠 |
internal/gps/strip_vendor_windows.go
Outdated
| @@ -10,6 +10,10 @@ import ( | |||
| ) | |||
|
|
|||
| func stripVendor(path string, info os.FileInfo, err error) error { | |||
| if err != nil && err != filepath.SkipDir { | |||
There was a problem hiding this comment.
I don't think filepath.SkipDir is ever passed again to the walkFn.
There was a problem hiding this comment.
yep, that was actually the problem, i think - see my comment on main thread.
There was a problem hiding this comment.
Have you actually seen filepath.SkipDir here? Wouldn't that be a sever std lib bug?
There was a problem hiding this comment.
it's possible that i'm cargo culting this, lemme see
|
oh, there was an actual bug here - we were removing the vendor dir, but not telling the walker to skip as a dir to then subsequently traverse. |
|
this windows problem is...bizarre, i don't understand why that's happening. pinging my trusty @ChrisHines 😄 - do you know if |
|
why, windows. WHY. 😠 |
| @@ -10,6 +10,10 @@ import ( | |||
| ) | |||
There was a problem hiding this comment.
Should there be a //+build windows at the top of this file?
There was a problem hiding this comment.
No, the _windows.go file suffix does the same thing.
|
I don't understand all the context here, but I notice that this PR updates the strip_vendor.go file, but not the strip_vendor_windows.go file. Does a similar change need to be made for the Windows implementation of |
|
@sdboyer You're most recent commit does not affect the Windows impl of |
|
@ChrisHines 🤦♂️ 🤦♂️ 🤦♂️ i am so sorry to have summoned you for this boneheaded mistake. i'm looking forward to my vacation. thank you! |
|
ok, somebody else approve and merge when it seems OK to them |
internal/gps/strip_vendor.go
Outdated
| @@ -12,10 +12,6 @@ import ( | |||
| ) | |||
|
|
|||
| func stripVendor(path string, info os.FileInfo, err error) error { | |||
| if err != nil && err != filepath.SkipDir { | |||
There was a problem hiding this comment.
I think we still need this check, but only the if err != nil { part.
There was a problem hiding this comment.
Because the path passed causes os.Lstat to return an error somehow, then walkFn will be passed nil for info and the error. Jumping to the code below will cause the panic again.
There was a problem hiding this comment.
yeah, finally looked at the filepath.Walk() implementation - lstat(), or readDirNames(). ok 😄
What does this do / why do we need it?
defends against errors/panics in the
filepath.WalkFuncwe use to strip out vendor directories,gps.stripVendor()What should your reviewer look out for in this PR?
Which issue(s) does this PR fix?
fixes #1022