Follow symlinks when stripping vendor directories

[spenczar]

A bit of extra trickiness here because I'm scared of repos with a symlink like ./vendor -> /.

I would love any guidance on how to write an integration test for this! I verified by hacking this into dep and running it on a test repo, but I don't feel comfortable without being able to run cross-platform tests...

Fixes #198

[codecov[bot]]

Codecov Report

Merging #199 into master will increase coverage by 0.03%. The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #199      +/-   ##
==========================================
+ Coverage   77.76%   77.79%   +0.03%     
==========================================
  Files          25       26       +1     
  Lines        3989     3995       +6     
==========================================
+ Hits         3102     3108       +6     
+ Misses        668      667       -1     
- Partials      219      220       +1

Impacted Files	Coverage Δ
vcs_source.go	73.76% <ø> (+0.69%)	:arrow_up:
strip_vendor.go	83.33% <83.33%> (ø)
source_manager.go	91.38% <0%> (-0.75%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 65f245d...05ef430. Read the comment docs.

[sdboyer]

Uuuuughh. Yeah, I spent some time a couple weeks ago trying to work through similar problems in #177, ultimately having to roll back #157 because I couldn't find a satisfactory answer. However, that the need here may be simpler, as I think there's only the simpler requirement of inspecting the outermost symlink. That may make it equivalent to the first-pass of work in #157; the implementation and/or tests in there may be worth looking at.

Hopefully something in there will spark a thought about a cross-platform test. If not...well, we'll have to put our heads together 🤔 😄 because yeah, I'm not willing to merge something as potentially destructive as this without cross-platform test coverage.

symlinks are the fscking worst.

[spenczar]

@sdboyer, this is ready for review at last!

[sdboyer]

Gonna try to have a look tonight - sorry for delay!

[sdboyer]

I can't remember if I've pointed you to it before or not, but the work being done in golang/dep#247 might be helpful here.

[spenczar]

Alright, I think we should just ignore junctions if we ever encounter them in this function. I don't think we ever will, though: junctions turn out to have much more in common with hard links than symbolic links, for one thing, and git won't place hard links on your file system. Similarly, msysgit won't put junctions on your file system. It's not at all clear how that would even work, since junctions must point to absolute paths and require lots of permissions to create.

There's also the lack of support in the Go os and filepath packages for handling junctions. It's hard to convince the go standard library's os.Remove call, for example, to remove just the junction link and not the thing it points to. I think you'd need to use the exec package and run linkd to do it. filepath.Walk doesn't behave the way I'd expect with junctions, too.

This function only runs against the code inside ./vendor, so it should only be running against stuff we pulled down with a VCS tool, so it probably won't ever encounter a junction. But if it does, I think the best thing is to ignore these things. Maybe we should raise an error instead, though?

I'm at least confident that we should not try to prune them.

[spenczar]

I can find no way to confidently detect whether a file is a 'regular' symlink versus a junction on windows, other than writing some very gnarly syscalls. The current code checks if a file named vendor is both a symlink and a directory - this only happens on windows, but it happens for both symlinks and junctions. Symlinks named vendor would be safe to os.Remove, but junctions would not. We could just skip both to be safe, or we could try to tell the difference between the two.

syscall.Readlink sniffs to tell the difference in the standard library - it's the only code I've found that gets into the muck of reading the reparse tags that describe this. Beware, unsafe.Pointer calls ahead: https://github.com/golang/go/blob/a1cedf08428bdb91916bb5317c8413212308048c/src/syscall/syscall_windows.go#L1005-L1060

I don't think this sort of thing belongs in gps. It seems way too low-level. I think we should just not support pruning out symlinks on windows. What do you think, @sdboyer?

[sdboyer]

Those look like real failures, no?

[spenczar]

So, tests don't pass on windows, because even a "normal" windows symlink created with os.Symlink which points at a directory looks like a dir and a symlink, not just junctions.

I'm kind of at a loss here. We could detect the difference between Windows symlinks (which can be safely removed) and junctions (which cannot) but we'd have to use a lot of unsafe and syscall to do so. Or, we could just not prune symlinked vendor directories on Windows. I'd like guidance from the benevolent leader here, @sdboyer :)

[spenczar]

This block is what it looks like to determine whether a file is a symlink or a junction, for reference: https://github.com/golang/go/blob/a1cedf08428bdb91916bb5317c8413212308048c/src/syscall/syscall_windows.go#L1007-L1023

[sdboyer]

https://github.com/golang/go/blob/a1cedf08428bdb91916bb5317c8413212308048c/src/syscall/syscall_windows.go#L1007-L1023

aaaaaack hack hack cough cough splutter

Yeah...let's just fall back to this:

we could just not prune symlinked vendor directories on Windows.

I'm content to kick this can down the road for now, as long as we make a TODO in the code and an issue on the repo. First, we have bigger fish to fry right now. Second, I'm wary about making too many choices with respect to symlinks now, as we may be making promises that ultimately aren't consistent with the final vision for the Go tool. Absent someone reporting an actual problem, I'd rather not invest in a bunch of logic that I don't fully understand that we may need to change anyway.

[spenczar]

(ﾉಥ益ಥ）ﾉ sʍopuıʍ

According to some guy on stackoverflow and some other guy on technet, icacls is unable to modify the permissions of a junction itself. It will always modify the junction's target.

There appears to be no way, other than modifying file system metadata directly (probably by using syscall and unsafe), to change the permission of the junction. You're not supposed to be able to list junctions contents - you have to go to the paths inside them directly (according to this blog post, anyway).

Sadly, we're just going to have to drop the test coverage of junctions. Rough.

[sdboyer]

(ﾉಥ益ಥ）ﾉ sʍopuıʍ

this

[sdboyer]

Well, I'm glad we have all the history here - and commits to potentially un-revert with tests - whenever we do decide to tackle this. Thanks for being indefatigable, as ever, @spenczar 😄