Merge into trunk : 004-no-go-get : Code : godeps

Status:	Merged
Merged at revision:	29
Proposed branch:	lp://staging/~rogpeppe/godeps/004-no-go-get
Merge into:	lp://staging/godeps
Diff against target:	124 lines (+62/-10) 1 file modified godeps.go (+62/-10)
To merge this branch:	bzr merge lp://staging/~rogpeppe/godeps/004-no-go-get
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
godeps-maintainers		2015-05-01	Pending
Review via email: mp+258017@code.staging.launchpad.net

Revision history for this message

Roger Peppe (rogpeppe) wrote on 2015-05-01:

#

Reviewers: mp+258017_code.launchpad.net,

Message:
Please take a look.

Description:
do not use go get to fetch new repositories

"go get" always recursively fetches dependencies,
and this feature cannot be disabled. This means that
when godeps used "go get -d" to fetch a repository,
it could fetch unwanted tip dependencies.

https://code.launchpad.net/~rogpeppe/godeps/004-no-go-get/+merge/258017

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/230460044/

Affected files (+59, -10 lines):
A [revision details]
M godeps.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: godeps.go
=== modified file 'godeps.go'
--- godeps.go 2014-12-04 16:32:37 +0000
+++ godeps.go 2015-05-01 11:00:22 +0000
@@ -19,6 +19,8 @@
"time"

"github.com/kisielk/gotool"
+
+ "launchpad.net/godeps/pkgrepo"
)

  var (
@@ -155,16 +157,8 @@
   }
   fmt.Printf("update %s failed; trying to fetch newer version\n",
info.project)
   if info.notThere {
- _, err := runCmd(".", "go", "get", "-d", info.project)
- // Note that we can't add a "/..." to the end of info.project, because
- // that causes go get to fail for vanity import paths with an
- // "unrecognized import path" error. But when we *don't*
- // use /..., we get a "no buildable Go source files" error when
- // the root of the repository does not contain any Go files.
- // It's a no-win situation, so we just ignore the latter error.
- // See https://code.google.com/p/go/issues/detail?id=8335
- if err != nil && !strings.Contains(err.Error(), "no buildable Go source
files") {
- return err
+ if err := createRepo(info); err != nil {
+ return fmt.Errorf("cannot create repo: %v", err)
    }
   } else {
    if err := info.vcs.Fetch(info.dir); err != nil {
@@ -174,6 +168,43 @@
   return info.vcs.Update(info.dir, info.revid)
  }

+func createRepo(info *depInfo) error {
+ // We would much prefer to just do:
+ //
+ // _, err := runCmd(".", "go", "get", "-nodeps", "-d", project)
+ //
+ // here, but there's no way to prevent go get from downloading
+ // dependencies recursively, which means that we can get
+ // extraneous dependencies when the package tip has dependencies
+ // not mentioned by the target revision, which can in turn cause
+ // build scripts that are particular about such things to fail.
+ // See also https://go-review.googlesource.com/#/c/8725/
+ //
+ // Instead, we use code abstracted from the go tool to do the
+ // job.
+ root, err := pkgrepo.Root(info.project)
+ if err != nil {
+ return fmt.Errorf("cannot find project root: %v", err)
+ }
+ rootDir := filepath.Join(buildContext.GOPATH, "src",
filepath.FromSlash(root.Root))
+ if string(root.VCS) != info.vcs.Kind() {
+ return fmt.Errorf("project has unexpected VCS kind %s; want %s",
root.VCS, info.vcs.Kind())
+ }
+
+ // The rest of this function is also taken directly from
+ // the downloadPackage funct...

Reviewers: mp+258017_code.launchpad.net,

Message:
Please take a look.

Description:
do not use go get to fetch new repositories

"go get" always recursively fetches dependencies,
and this feature cannot be disabled. This means that
when godeps used "go get -d" to fetch a repository,
it could fetch unwanted tip dependencies.

https://code.launchpad.net/~rogpeppe/godeps/004-no-go-get/+merge/258017

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/230460044/

Affected files (+59, -10 lines):
   A [revision details]
   M godeps.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details]	2012-01-01 00:00:00 +0000
+++ [revision details]	2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: roger.peppe@canonical.com-20150424154549-wmf75c3uzogyxy2v
+New revision: roger.peppe@canonical.com-20150501110022-avvyfwpgw7wif60o

Index: godeps.go
=== modified file 'godeps.go'
--- godeps.go	2014-12-04 16:32:37 +0000
+++ godeps.go	2015-05-01 11:00:22 +0000
@@ -19,6 +19,8 @@
  	"time"

"github.com/kisielk/gotool"
+
+	"launchpad.net/godeps/pkgrepo"
  )

var (
@@ -155,16 +157,8 @@
  	}
  	fmt.Printf("update %s failed; trying to fetch newer version\n",  
info.project)
  	if info.notThere {
-		_, err := runCmd(".", "go", "get", "-d", info.project)
-		// Note that we can't add a "/..." to the end of info.project, because
-		// that causes go get to fail for vanity import paths with an
-		// "unrecognized import path" error. But when we *don't*
-		// use /..., we get a "no buildable Go source files" error when
-		// the root of the repository does not contain any Go files.
-		// It's a no-win situation, so we just ignore the latter error.
-		// See https://code.google.com/p/go/issues/detail?id=8335
-		if err != nil && !strings.Contains(err.Error(), "no buildable Go source  
files") {
-			return err
+		if err := createRepo(info); err != nil {
+			return fmt.Errorf("cannot create repo: %v", err)
  		}
  	} else {
  		if err := info.vcs.Fetch(info.dir); err != nil {
@@ -174,6 +168,43 @@
  	return info.vcs.Update(info.dir, info.revid)
  }

+func createRepo(info *depInfo) error {
+	// We would much prefer to just do:
+	//
+	//	_, err := runCmd(".", "go", "get", "-nodeps", "-d", project)
+	//
+	// here, but there's no way to prevent go get from downloading
+	// dependencies recursively, which means that we can get
+	// extraneous dependencies when the package tip has dependencies
+	// not mentioned by the target revision, which can in turn cause
+	// build scripts that are particular about such things to fail.
+	// See also https://go-review.googlesource.com/#/c/8725/
+	//
+	// Instead, we use code abstracted from the go tool to do the
+	// job.
+	root, err := pkgrepo.Root(info.project)
+	if err != nil {
+		return fmt.Errorf("cannot find project root: %v", err)
+	}
+	rootDir := filepath.Join(buildContext.GOPATH, "src",  
filepath.FromSlash(root.Root))
+	if string(root.VCS) != info.vcs.Kind() {
+		return fmt.Errorf("project has unexpected VCS kind %s; want %s",  
root.VCS, info.vcs.Kind())
+	}
+
+	// The rest of this function is also taken directly from
+	// the downloadPackage function in the go tool.
+
+	// Some version control tools require the parent of the target to exist.
+	parent, _ := filepath.Split(rootDir)
+	if err := os.MkdirAll(parent, 0777); err != nil {
+		return err
+	}
+	if err := info.vcs.Create(root.Repo, rootDir); err != nil {
+		return err
+	}
+	return nil
+}
+
  func parseDepFile(file string) (map[string]*depInfo, error) {
  	f, err := os.Open(file)
  	if err != nil {
@@ -515,6 +546,7 @@
  	Kind() string
  	Info(dir string) (VCSInfo, error)
  	Update(dir, revid string) error
+	Create(repo, rootDir string) error
  	Fetch(dir string) error
  }

@@ -577,6 +609,11 @@
  	}, nil
  }

+func (gitVCS) Create(repo, rootDir string) error {
+	_, err := runUpdateCmd("", "git", "clone", repo, rootDir)
+	return err
+}
+
  func (gitVCS) Update(dir string, revid string) error {
  	_, err := runUpdateCmd(dir, "git", "checkout", revid)
  	return err
@@ -626,6 +663,11 @@
  	}, nil
  }

+func (bzrVCS) Create(repo, rootDir string) error {
+	_, err := runUpdateCmd("", "bzr", "branch", repo, rootDir)
+	return err
+}
+
  func (bzrVCS) Update(dir string, revid string) error {
  	_, err := runUpdateCmd(dir, "bzr", "update", "-r", "revid:"+revid)
  	return err
@@ -665,6 +707,11 @@
  	return "hg"
  }

+func (hgVCS) Create(repo, rootDir string) error {
+	_, err := runUpdateCmd("", "hg", "clone", "-U", repo, rootDir)
+	return err
+}
+
  func (hgVCS) Update(dir string, revid string) error {
  	_, err := runUpdateCmd(dir, "hg", "update", revid)
  	return err

Revision history for this message

Matthew Williams (mattyw) wrote on 2015-05-01:

#

LGTM

some tests might be useful if you're so inclined.

https://codereview.appspot.com/230460044/diff/1/godeps.go
File godeps.go (right):

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode172
godeps.go:172: // We would much prefer to just do:
I think comment would serve better as a go for the function rathen than
in the body.

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode183
godeps.go:183: // Instead, we use code abstracted from the go tool to do
the
abstracted or copied?

or inspired by/ in memory of etc

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode183
godeps.go:183: // Instead, we use code abstracted from the go tool to do
the
Also, this comment can stay where it is I think: Here we use code ...

https://codereview.appspot.com/230460044/

Revision history for this message

Roger Peppe (rogpeppe) wrote on 2015-05-01:

#

https://codereview.appspot.com/230460044/diff/1/godeps.go
File godeps.go (right):

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode172
godeps.go:172: // We would much prefer to just do:
On 2015/05/01 11:26:58, mattyw wrote:
> I think comment would serve better as a go for the function rathen
than in the
> body.

The comment is about the implementation of the function.
Doc comments are usually about what the function does,
not how it's done, so I think this is in the right place.

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode183
godeps.go:183: // Instead, we use code abstracted from the go tool to do
the
On 2015/05/01 11:26:58, mattyw wrote:
> Also, this comment can stay where it is I think: Here we use code ...

Ack but I think it's ok as it is.

https://codereview.appspot.com/230460044/

Revision history for this message

Roger Peppe (rogpeppe) wrote on 2015-05-01:

#

On 2015/05/01 11:26:58, mattyw wrote:
> LGTM

> some tests might be useful if you're so inclined.

Tests would of course be great, but I can't think
of decent offline tests that don't involve me
building an entire mock git, mercurial and bzr servers.

I did at least QA it. I did this:

I created two repositories, github.com/rogpeppe/test
and github.com/rogpeppe/test2. The first was a library package
that initially depended on gopkg.in/errgo.v1 only; the second
was a main package that depended on test2 only, and had
a godeps file.

I created a new GOPATH directory, cloned github.com/rogpeppe/test2
into $GOPATH/src/github.com/rogpeppe/test2, then ran
godeps -u on it. It worked OK.

Then I pushed a new commit to github.com/rogpeppe/test that
used a new dependency (gopkg.in/mgo.v2), and followed the
above steps again. The packages were fetched without gopkg.in/mgo.v2,
which was the aim of the whole thing.

I then checked that godeps -u worked correctly on the juju repo
with a fresh checkout. It did.

If anyone can think of something else that might break it, please
give it a go.

https://codereview.appspot.com/230460044/

Revision history for this message

Casey Marshall (cmars) wrote on 2015-05-01:

#

https://codereview.appspot.com/230460044/diff/1/godeps.go
File godeps.go (right):

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode189
godeps.go:189: rootDir := filepath.Join(buildContext.GOPATH, "src",
filepath.FromSlash(root.Root))
What if $GOPATH is a list of paths?

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode200
godeps.go:200: return err
Might be nice to annotate the cause of this error, to help the user fix
their GOPATH.

https://codereview.appspot.com/230460044/

Revision history for this message

Roger Peppe (rogpeppe) wrote on 2015-05-01:

#

https://codereview.appspot.com/230460044/diff/1/godeps.go
File godeps.go (right):

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode189
godeps.go:189: rootDir := filepath.Join(buildContext.GOPATH, "src",
filepath.FromSlash(root.Root))
On 2015/05/01 17:36:23, cmars_ wrote:
> What if $GOPATH is a list of paths?

Yes! This is why we have code reviews. Given the lengths the rest of
this piece of code goes to to deal with multiple GOPATH elements you
might have thought I'd remember that here... Thanks, will fix.

https://codereview.appspot.com/230460044/

Revision history for this message

Roger Peppe (rogpeppe) wrote on 2015-05-05:

#

Please take a look.

https://codereview.appspot.com/230460044/

Revision history for this message

Matthew Williams (mattyw) wrote on 2015-05-05:

#

https://codereview.appspot.com/230460044/diff/20001/godeps.go
File godeps.go (right):

https://codereview.appspot.com/230460044/diff/20001/godeps.go#newcode189
godeps.go:189: rootDir := filepath.Join(buildContext.GOPATH, "src",
filepath.FromSlash(root.Root))
Isn't this code still wrong for GOPATHS with multiple entries?

https://codereview.appspot.com/230460044/

Revision history for this message

Roger Peppe (rogpeppe) wrote on 2015-05-05:

#

https://codereview.appspot.com/230460044/diff/1/godeps.go
File godeps.go (right):

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode200
godeps.go:200: return err
On 2015/05/01 17:36:23, cmars_ wrote:
> Might be nice to annotate the cause of this error, to help the user
fix their
> GOPATH.

I can't currently think of a decent way of annotating the message
that won't just duplicate the info in the error message we're
getting from MkdirAll, which will include the directory
name and what it's trying to do. Suggestions?

https://codereview.appspot.com/230460044/

lp://staging/~rogpeppe/godeps/004-no-go-get updated on 2015-05-05

30. By Roger Peppe on 2015-05-05: adjust to work with multiple-element GOPATH

Revision history for this message

Roger Peppe (rogpeppe) wrote on 2015-05-05:

#

Please take a look.

https://codereview.appspot.com/230460044/

Revision history for this message

Matthew Williams (mattyw) wrote on 2015-05-05:

#

LGTM - but I didn't notice the last problem - so get a review from
someone else

https://codereview.appspot.com/230460044/

Revision history for this message

Martin Hilton (martin-hilton) wrote on 2015-05-05:

#

On 2015/05/05 08:25:28, rog wrote:
> Please take a look.

LGTM

https://codereview.appspot.com/230460044/

Revision history for this message

Roger Peppe (rogpeppe) wrote on 2015-05-05:

#

*** Submitted:

do not use go get to fetch new repositories

"go get" always recursively fetches dependencies,
and this feature cannot be disabled. This means that
when godeps used "go get -d" to fetch a repository,
it could fetch unwanted tip dependencies.

R=mattyw, cmars_, martin.hilton
CC=
https://codereview.appspot.com/230460044

https://codereview.appspot.com/230460044/

godeps

Merge lp://staging/~rogpeppe/godeps/004-no-go-get into lp://staging/godeps

Commit message

Description of the change

Preview Diff

Subscribers