Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

dep sometimes omits a dependency #2001

Closed
benesch opened this issue Sep 5, 2018 · 3 comments
Closed

dep sometimes omits a dependency #2001

benesch opened this issue Sep 5, 2018 · 3 comments
Assignees

Comments

@benesch
Copy link
Contributor

benesch commented Sep 5, 2018

We've been having trouble with Dep over at CockroachDB lately; it's been spuriously dropping a dependency on the floor! I finally managed to get a minimal reproduction.

// main.go
package main

import _ "github.com/shirou/gopsutil/cpu"
import _ "github.com/shirou/gopsutil/host"

func main() {
}

Run dep init once, then the following:

rm Gopkg.lock && dep ensure -v; GOOS=windows go build

That command will non-deterministically fail! Sometimes dep will install the required Windows dependency github.com/shirou/w32; sometimes it won't. I've dug into this to the best of my ability, but the error seems to occur in pkgtree.wmToReach, and I'm afraid that function is well beyond my understanding at the moment.

/cc @dt

@sdboyer
Copy link
Member

sdboyer commented Sep 6, 2018

Interesting! First reported bug in that function since before we released dep publicly :)

i've replicated it locally. This kind of nondeterministic behavior does strongly suggest you've tracked it to the right place, as that function relies on maps (and, evidently, map iteration order) for its depth-first search process.

i'll get it figured out and patched up.

@sdboyer sdboyer self-assigned this Sep 6, 2018
benesch added a commit to benesch/dep that referenced this issue Sep 6, 2018
Determining reachability in the face of cyclic imports requires
"merging" the reachsets of the packages in the cycle. Previously, dep
could nondeterministically lose dependencies in cyclic graphs depending
on the order in which it traversed the graph.

Fix golang#2001.
@benesch
Copy link
Contributor Author

benesch commented Sep 6, 2018

Interesting! First reported bug in that function since before we released dep publicly :)

Heh, no kidding! I guess that checks out: once I dove in it was actually quite easy to follow. It looked far scarier on the surface.

Hopefully you haven't sunk too much time into this. I couldn't help but poke at this some more tonight, and I think I finally cracked it: #2003.

Thanks for being insanely quick to respond! It's much appreciated.

@sdboyer
Copy link
Member

sdboyer commented Sep 6, 2018

Awesome! Thanks for digging into that. And you're lucky, tbqh, i didn't look at the queue much in August 😢

And yeah, the last time we had a bug here, it was the very xtest bug on cycles that you ran into again. Exact same class of problem, owing to the lack of distinguishing information that we have in the search function.

lhauspie pushed a commit to lhauspie/dep that referenced this issue Apr 9, 2019
Determining reachability in the face of cyclic imports requires
"merging" the reachsets of the packages in the cycle. Previously, dep
could nondeterministically lose dependencies in cyclic graphs depending
on the order in which it traversed the graph.

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

No branches or pull requests

2 participants