Skip to content
This repository has been archived by the owner on Feb 3, 2018. It is now read-only.

Follow symlinks when stripping vendor directories #199

Merged
merged 22 commits into from
Apr 13, 2017
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
3b518fc
Follow symlinks when stripping vendor directories
spenczar Mar 20, 2017
5bf17a1
Add test of inDirectory function
spenczar Mar 21, 2017
352abcf
Remove symlinks named 'vendor' when trimming
spenczar Mar 21, 2017
21bb8a5
Make OS-agnostic link targets in test
spenczar Mar 21, 2017
00bcf14
Reorder fs test check of symlink and directory
spenczar Mar 21, 2017
c281797
Use absolute symlinks when testing on windows
spenczar Mar 21, 2017
b0151a8
Give tested monitored cmd a little slack
spenczar Mar 21, 2017
8720092
Revert change to TestMonitoredCmd
spenczar Mar 21, 2017
c2a099c
Fix bad Errorf argument in tests
spenczar Apr 1, 2017
42ce45c
Run vendor stripping tests against junctions on windows
spenczar Apr 2, 2017
3e1d193
Fix comment typos
spenczar Apr 2, 2017
d59bce6
Don't delete target directory of junctions
spenczar Apr 2, 2017
87ee43d
Create junction to an absolute path
spenczar Apr 2, 2017
5e19c27
Skip junctions when pruning out recursive vendor dirs
spenczar Apr 2, 2017
c32885d
Remove nearly-empty test file
spenczar Apr 10, 2017
6eda647
Clean up file system tests from CR comments
spenczar Apr 10, 2017
06bcb96
Only remove symlinks to vendor dirs on non-Windows OSes
spenczar Apr 10, 2017
edee8c8
Add TODO note about improving windows symlink support
spenczar Apr 10, 2017
9a38a0e
Remove duplicated windows test
spenczar Apr 10, 2017
e640789
Set permissions on created junctions so filepath.Walk works
spenczar Apr 10, 2017
853e768
Call icacls on the junction link itself
spenczar Apr 10, 2017
05ef430
Remove tests of junctions
spenczar Apr 11, 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
34 changes: 34 additions & 0 deletions filesystem_nonwindows_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// +build !windows

package gps

import (
"os"
"testing"
)

// setup inflates fs onto the actual host file system
func (fs filesystemState) setup(t *testing.T) {
for _, dir := range fs.dirs {
p := dir.prepend(fs.root)
if err := os.MkdirAll(p.String(), 0777); err != nil {
t.Fatalf("os.MkdirAll(%q, 0777) err=%q", p, 0777)
Copy link
Owner

Choose a reason for hiding this comment

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

seems like second param should be err

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! Will do.

}
}
for _, file := range fs.files {
p := file.prepend(fs.root)
f, err := os.Create(p.String())
if err != nil {
t.Fatalf("os.Create(%q) err=%q", p, err)
}
if err := f.Close(); err != nil {
t.Fatalf("file %q Close() err=%q", p, err)
}
}
for _, link := range fs.links {
p := link.path.prepend(fs.root)
if err := os.Symlink(link.to, p.String()); err != nil {
t.Fatalf("os.Symlink(%q, %q) err=%q", link.to, p, err)
}
}
}
108 changes: 108 additions & 0 deletions filesystem_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
package gps

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

// This file contains utilities for running tests around file system state.

// fspath represents a file system path in an OS-agnostic way.
type fsPath []string

func (f fsPath) String() string { return filepath.Join(f...) }

func (f fsPath) prepend(prefix string) fsPath {
p := fsPath{prefix}
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's worth wrapping prefix here in a filepath.FromSlash(). I realize it's not necessary to do with the current code, as ioutil.TempDir() is the only thing that ends up populating that var, but it takes away a footgun in the event that it gets reused differently in the future by someone not developing on windows.

return append(p, f...)
}

// filesystemState represents the state of a file system. It has a setup method
// which inflates its state to the actual host file system, and an assert
// method which checks that the actual file system matches the described state.
type filesystemState struct {
root string
dirs []fsPath
files []fsPath
links []fsLink
}

// assert makes sure that the fs state matches the state of the actual host
// file system
func (fs filesystemState) assert(t *testing.T) {
dirMap := make(map[string]struct{})
fileMap := make(map[string]struct{})
linkMap := make(map[string]struct{})

for _, d := range fs.dirs {
dirMap[d.prepend(fs.root).String()] = struct{}{}
}
for _, f := range fs.files {
fileMap[f.prepend(fs.root).String()] = struct{}{}
}
for _, l := range fs.links {
linkMap[l.path.prepend(fs.root).String()] = struct{}{}
}

err := filepath.Walk(fs.root, func(path string, info os.FileInfo, err error) error {
if err != nil {
t.Errorf("filepath.Walk path=%q err=%q", path, err)
return err
}

if path == fs.root {
return nil
}

// Careful! Have to check whether the path is a symlink first because, on
// windows, a symlink to a directory will return 'true' for info.IsDir().
if (info.Mode() & os.ModeSymlink) != 0 {
_, ok := linkMap[path]
if !ok {
t.Errorf("unexpected symlink exists %q", path)
} else {
delete(linkMap, path)
}
return nil
}

if info.IsDir() {
_, ok := dirMap[path]
if !ok {
Copy link
Owner

Choose a reason for hiding this comment

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

Let's change dirMap, fileMap and linkMap all into map[string]bool. Yes, it's a bit more sloppy underneath, but it makes these if statements more readable. Because the zero value of bool is false, we can just:

if dirMap[path] {
  // ...
else {
  ...
}

instead of having to do the two-value map lookup return style.

t.Errorf("unexpected directory exists %q", path)
} else {
delete(dirMap, path)
}
return nil
}

_, ok := fileMap[path]
if !ok {
t.Errorf("unexpected file exists %q", path)
} else {
delete(fileMap, path)
}
return nil
})

if err != nil {
t.Errorf("filesystem.Walk err=%q", err)
}

for d := range dirMap {
t.Errorf("could not find expected directory %q", d)
}
for f := range fileMap {
t.Errorf("could not find expected file %q", f)
}
for l := range linkMap {
t.Errorf("could not find expected symlink %q", l)
}
}

// fsLink represents a symbolic link.
type fsLink struct {
path fsPath
to string
}
42 changes: 42 additions & 0 deletions filesystem_windows_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// +build windows

package gps

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

// setup inflates fs onto the actual host file system
func (fs filesystemState) setup(t *testing.T) {
for _, dir := range fs.dirs {
p := dir.prepend(fs.root)
if err := os.MkdirAll(p.String(), 0777); err != nil {
t.Fatalf("os.MkdirAll(%q, 0777) err=%q", p, 0777)
Copy link
Owner

Choose a reason for hiding this comment

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

ibid above

}
}
for _, file := range fs.files {
p := file.prepend(fs.root)
f, err := os.Create(p.String())
if err != nil {
t.Fatalf("os.Create(%q) err=%q", p, err)
}
if err := f.Close(); err != nil {
t.Fatalf("file %q Close() err=%q", p, err)
}
}
for _, link := range fs.links {
p := link.path.prepend(fs.root)

// On Windows, relative symlinks confuse filepath.Walk. This is golang/go
// issue 17540. So, we'll just sigh and do absolute links, assuming they are
// relative to the directory of link.path.
dir := filepath.Dir(p.String())
to := filepath.Join(dir, link.to)

if err := os.Symlink(to, p.String()); err != nil {
t.Fatalf("os.Symlink(%q, %q) err=%q", to, p, err)
}
}
}
22 changes: 22 additions & 0 deletions result.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,25 @@ func (r solution) Attempts() int {
func (r solution) InputHash() []byte {
return r.hd
}

func stripVendor(path string, info os.FileInfo, err error) error {
if info.Name() == "vendor" {
if _, err := os.Lstat(path); err == nil {
if info.IsDir() {
return removeAll(path)
}

if (info.Mode() & os.ModeSymlink) != 0 {
Copy link
Owner

Choose a reason for hiding this comment

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

wait - shouldn't this check be first, because Windows junction dirs? i must be missing something...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You probably aren't missing anything. I don't know much of anything about Windows and don't know what a junction dir is. Should these be reversed? What's the test that would prove it?

Copy link
Owner

Choose a reason for hiding this comment

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

hah, i scarcely know either. it's just problems discussed in golang/go#17540 - Windows APIs can indicate that a file is both a symlink AND a dir. they do it when it's one of these "junction dirs."

Bottom line is, this approach on windows would potentially (?) erase the target of the symlink.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, gotcha. I'll try to figure out how to create a junction dir and write a test that exercises this.

For what it's worth, if this erases the target of symlinks, then we're already doing that today.

realInfo, err := os.Stat(path)
if err != nil {
return err
}
if realInfo.IsDir() {
return os.Remove(path)
}
}
}
}

return nil
}
Loading