-
-
Notifications
You must be signed in to change notification settings - Fork 269
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
Complete refactor of the dependency graph and related functions #81
Conversation
Ok I think I have stress-tested this quite a lot. The core stuff is ready, I believe. The only remaining issue is what to do exactly about potential conflicts in the data read from the inputs. @StefanKarpinski, I know these are busy times, but I need some quick advice here:
The second way is somewhat more "natural", since that's how the data comes in, and the Thoughts? |
I think we should carry on with the way you have it here and tweak it later in case we need to. Right now, I think it is important to focus on getting things in place and not getting stuck at details (unless these details fundamentally change design choices, but I don't think this is the case here). Could we merge this @StefanKarpinski so that the rest of Carlo's work based on this can continue? |
Thanks @KristofferC. In case of green light, I think I'll better squash this (and the other related PRs) since the history is rather messy in these cases. |
Yes, let's go ahead and merge all of these improvements. They're fairly decoupled from the rest of the package manager, so it should be good to go whenever @carlobaldassi feels that it is. |
The new graph is basically the old maxsum graph plus the old interface plus the uuid_to_name dict. But it's built directly from the TOML data, and manipulated directly.
Prevents an obscure TypeAssertion error later in the process.
22d0077
to
776439b
Compare
Summary: in Pkg2, information from the file system was parsed into a dependency graph, which then got variously manipulated, and finally translated into a constraint satisfaction graph to be passed to the solver. This change removes the previous intermediate representation and goes directly from the combination of "dependencies.toml" and "compatibility.toml" to the constraint satisfaction representation.
Thus, it uses a single graph type, with its own file and related manipulation functions. The functions reproduce (actually, extend and improve) the various forms of simplification and pruning of the previous code. But their logic is actually much, much cleaner, and also more general (the implementation of the pruning itself is the ugliest part of the code, since the internal representation is optimized for the solver and it's not as "dynamic" as using Dicts). In general it's quite a big improvement IMO.
Ah and it also implements the logic for fixed packages, including the fixed julia version, which was disabled/missing. This means again that some coordination will be required with #56, but either passing the fixed packages in the
Graph
constructor or calling theadd_fixed!
function should be enough to deal with that, everything else is done automatically.(By the way, the graph representation is as follows: each package is a node that can have a variable number of states; each state represents one possible version, plus there is the "uninstalled" state; constraints such as requirements and fixed packages are implemented as boolean 1-dimensional masks on the allowed states; dependency relations between two packages are implemented as boolean 2-dimensional masks on allowed combinations of their states. That's all.)
There are some things related to the operations code that I need to ask/check though, related to the possibility of contradicting information being obtained for the same package from two different paths (e.g. if the "dependencies.toml" file contains different version ranges, or different information for the same version ranges etc.). The code in this PR may behave a little differently from the previous one in case of conflicts. Is there some place where I can read about the intended logic in cases like these? Or should I just ask specific questions here? In any case, the parts of the code I'm referring to are the
deps_graph
function in "Operations.jl", and theGraph
constructor in "GraphType.jl".Once these issues are sorted out, I'll do a final cleanup.