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

godep save results in incomplete dependency list #27

Closed
wants to merge 1 commit into from

Conversation

babldev
Copy link

@babldev babldev commented Nov 14, 2013

Steps to reproduce:

go get github.com/crowdmob/goamz/sqs
godep save

This results in the following dependency:

github.com/crowdmob/goamz/aws

... which is incorrect. The sqs directory depends on the aws directory, but because the aws is first alphabetically and in the same git repo, the sqs directory is ignored. This change is probably overkill, since it adds multiple dependencies per git repo, but it at least makes godep save work. I'm happy to hear feedback on this PR!

github.com/crowdmob/goamz/sqs

Resulted in the `godep save` dependency:

github.com/crowdmob/goamz/aws

... which is incorrect.
@kr
Copy link
Member

kr commented Nov 14, 2013

Where did you run the godep save command?
Which project or package was it?

@babldev
Copy link
Author

babldev commented Nov 14, 2013

It's for a private repo, but I ran it in the root of my git repo -- effectively src/github.com/org/project of the GOPATH. It runs without error, but Load() only looks for one dependency per git repo and it picked the wrong one. This was because of the logic of the if contains(seen, importPath) { continue }

If this is too vague I can give more specific repro steps using your Heroku godeps example.

@babldev
Copy link
Author

babldev commented Nov 14, 2013

Also, thanks for making Godeps and the buildpack!

@kr
Copy link
Member

kr commented Nov 15, 2013

Bear in mind that Godeps.json doesn't just list the direct dependencies
of your project, it lists all transitive dependencies. Both packages sqs
and aws are dependencies of your package, but as you've observed,
we only include one entry for each repo, and the choice is arbitrary
(currently alphabetical). Neither one is right or wrong; they're both
dependencies of your project.

Would you feel more comfortable with github.com/crowdmob/goamz
as the path listed in Godeps.json? I've been thinking about making
that change. (Arguably the name ImportPath is no longer correct,
since it can be a directory with no .go files, as in this case.)

@kr
Copy link
Member

kr commented Nov 15, 2013

And, you're welcome! :)

@kr kr mentioned this pull request Nov 15, 2013
@babldev
Copy link
Author

babldev commented Nov 15, 2013

I'd be comfortable with using the RepoRoot. It doesn't guarantee a 1:1 mapping with what is brought in via go get ..., but it is atleast a superset of the dependencies instead of a subset.

After looking at #29, I just wanted to clarify that what is being saved to Godeps.json is in fact wrong and causes godep save to only cache github.com/crowdmob/goamz/aws and not github.com/crowdmob/goamz/sqs inside of the _workspace directory, despite both living in the same git repo.

@kr
Copy link
Member

kr commented Nov 16, 2013

My apologies, I misunderstood the problem here.

The save command needs to copy both packages aws
and sqs, and it's only copying aws. Yes, I think the simplest
solution is to move to using the repo root for both the json
entry and the path to copy in.

@kr
Copy link
Member

kr commented Nov 16, 2013

If you'd like to update this pull request to use the repo root
(but keep the json field name "ImportPath" for now), go for it.
Otherwise I'll take care of it.

@kr kr mentioned this pull request Nov 18, 2013
@kr kr closed this in e271736 Nov 18, 2013
@kr
Copy link
Member

kr commented Nov 18, 2013

Cc @goinggo you were interested in this issue as well.
It should be fixed. Please reopen or open a new issue
if you still see problems.

@goinggo
Copy link

goinggo commented Nov 18, 2013

Cc @goinggo you were interested in this issue as well.
It should be fixed. Please reopen or open a new issue
if you still see problems.

Now the entire projects are being brought into the Godeps folder. I didn't realize till just now you were only bring in the source code being used.


Reply to this email directly or view it on GitHub.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants