-
Notifications
You must be signed in to change notification settings - Fork 24
GPS needs to respect flat packages #181
base: master
Are you sure you want to change the base?
Conversation
I am going to play with the tests in a little, but wanted to push this up so you could see where I was at.. Thoughts? I suppose this is a work in progress PR 😛 |
(I'm very excited that someone else is poking at the actual guts of the algorithm 🎉 🎉 🎊 !!) Hmm...let's start with a definition for "flat package" - do you mean a project containing only the single package at the project root? |
Ah yes, so please forgive my vernacular as I am still learning all of your GPS terms.. but I will at least be consistent and will learn quickly. So to me a flat package is a Go package that contains no other packages inside of it. So yes, a single package, at the root, no other nested packages. With of course typewriter being a great example. |
oh no worries, of course, there's a lot of verbiage in here. |
if !bmi.fromRoot { | ||
// Here we have a flat package, and we don't need to check any dependencies | ||
// for it. There is nothing to check here, so remove it. | ||
s.unsel.remove(bmi) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't doing what you want it to :)
First, if the bmi does represent a "flat package", then that doesn't guarantee anything about whether or not the package has external imports. This point in the algorithm is along the "unselected" path (!is
), meaning that it's the first time we're seeing a bmi for this particular ProjectRoot
. We have to follow the existing logic, because that's what will look at all of its imports, pull in its manifest, and do the satisfiability checks.
If we were on the other branch - is
- then this sort of an escape hatch would make sense. We don't need it, though, because...
Hmm. Well, I'd say that it's because, in selectAtom()
, we only add bmis to the unselected queue if the package set we need is unvisited. But the check there (https://github.com/sdboyer/gps/blob/master/solver.go#L1113-L1124) doesn't seem like quite what I'd expect. Let me refresh myself on it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait, no, it's right. It's checking rpm[pkg] == 1
because the map that's returned tells us the number of things importing on a particular pkg in the current graph.
So, anyway. Because we have that check there, it means that we only add a bmi to the unselected queue if it would add to the package set required from a given project. If there's only one package in a project, and the project's already been pulled in, then that single package has necessarily already been selected, meaning that no new bmi can enter the unselected queue...so the case you're guarding against up in the main solve loop can't occur.
I hope it was vaguely possible to follow that :)
I can't remember if this was still needed, or if the surface problem in dep was fixed by something else... |
Thanks for the ping @sdboyer - I completely forgot about this PR... I am not sure either.. looks like the issue is still open - I will ping you in slack and we can figure it out |
Right now dep was trying to find versions for a bmi if it was a root level package.. This PR bypasses the BMI in this case so we don't fail trying to check on ourselves
Closes golang/dep#270
Also fixes indentation in tracer