Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve vendor directory symlinks #535

Merged
merged 2 commits into from
Aug 5, 2016
Merged

Resolve vendor directory symlinks #535

merged 2 commits into from
Aug 5, 2016

Conversation

FugiTech
Copy link
Contributor

@FugiTech FugiTech commented Aug 2, 2016

If vendor is a symlink to another directory (for example, __vendor), filepath.Walk will not descend into it, preventing --strip-vcs and --strip-vendor from working. Appending os.PathSeperator is enough to resolve the symlink.

I'm not sure if this is the ideal solution, but I copied the pattern from glide-vc which works as expected on symlink'd vendor directories.

@mattfarina
Copy link
Member

Thanks for the PR. Before merging I'd like to understand what's going on here and why. Also, I want to make sure it works across nix/windows.

@FugiTech
Copy link
Contributor Author

FugiTech commented Aug 2, 2016

Sure! I made https://github.com/Fugiman/vendortest to try and make this easier to reproduce. I ran glide up -u -v -s in that repo to generate the current state, and then ran rm -rf vendor/**/.git to ensure submodules weren't committed.

This repo demonstrates the current behavior of "glide up -u -v -s does not strip nested vendor directories if vendor is a symlink", and should be able to be used to ensure the patch works on nix/windows (I only tested on mac).


As for why appending / to vendor works: Glide uses glide/path.StripVendor to remove nested vendor directories. That uses filepath.Walk to recurse through the vendor directory. filepath.Walk users os.Lstat to determine if a file is a directory, which does not resolve symlinks. As far as I can tell, what os.Lstat does varies by platform. However, for mac appending / causes os.Lstat to return a FileInfo where IsDir is true.

I could make this behavior more explicit by manually resolving the symlink before passing it into filepath.Walk if you want?

@mattfarina
Copy link
Member

@Fugiman could you detect a symlink and resolve the location? The tailing / is something I don't really trust cross platform which we need to work. There is an IsLink function to detect symlinks. In the resolver there is also a function to return the base directory for a symlink. These may be helpful.

@FugiTech
Copy link
Contributor Author

FugiTech commented Aug 4, 2016

@mattfarina As requested, I now explicitly detect and resolve symlinks. I made the change to Vendor(), and then modified StripVcs() and StripVendor() to use that function.

I added another test to Vendor() to ensure the symlink behavior remains functional, and adjusted the scaffolding of the StripVcs() test to work with the format expected by Vendor().

Please let me know if you'd prefer to have the change in the Strip functions rather than Vendor and I'll move it. I also set it up to resolve nested symlinks, but I don't know if that is overkill or not.

@mattfarina mattfarina merged commit 497a9b1 into Masterminds:master Aug 5, 2016
mattfarina added a commit that referenced this pull request Aug 5, 2016
mattfarina added a commit that referenced this pull request Aug 5, 2016
@mattfarina
Copy link
Member

@Fugiman thanks for the contribution!

@FugiTech FugiTech deleted the resolve_vendor_dir_symlinks branch August 15, 2016 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants