Skip broken vendor symlink rather than returning an error.

[wolkykim]

What does this do / why do we need it?

Skip broken vendor symlink rather than returning an error

What should your reviewer look out for in this PR?

When one of 3rd party packages has a broken vendor symlink, it fails whole ensure process.

Current logic behaves:

• remove a vendor symlink linked to a valid directory
• leave a vendor symlink linked to a valid file
• return error if a vendor symlink is broken (linked file doesn't exist)

I think this behavior is not intended and it seems to be that it's better to skip the check except the case it's linked to a valid directory as it's harmless rather than returning an error causing the whole process stop.

Expected/New behavior:

• remove a vendor symlink linked to a valid directory
• leave rest of the cases(linked to a file or broken) as they are not harmful rather than returning an error

Do you need help or clarification on anything?

No

Which issue(s) does this PR fix?

This is a new behavior introduced in v0.3.1 with its improved type checking
Since this is a breaking change, some users migrating from 0.3.0 could experience this issue.
The main problem is that when this happens on some 3rd party libraries that you don't have the control, you just have to downgrade and use v0.3.0

v0.3.1
$ dep ensure -v
(28/45) Failed to write github.com/...(removed)...@v1.8.0
grouped write of manifest, lock and vendor: error while writing out vendor tree: failed to write dep tree: failed to strip vendor from github.com/...(removed)...: stat /var/folders/7q/lqjydj5s3y72cxbtv4srgy4myyfy7s/T/dep540422238/vendor/github.com/...(removed).../vendor: no such file or directory
$ echo $?
1

v0.3.0
$ dep ensure
$ echo $?
0

[sdboyer]

Rather than simply ignoring errors here, let's verify that it's an error indicating that the target file does not exist.

(I'm still thinking about this change overall, but if we do go this direction, then we absolutely need this check.)

[wolkykim]

I guess adding err != nil { do_nothing } is not necessary. it ignores when lstat() succeed but stat() is either returns null or non-directory. What checks would you like to add specifically?
This is a case our whole team had to rollback.

[spenczar]

(I'm still thinking about this change overall, but if we do go this direction, then we absolutely need this check.)

@sdboyer, what are your concerns on skipping broken vendor symlinks? I'd like to talk it out - this issue is blocking several teams' usage of dep at $EMPLOYER right now, so I feel invested as $EMPLOYER's resident dep champion :)

[sdboyer]

sorry, yes, i wasn't thinking clearly about the implications here. this is definitely fine.