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

Improve and guard revision-as-constraint handling #442

Closed
fabulous-gopher opened this issue Apr 21, 2017 · 2 comments
Closed

Improve and guard revision-as-constraint handling #442

fabulous-gopher opened this issue Apr 21, 2017 · 2 comments

Comments

@fabulous-gopher
Copy link

From @sdboyer on July 6, 2016 4:19

#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

Copied from original issue: sdboyer/gps#53

@fabulous-gopher
Copy link
Author

From @sdboyer on April 16, 2017 16:46

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.

@sdboyer
Copy link
Member

sdboyer commented Apr 27, 2017

Reversing course and closing this one, because yeah, we can always reopen later and we don't need more open issues hanging around 😄

@sdboyer sdboyer closed this as completed Apr 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants