Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Guard against errs in stripVendor WalkFunc #1026

Merged
merged 5 commits into from
Aug 17, 2017
Merged

Guard against errs in stripVendor WalkFunc #1026

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b4373a7
Guard against errs in stripVendor WalkFunc
sdboyer Aug 17, 2017
8f928b1
Accommodate error channel size properly
sdboyer Aug 17, 2017
b4ff96f
Fix remove-before-visit bug
sdboyer Aug 17, 2017
151b3c8
Fix remove-before-visit for Windows, too
sdboyer Aug 17, 2017
8c85179
Drop unnecessary err check at start of walker
sdboyer Aug 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion internal/gps/result.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,10 @@ func WriteDepTree(basedir string, l Lock, sm SourceManager, sv bool, logger *log
}

if sv {
filepath.Walk(to, stripVendor)
err := filepath.Walk(to, stripVendor)
if err != nil {
errCh <- errors.Wrapf(err, "failed to strip vendor from %s", p.Ident().ProjectRoot)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes good call

}
wg.Done()
}(p)
Expand Down
9 changes: 8 additions & 1 deletion internal/gps/strip_vendor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,16 @@

package gps

import "os"
import (
"os"
"path/filepath"
)

func stripVendor(path string, info os.FileInfo, err error) error {
if err != nil && err != filepath.SkipDir {
return err
}

if info.Name() == "vendor" {
if _, err := os.Lstat(path); err == nil {
if (info.Mode() & os.ModeSymlink) != 0 {
Expand Down
4 changes: 4 additions & 0 deletions internal/gps/strip_vendor_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ import (
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a //+build windows at the top of this file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the _windows.go file suffix does the same thing.


func stripVendor(path string, info os.FileInfo, err error) error {
if err != nil && err != filepath.SkipDir {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think filepath.SkipDir is ever passed again to the walkFn.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, that was actually the problem, i think - see my comment on main thread.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you actually seen filepath.SkipDir here? Wouldn't that be a sever std lib bug?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's possible that i'm cargo culting this, lemme see

return err
}

if info.Name() == "vendor" {
if _, err := os.Lstat(path); err == nil {
symlink := (info.Mode() & os.ModeSymlink) != 0
Expand Down