Use off-the-shelf dependency resolver instead of handrolled code#241
Draft
bbannier wants to merge 11 commits into
Draft
Use off-the-shelf dependency resolver instead of handrolled code#241bbannier wants to merge 11 commits into
bbannier wants to merge 11 commits into
Conversation
`semver.Spec` raises `PendingDeprecationWarning` as of semantic-version 2.10 and will be removed in 3.1. `semver.SimpleSpec` is the direct replacement with an identical interface.
The extraction sets up a clean seam for the next commit, which switches to reading metadata via `git show` so deps can be fetched at arbitrary versions rather than only the currently checked-out one.
`PackageInfo` only reflects the currently checked-out version. Reading deps at a specific tag via `git show` is what lets the nab-resolver provider fetch accurate constraints for every version it considers, not just the one on disk.
The (raw_tag, coerced_version) pairing is needed in several places that follow; keeping it in one place also makes the filtering logic testable.
Add the nab-resolver dependency and the resolver infrastructure in `zeekpkg/_resolver.py`. `validate_dependencies` is not yet wired to use it -- keeping the wiring separate lets each commit stay reviewable. The behaviour is pinned by unit tests in `test_resolver.py` so the wiring commit can focus purely on the call site.
Without seeding installed and builtin packages into the graph at the start, the solver sees no version information for them and cannot enforce their constraints against incoming dependency requests. Seeding upfront also ensures zeek and zkg appear as nodes the solver can pin.
Captures the expected failure when a newly requested package requires a newer version of an already-installed one. The baseline reflects hard-pinning, which the following commit relaxes -- at that point the test will start showing the upgraded version in the installed list.
Wire `_ZkgProvider` into `validate_dependencies` and replace the BFS conflict-detection loop with a nab-resolver call. The `constraints=` parameter is the key semantic difference from the old approach: installed packages are pinned there rather than by filtering which versions get registered, so the provider always sees the full version history and the solver decides what fits. Three non-obvious cases required hardening before this could work in practice. Packages without any semver git tags need a fallback version (we try `metadata_version`, installed state, `versions[-1]`, then `0.0.0`) or the solver has nothing to pick and rejects every request. Directory- backed packages have a `metadata_file` path but no git repository, so `git.Repo()` would crash without an `InvalidGitRepositoryError` guard. And nab-resolver formats unsatisfied ranges as `(-inf, X) | (X, +inf)`, which we intercept in `narrow_for_display` to produce readable output.
Hard-pinning all installed packages blocked the common case where a new package requires a newer version of something already installed. Directory- backed packages and zeek/zkg stay hard-pinned because zkg has no mechanism to upgrade them.
`manager.py` imported nab-resolver types directly. Move `_run_solver` (resolver construction, DFS topo sort) into `_resolver.py` and re-export `Range` from there so nab-resolver stays an implementation detail of the resolver module. `manager.py` now only passes in already-constructed `Range` values and a `lookup_dep` callable.
nab-resolver's error reporter renders constraints via two code paths: _narrow_positive (which calls narrow_for_display) and direct interpolation of Incompatibility.constraint_range in the CONSTRAINT-cause path. The previous narrow_for_display approach only covered the first path, leaving CONSTRAINT-cause lines with raw (-inf, X) sentinel notation. _FmtRange overrides __str__ to produce operator-prefixed output and overrides all Range operators to preserve the subclass through composition. _constraint_to_range now returns _FmtRange, so every range we construct formats itself at every render site. narrow_for_display becomes a one-line fallback that wraps any library-internal Range the resolver might surface directly.
bbannier
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We were using a hand-rolled dependency solver which had a number of issues, e.g.,
zkg.metaand used that for all versions which probably is not a valid assumption to make in generalThis PR moves us to an off-the-shelf pubgrub-style library instead which can address all of the above shortcomings. While we might not exactly shave off lines of code (we add almost 500 loc), we now make things more explicit. We can also unit test dependency resolution better now.
Warning