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

Commit

Permalink
Fix sub dirs
Browse files Browse the repository at this point in the history 
TL;DR: We were vendoring too much, this should fix that w/o breaking
anything (tm).

While working with an internal project and a bunch of go1.6 repos I
observed `go install ./...` trying to compile stuff that is in the
vendor directory, but doesn't have all of the deps also vendored. This
means that they were being copied in incorrectly.

Upon inspection of the code I noticed that vcs files would list all
files and the sub directories and the dependency checks in fill would
skip packages based on import prefix, when (AFAICT) it should only skip
standard and the primary root package importPath.

I also discovered a few tests that appear to be faulty, or exhibit older
behaviour that I never reviewed.

Basically this commit makes it so that when filling godeps we include
all dependencies and when saving we don't automatically process sub
directories of packages unless those sub-directories are also packages
that are required by the dependency search.

This commit adjusts some tests which I hand validated for behaviour.
Most tests though are unchanged. Also added positional identifier for
more save test errors.

More debug added for later operations of save/update.
  • Loading branch information
Edward Muller committed Feb 20, 2016
1 parent 96d8db3 commit 0a24d37
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 17 deletions.
6 changes: 3 additions & 3 deletions godepfile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ func (g *Godeps) fill(pkgs []*Package, destImportPath string) error {
ppln(pkgs)
var err1 error
var path, testImports []string
dipp := []string{destImportPath}
for _, p := range pkgs {
if p.Standard {
log.Println("ignoring stdlib package:", p.ImportPath)
Expand Down Expand Up @@ -110,17 +111,16 @@ func (g *Godeps) fill(pkgs []*Package, destImportPath string) error {
if err != nil {
return err
}
seen := []string{destImportPath}
for _, pkg := range ps {
if pkg.Error.Err != "" {
log.Println(pkg.Error.Err)
err1 = errorLoadingDeps
continue
}
if pkg.Standard || containsPathPrefix(seen, pkg.ImportPath) {
if pkg.Standard || containsPathPrefix(dipp, pkg.ImportPath) {
debugln("standard or dest skipping", pkg.ImportPath)
continue
}
seen = append(seen, pkg.ImportPath)
vcs, reporoot, err := VCSFromDir(pkg.Dir, filepath.Join(pkg.Root, "src"))
if err != nil {
log.Println(err)
Expand Down
4 changes: 4 additions & 0 deletions save.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,7 @@ func copySrc(dir string, deps []Dependency) error {
visited := make(map[string]bool)
ok := true
for _, dep := range deps {
debugln("copySrc for", dep.ImportPath)
srcdir := filepath.Join(dep.ws, "src")
rel, err := filepath.Rel(srcdir, dep.dir)
if err != nil { // this should never happen
Expand All @@ -368,6 +369,7 @@ func copySrc(dir string, deps []Dependency) error {

// copy actual dependency
vf := dep.vcs.listFiles(dep.dir)
debugln("vf", vf)
w := fs.Walk(dep.dir)
for w.Step() {
err = copyPkgFile(vf, dir, srcdir, w)
Expand Down Expand Up @@ -478,8 +480,10 @@ func copyFile(dst, src string) error {
}

if strings.HasSuffix(dst, ".go") {
debugln("Copy Without Import Comment", w, r)
err = copyWithoutImportComment(w, r)
} else {
debugln("Copy (plain)", w, r)
_, err = io.Copy(w, r)
}
err1 := w.Close()
Expand Down
71 changes: 59 additions & 12 deletions save_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ func pkg(name string, imports ...string) string {
return buf.String()
}

func license() string {
return "I AM A LICENSE FILE"
}

func pkgWithTags(name, tags string, imports ...string) string {
return "// +build " + tags + "\n\n" + pkg(name, imports...)
}
Expand Down Expand Up @@ -289,6 +293,7 @@ func TestSave(t *testing.T) {
ImportPath: "C",
Deps: []Dependency{
{ImportPath: "D", Comment: "D1"},
{ImportPath: "D/P", Comment: "D1"},
},
},
},
Expand Down Expand Up @@ -544,7 +549,7 @@ func TestSave(t *testing.T) {
},
want: []*node{
{"C/Godeps/_workspace/src/D/main.go", pkg("D") + decl("D1"), nil},
{"C/Godeps/_workspace/src/D/A/main.go", pkg("A") + decl("A1"), nil},
{"C/Godeps/_workspace/src/D/A/main.go", "(absent)", nil},
},
wdep: Godeps{
ImportPath: "C",
Expand Down Expand Up @@ -824,7 +829,7 @@ func TestSave(t *testing.T) {
},
},
},
{ // 20 - exclude dependency subdirectories even when obtained by a rewritten import path
{ // 20 - include flattened, rewritten deps
cwd: "C",
start: []*node{
{
Expand Down Expand Up @@ -859,12 +864,14 @@ func TestSave(t *testing.T) {
{"C/main.go", pkg("main", "D", "T"), nil},
{"C/Godeps/_workspace/src/D/main.go", pkg("D", "T/X"), nil},
{"C/Godeps/_workspace/src/T/main.go", pkg("T"), nil},
{"C/Godeps/_workspace/src/T/X/main.go", pkg("X"), nil},
},
wdep: Godeps{
ImportPath: "C",
Deps: []Dependency{
{ImportPath: "D", Comment: "D1"},
{ImportPath: "T", Comment: "T1"},
{ImportPath: "T/X", Comment: "T1"},
},
},
},
Expand Down Expand Up @@ -1123,23 +1130,26 @@ func TestSave(t *testing.T) {
"D",
"",
[]*node{
{"LICENSE", pkg("D"), nil},
{"LICENSE", license(), nil},
{"P/main.go", pkg("P"), nil},
{"P/LICENSE", pkg("P"), nil},
{"Godeps/_workspace/src/E/LICENSE", pkg("E"), nil},
{"P/LICENSE", license(), nil},
{"Godeps/_workspace/src/E/LICENSE", license(), nil},
{"Godeps/_workspace/src/E/main.go", pkg("E"), nil},
{"Q/main.go", pkg("Q"), nil},
{"Z/main.go", pkg("Z"), nil},
{"Z/LICENSE", license(), nil},
{"+git", "D1", nil},
},
},
},
want: []*node{
{"C/main.go", pkg("main", "D/P", "D/Q"), nil},
{"C/Godeps/_workspace/src/D/LICENSE", pkg("D"), nil},
{"C/Godeps/_workspace/src/D/LICENSE", license(), nil},
{"C/Godeps/_workspace/src/D/P/main.go", pkg("P"), nil},
{"C/Godeps/_workspace/src/D/P/LICENSE", pkg("P"), nil},
{"C/Godeps/_workspace/src/D/P/LICENSE", license(), nil},
{"C/Godeps/_workspace/src/D/Q/main.go", pkg("Q"), nil},
{"C/Godeps/_workspace/src/D/Godeps/_workspace/src/E/LICENSE", "(absent)", nil},
{"C/Godeps/_workspace/src/D/Godeps/_workspace/src/E/LICENSE", "(absent)", nil}, // E is also not used, technically this wouldn't even be here
{"C/Godeps/_workspace/src/Z/LICENSE", "(absent)", nil}, // Z Isn't a dep, so shouldn't have a LICENSE file.
},
wdep: Godeps{
ImportPath: "C",
Expand Down Expand Up @@ -1179,6 +1189,7 @@ func TestSave(t *testing.T) {
ImportPath: "C",
Deps: []Dependency{
{ImportPath: "D", Comment: "D1"},
{ImportPath: "D/P", Comment: "D1"},
},
},
},
Expand Down Expand Up @@ -1446,6 +1457,42 @@ func TestSave(t *testing.T) {
},
},
},
{ // 37 Do not copy in sub directories that aren't required
vendor: true,
cwd: "C",
start: []*node{
{
"C",
"",
[]*node{
{"main.go", pkg("main", "D"), nil},
{"+git", "", nil},
},
},
{
"D",
"",
[]*node{
{"main.go", pkg("D"), nil},
{"sub/main.go", pkg("sub"), nil},
{"sub/sub/main.go", pkg("subsub"), nil},
{"+git", "D1", nil},
},
},
},
want: []*node{
{"C/main.go", pkg("main", "D"), nil},
{"C/vendor/D/main.go", pkg("D"), nil},
{"C/vendor/D/sub/main.go", "(absent)", nil},
{"C/vendor/D/sub/sub/main.go", "(absent)", nil},
},
wdep: Godeps{
ImportPath: "C",
Deps: []Dependency{
{ImportPath: "D", Comment: "D1"},
},
},
},
}

wd, err := os.Getwd()
Expand Down Expand Up @@ -1482,7 +1529,7 @@ func TestSave(t *testing.T) {
if err != nil {
t.Log(pos, err)
}
t.Errorf("save err = %v want %v", g, test.werr)
t.Errorf("%d save err = %v want %v", pos, g, test.werr)
}
err = os.Chdir(wd)
if err != nil {
Expand All @@ -1503,13 +1550,13 @@ func TestSave(t *testing.T) {
f.Close()

if g.ImportPath != test.wdep.ImportPath {
t.Errorf("ImportPath = %s want %s", g.ImportPath, test.wdep.ImportPath)
t.Errorf("%d ImportPath = %s want %s", pos, g.ImportPath, test.wdep.ImportPath)
}
for i := range g.Deps {
g.Deps[i].Rev = ""
}
if !reflect.DeepEqual(g.Deps, test.wdep.Deps) {
t.Errorf("Deps = %v want %v", g.Deps, test.wdep.Deps)
t.Errorf("%d Deps = %v want %v", pos, g.Deps, test.wdep.Deps)
}
}
}
Expand Down Expand Up @@ -1544,7 +1591,7 @@ func makeTree(t *testing.T, tree *node, altpath string) (gopath string) {
case n.path == "+git":
dir := filepath.Dir(path)
run(t, dir, "git", "init") // repo might already exist, but ok
run(t, dir, "git", "add", ".")
run(t, dir, "git", "add", "-A", ".")
run(t, dir, "git", "commit", "-m", "godep")
if body != "" {
run(t, dir, "git", "tag", body)
Expand Down
8 changes: 6 additions & 2 deletions vcs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ func (v *VCS) identify(dir string) (string, error) {

func (v *VCS) root(dir string) (string, error) {
out, err := v.runOutput(dir, v.RootCmd)
return string(bytes.TrimSpace(out)), err
return filepath.Clean(string(bytes.TrimSpace(out))), err
}

func (v *VCS) describe(dir, rev string) string {
Expand Down Expand Up @@ -146,6 +146,7 @@ func (vf vcsFiles) Contains(path string) bool {
// listFiles tracked by the VCS in the repo that contains dir, converted to absolute path.
func (v *VCS) listFiles(dir string) vcsFiles {
root, err := v.root(dir)
debugln("vcs root", root)
if err != nil {
return nil
}
Expand All @@ -160,7 +161,10 @@ func (v *VCS) listFiles(dir string) vcsFiles {
if err != nil {
panic(err) // this should not happen
}
files[path] = true

if filepath.Dir(path) == dir {
files[path] = true
}
}
}
return files
Expand Down

0 comments on commit 0a24d37

Please sign in to comment.