sdboyer / gps

your dependencies have arrived
MIT License
270 stars 24 forks source link

Improve and guard revision-as-constraint handling #53

Open sdboyer opened 8 years ago

sdboyer commented 8 years ago

51 left a nasty hole in dealing with #49: if a constraint specifies a revision that doesn't actually exist, it tosses out an error much later in the solve process than it maybe really should. This issue also extends to both preferred versions (#16) and lock versions.

There's a fair bit of logic included that's dedicated to ensuring specified revisions actually exist. But it's currently disabled, first because it's guarding against what seems like a really remote case (at least, with respect to the level of quality we need for MVP), and second because having it in there further, and kinda unconditionally, drags down performance.

There are some approaches that we could take to making this better, but they've all got tradeoffs:

  1. Revision verification in checker step (the current, commented-out approach):
    • Pros:
      • Mostly central - not a lot of checks sprinkled all over
      • Clear (?) fail conditions presented to trace logger
      • Covers lockv and prefv cases as well
    • Cons:
      • Doesn't cover revision constraints declared by root (so, not fully central)
      • Deviates from existing checker pattern (in the way it busts on root)
      • VERY unfriendly to locked versions allowing a fast solve run
  2. Revision lookups done as part of *versionQueue.advance()'s "all loaded" logic
    • Pros:
      • Again, mostly central - covers the above root case
      • Follows the fastpath for locked versions logic
    • Cons:
      • Does nothing for lockv or prefv verification, so not fully central
  3. Optionally verify revisions at the end of solve
    • Pros:
      • Will probably jive nicely with prefetching, and thus the fastpath
      • Fully central
    • Cons:
      • Totally out of character with other checks
      • Weird failure location for nonexistent revs - at the end?
      • Potentially VERY inefficient - if a lockv rev failed in this way, this would pretty much invalidate the entire solve
sdboyer commented 7 years ago

We do have an implementation in now that follows path 1. I'm leaving this open, though, as it's something we might want to reconsider in the future.

fabulous-gopher commented 7 years ago

This issue was moved to golang/dep#442