-
Notifications
You must be signed in to change notification settings - Fork 174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Iterative dependency resolver #971
base: master
Are you sure you want to change the base?
Conversation
The only standing CI failure is:
It seems that the current master only tries develop versions installed, whereas this branch will try to find a compatible version from the package_list even if there is a develop file (and fails because no version > 0.1.0 exists) Apart from that, CI is green, should be ready for review |
let dependencies = pkgInfo.processFreeDependencies(options).map( | ||
pkg => pkg.toFullInfo(options)).toSeq | ||
let | ||
packageList = getInstalledPkgsMin(options.getPkgsDir(), options) | ||
developDeps = processDevelopDependencies(pkgInfo, options) | ||
installedInfo = installIteration(concat(packageList, developDeps), pkgInfo.requires, options) | ||
dependencies = | ||
block: | ||
# To avoid warning on `map` | ||
var res = toSeq(installedInfo.installed.values) | ||
res.apply(x => x.toFullInfo(options)) | ||
res | ||
dependenciesGraph = dependencies.buildLockFileDeps(installedInfo.dependencies, options) | ||
|
||
pkgInfo.validateDevelopDependenciesVersionRanges(dependencies, options) | ||
var dependencyGraph = buildDependencyGraph(dependencies, options) | ||
|
||
if currentDir.lockFileExists: | ||
# If we already have a lock file, merge its data with the newly generated | ||
# one. | ||
# | ||
# IMPORTANT TODO: | ||
# To do this properly, an SMT solver is needed, but anyway, it seems that | ||
# currently Nimble does not check properly for `require` clauses | ||
# satisfaction between all packages, but just greedily picks the best | ||
# matching version of dependencies for the currently processed package. | ||
|
||
dependencyGraph = mergeLockedDependencies(pkgInfo, dependencyGraph, options) | ||
|
||
let (topologicalOrder, _) = topologicalSort(dependencyGraph) | ||
writeLockFile(dependencyGraph, topologicalOrder) | ||
writeLockFile(dependenciesGraph, toSeq(dependenciesGraph.keys()).sorted) |
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.
I modified this bit because it was based on the PackageInfo.requires
, but the requires is less useful than the dependency graph returned by installIteration
.
The returned dependency graph will be fully resolved (eg, in require there could be https://github.com/status-im/nim-unittest2.git
which will become unittest2
in the dependency graph)
I'm also not sure why the merge system was there? Instead this version will generate a predictable lockfile (since everything is sorted), so if one dependency change, only the lines of the dependency will be impacted.
let | ||
depsGraph = toSeq(pkgInfo.lockedDeps.pairs).map(pair => (pair[0], pair[1].dependencies)) | ||
(order, _) = topologicalSort(depsGraph.toOrderedTable()) | ||
developModeDeps = getDevelopDependencies(pkgInfo, options) |
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.
The current master version relies on json object key ordering to install locked deps in the right order, but json doesn't give this guarantee afaik
I added a second sort topological sort here, to be safe. See also comment on lock(
This PR switch nimble from a recursive dependency solver to an iterative one. This allows the algorithm to be a lot smarter, since it has a global view of dependency tree. Ex:
Example with already installed dependencies
Example with no dependency installed
As a reminder, current nimble output to install libp2p
The current iterative algorithm is fairly simple, but effective. We start by building the dependency graph:
stew required by: user (#head)
stew
, so we add it to stew constaintsstew required by: user (#head), chronos (any version)
)When we've finished building the dep graph, we'll install every missing package (after their deps).
This is still WiP. I originally based my work on 0.13.1, and when porting it to master a lot of things broke. Currently fixing remaining issues.
The major thing missing from this algorithm is what python calls "backtracking ".
eg, if two have:
nimble install A@>2.0 B@>2.0
will not work, because the latest version of A & B are not compatible. But we could find out that using B@3.0 would solve the issue.With the current nimble file format, that would imply downloading a lot of versions to find a compatible one, like python does, which is not ideal.
If a user stumble onto this, he can "manually" backtrack by asking for an older version of B manually.