Skip to content
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

Merged
merged 5 commits into from
Jan 6, 2018

Conversation

carlobaldassi
Copy link
Member

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 the add_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 the Graph constructor in "GraphType.jl".

Once these issues are sorted out, I'll do a final cleanup.

@carlobaldassi
Copy link
Member Author

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:

  1. I could restore the logic such that everything works exactly as before this PR. I had assumed that the way things were done (filling in dictionaries one version at a time) was derived from the need to create Pkg2-style data structures. But maybe it wasn't, and that behavior was precisely intended? I'm referring to these lines in current master: https://github.com/JuliaLang/Pkg3.jl/blob/a554607c939ab73d89c3e7c6a3dd08fd870145e8/src/Operations.jl#L127-L141 and the fact that different information may be present in different paths.

  2. We could move on with the way things are done in this PR, and tweak the precise behavior later (again, it only affects the case of conflicts). The previous lines are essentially the same except that they work at the level of VersionRanges rather than VersioinNumbers (https://github.com/JuliaLang/Pkg3.jl/blob/22d00776663edfdf7def09dd468eb439f604feca/src/Operations.jl#L152-L183) and then the association between the names (as read from "compatibility.toml") and UUIDs (as read from "dependencies.toml") is done in the Graph constructor (https://github.com/JuliaLang/Pkg3.jl/blob/22d00776663edfdf7def09dd468eb439f604feca/src/GraphType.jl#L185-L217).

The second way is somewhat more "natural", since that's how the data comes in, and the Graph object actually only stores compatibility masks internally (such that actually it wouldn't even need to know about the direction of the dependency, and it could manage just fine with compatibility constraints between two packages that don't even depend on each other at all, in principle).

Thoughts?

@KristofferC
Copy link
Member

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?

@carlobaldassi
Copy link
Member Author

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.

@StefanKarpinski
Copy link
Member

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants