-
Notifications
You must be signed in to change notification settings - Fork 701
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
RFC/Solver split (v2) #3222
RFC/Solver split (v2) #3222
Conversation
/cc @dcoutts |
@BardurArantsson while enforcing proper API boundaries is a desirable thing, I'm still wondering about the packaging aspect. So you seem to imply that we're gonna have to bump |
@hvr Yes, the version numbers would all be in sync. (At least until some other scheme is decided upon.) This is not really aimed at making the solver an "independent entity in its own right" (i.e. separating it from Cabal/cabal-install). Long term, that might become a thing to strive for, but I'm trying to be as practical about it as possible and taking small steps :). EDIT: Oh, and yes, it's definitely going to remain in the main "cabal" git repo. |
Ok that means we'd be limited in what we can change inside the say e.g. 1.24 branch. As it could easily be that we'd want to make an incompatible change requiring a major version bump to the cabal-solver component for But once we separate out I just want to make sure everyone's aware that there's also a cost involved splitting off Otoh, we could split PS: there was the idea to implement internal utility-libraries for cabal (iirc by @ezyang) then the cabal-solver could be just such an internal non-installed uility-library in |
Sure, but I think going with "NO GUARANTEES ABOUT ANYTHING" in the documentation would be acceptable as a first step.
Certainly... but it should also be noted that there's a cost to not doing it. (Namely the pain for 3rd parties wanting to integrate with Cabal/cabal-install.) This is clearly something that the wider community has been wanting for for quite some time. (I won't bother looking up GH issues about this -- as I'm sure you're aware there are at least... "several". :) )
I'm not sure what you mean by "embed its modules into cabal-install's .cabal file"...? Would it be possible to still rely on the compiler to mechanically reject any API boundary violations if we don't create an actual .cabal package? |
Can you explain why the version numbers need to stay exactly in sync? It seems to me that as long as we follow the PVP there should be no problems. |
@ttuegel Oh, it's really just me choosing the path of least resistance 😄 . My thinking was that initially we'll just keep EDIT: Just to be absolutely explicit about it: Yes, I think PVP would work just fine. Though IME maintaining packages with different versions in one repo tends to get a bit weird over time. |
Git branching/merging gets in the way of unsynced versioning. By having everything (i.e. multiple packages) in the same Git repo the Git history of all components gets entangled with each other, and you effectively are bound to develop them synchronously as a unit, rather than as individual packages. The issues start with version tags that by construction cover all components checked into Git. A symptom of that is that you can't use idiomatic
containing both Also While this is at best confusing, it gets really troublesome once you need to maintain Git branches and merge between them, as then you have to handle conflicts for all components involved rather than the single one you're actually interested in. If you don't synchronise versions, you'd need different Git branches for each component, and do the same workaround you're already doing for Git tags: you'd need a You're basically emulating what SVN did, except that SVN as bad as it was had actually better support for that kind of workflow (e.g. TLDR: Having multiple "versioning-threads" in the same Git repo is a very bad idea; either have all components share one versioning, or move each component following its own versioning-thread into its own Git repo. |
@hvr Thank you for that rant -- saved me from having to do one! 😄 TL;DR is: Choose the path of least resistance, i.e. lock version numbers. If it turns out we need something else in the future, then so be it -- let's figure it out at that time instead of fretting endlessly about it now. |
👍 I haven't had a chance to look at the whole PR, but I have one comment. Would it make sense to move |
I'd expect #3022 to provide just that sort of isolation, as those "convenience libraries" are installed just as if they were normal libraries, except they can't be used by anyone else. |
@grayjay Good points; I suspect we might want to tweak "what gets exported" (and conversely "what gets hidden as an impl. detail") as we go forward. |
@grayjay (et al): So, I've had a look at
However, I think we can try something slightly different than my original plan here -- and then try to move TopDown later[1]. What if we change my plan as follows:
WDYT? [1] FWIW, I still think it should just be removed. I can see that the current code already issues a deprecation warning to the user when using it, and we've had a release... (Unless there's some sort of two-releases-post-deprecation-before-removal policy I'm unaware of...). It seems to me to be a huge waste of time to expend any effort on it. EDIT: Hm, if I'm reading git-blame correctly, the deprecation warning was added 2015-04-07 so it hasn't been in for a full release cycle, I don't think. However, it's in 1.24.x and so it should be OK to remove in 1.26.x (which is what master is targeting) as far as I can tell?!? |
hasktags + |
@23Skidoo Thanks for the tip. I had ghc-mod working at one point, but it just got so tedious keeping it working that I gave up. I guess I should look into it again and/or try hasktags -- finding dependencies by moving files around is definitely very inefficient :). |
@BardurArantsson Would we release Phase 1, or is it just a separate PR? Managing four solver packages sounds difficult. Could we combine them into |
@grayjay I suppose some of the annoyance would be alleviated by the upcoming(?) support for having multiple-package builds using a single cabal invocation. (I tend to just use That said, I suppose we wouldn't have to split the solvers from each other -- it might be overkill. But of course then we don't necessarily get the cleanest API serparation possible. Another option might be to just do the original "phase 1" of this proposal and then gradually try to move TopDown + PS: Have I mentioned that I'd love to just remove the TopDown solver? :) |
Another reason that I suggested combining the solvers was to make it easier to remove the topdown solver later. It would be a smaller change to the APIs, especially if we wanted to combine the three remaining solver packages. |
Last time I looked (which admittedly was quite a while ago) moving the TopDown solver would require moving a lot of extra code out -- it's pretty intertwined with the rest of I really really want to avoid having to move loads of code out only to delete it a short while later. |
Note to self: Perhaps use #3022 for the first step? |
The downside is I haven't debugged nix-local-build plus convenience libraries yet, so they don't quite work together yet, |
OK, good to know. Not sure when I'll pick this up again, so I guess we'll see how it works at that point :). |
That makes sense. Then maybe we should remove the topdown solver in Phase 2. |
I'd love to. What does everyone else think? (I seem to recall @kosmikus wanting to perhaps keep it around for interactive testing? Is that still relevant now that there are QuickCheck solver tests?) |
I'm in favour of removing code, if @kosmikus doesn't object. |
I think we need to be clear on what the costs and benefits are. There are clear maintenance costs. Are we clear on what the benefits are? What projects want to use the solver separately? Would they help shoulder the maintenance costs? |
What are those costs as you see them?
One benefit I forgot to mention is that it'll "keep us honest" in that it becomes very hard to accidentally introduce accidental dependencies from the solver to cabal-install (like the spurious "package location" bit that I removed #3210 which wasn't actually being used by the solver).
I'm not seeing the additional maintenance costs here. I mean we're just talking about introducing a new .cabal file and moving some modules around. |
Ok, I think we're at a bit of an impassé on the TopDown solver, so I've created #3364 so we can hash that out separately and without reference to any potential solver split. |
I've been talking to @dcoutts and @hvr, and they both seem mildly skeptical as well. The advantages and use case seem relatively vague. On the other hand, I'm slowly coming around on this. While I still think that even "just moving a few modules and creating a new cabal file" increases the maintenance burden, I also agree that if things stay within the same repository, it should not be too bad. So if everyone else wants this, I'm willing to give it a try, even though I don't really believe in it. I really want all the tests in the solver package itself. We should look into what we need to do in order to make this possible. As for the modules the solver currently depends on: this set is a bit arbitrary, and might change at any point in time (might get larger / smaller). Do we then plan to move modules back and forth, or will we just move more modules over to solver? Or should we already move more modules? Should there be a criterion that is different from "currently being depended on by the solver"? @hvr and others raise good points regarding versioning: The library should follow the PVP, but it should also remain easy to associate the library versions with cabal-install versions, at least in the beginning. We may want to be able to make changes to the solver library that require a major version bump according to the PVP without having to bump cabal-install as well. So we should agree on a versioning scheme that makes this possible. Some options I can see would be:
where x gives us room for major version bumps on the library. We should agree on such a scheme before we make the split, because it may be difficult to change it later. |
Fwiw, if we keep everything in a single Git repo we need to make sure that commits are structured in a way that's easy to cherry-pick between branches (since |
I'll try to look into it next week. I think we could be even more cautious in how we proceed. It might make sense to start by not actually creating a new .cabal file just yet, but to instead just create a new module namespace for all the solver-needed bits so that we can at least get everything organized. (Of course this won't give us the advantages of real isolation of dependencies, but it should at least make it more obvious what we can split out and what the test dependencies really are.) Doing it that way will also minimize disruption. I think I'll try to see how much work is needed to do such a reorganization as a first step. I think this should at the very least be pretty uncontroversial :). Re: Versioning: I'm not sure we actually need to worry too much about it at this point: We can start by just having I think this is a case where "perfect is the enemy of good" -- it's very hard to predict exactly what modules should eventually by separated out, so I'd be leaning towards just starting with the absolutely minimal bit of functionality (which would basically be just enough for the tests to run) and then seeing where that takes us. |
Sounds good to me.
This is at least uncontroversial to me. I don't see any need for the solver packages (and quite a few others) to have a
No, sorry, but I think this is exactly the wrong thing to do. If we follow the PVP for a library, then it dictates when we have to bump what component, and this will then force the
I think I can in principle live with that, because I don't currently know any other good rule to decide which modules, but @dcoutts may have some ideas. |
The |
@kosmikus Fair enough points. I'll try to get started on the module reorganization sometime next week. |
Ok, so here's the first PR for the solver reorganization: #3381 That basically encompasses most of the changes from this PR (a little cleaned up, see the PR) except the separate .cabal file. I'll leave this open for now. |
Sigh... Travis says FTP/AMP failures. I'll fix those in a bit. |
@BardurArantsson btw, please try to avoid CPP for handling those. In most cases it's quite easy to |
default-language: Haskell2010 | ||
-- starting with GHC 7.0, rely on {-# LANGUAGE CPP #-} instead | ||
if !impl(ghc >= 7.0) | ||
default-extensions: CPP |
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.
since we don't support pre-H2010 anyway, we certainly don't want to enable CPP by default either
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.
there's a if impl(ghc >= 8.0)
-conditional in cabal-install.cabal
and Cabal.cabal
enabling forward-compat warnings. Please add that here as well.
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 believe I just copied the cabal-install.cabal
file as it looked at the time of starting on this PR. Obviously, I'll do the same again if/when I rework this.
(If there's stuff that can/should be eliminated from the file then that'll be strictly follow-up.)
@hvr: Re: AMP/FTP: I think I posted that comment in the wrong PR, but regardless: I'm doing absolutely no changes to any functionality or that could have any conceivable impact on functionality. That's on purpose. (I've done enough reworks of this set of changes that I'm not going to get creative with anything. Things must settle down before any sort of cleanup happens.) |
@kosmikus wrote:
and
I'm not sure if these two goals are reconcilable even in principle. Allow me to step back a little and ask: Why should cabal-install's version number be tied at all to cabal-solver's version number? After all cabal-install doesn't re-export anything related to the solver (except very indirectly though its actual dep. solutions). In the meantime, it specifies exactly which version of cabal-solver it uses in its .cabal file, so it should be easy to see which cabal-solver version is being used in any given version of cabal-install. Now, in practice I'd expect basically every solver change to be accompanied by a change in cabal-install, if nothing else then just to bump the cabal-solver dependency version number to actually use New Feature(TM) or get The Bugfix(TM). So that would mandate a simultaneous bump in cabal-install's version number. So effectively if we start at the same version number, they can just follow each other. If EDIT: I don't think the PVP forbids version number bumps when no changes have occured, right? @hvr: I certainly agree that there's a thorny issue here wrt. keeping multiple different versions of "different-but-related" projects in the same repository. (Which is why I'm mostly arguing for an 'enforce the same version numbers' type thing.). Incidentally, how are we handling this wrt. Cabal and cabal-install at the moment? I'd expect cabal-solver -- at least for the foreseeable future -- to be about as intimately tied to Cabal as cabal-install is currently. So why not do exactly the same as what we're doing wrt. the Cabal and cabal-install dependency? |
Stack developers seem to be interested in this: commercialhaskell/stack#1615. |
Given that "new-build" basically addresses all my original reasons for wanting to split out the solver, I'm going to close this. It'd still be nice to split the solver modules out into a "package local" library (or whatever it's called) just to avoid accidentally introducing new dependencies from the solver into |
I couldn't find an issue for this, so I'll open a new one. I don't want to forget about it. EDIT: #3781 |
@BardurArantsson this still needs to happen. Is there a reason this couldn't get caught up/refreshed for current /cc @ttuegel |
@bitemyapp Most of the actual "really important" changes in here have been merged (separately) -- those were mostly about separating the logical "solver" bits from "cabal-install". The rest of this PR was mostly just about creating a new .cabal file, etc. etc., and so was (ultimately) not that interesting. (EDIT: Well, obviously, it'd be interesting at some point, but the first step was to separate the "solver" bits from the "everything else" bits.)
I'm not sure I understand what you mean here. Can you clarify? |
@BardurArantsson I had misunderstood, so I'll ask a different question - how do you avoid introducing dependencies from the solver into For my part, I'd like for Stack to be able to run the solver in-process and not shell out, as that's the final dependency on |
On the contrary, that was the most interesting part! There are at least three ways I can think of to use the solver externally:
The barrier to doing these things is lowered significantly if we have direct access to the solver. |
@bitemyapp Oh, sure, "dependencies from X to into Y" (in my parlance) would mean that X has a dependency on Y. So, the point would be that cabal-solver would occupy a "middle ground", so we'd have a dependency graph like "cabal-install" depends-on "cabal-solver" (among other things) depends-on "Cabal". Does that description make more sense? |
@BardurArantsson I understand what you just described but not what the original words were trying to say, but we can skip that for now. I concur with @ttuegel that the rest of this PR needs to happen. I'm unclear how following PVP in cabal packages would be controversial or objectionable. |
@ttuegel Oh, I knew I'd get called out for that! :) I meant uninteresting in the strictly "technical" sense... it's mostly just make-work. The "technically" more interesting bits were about introducing even more parametric polymorphism to break the dependency from various datatypes to decidedly EDIT: Grammerz is hard |
@bitemyapp See #3781 :) |
@BardurArantsson it sounds like I should continue with what I was doing then. Cheers. |
@bitemyapp ... not sure what you were already doing, but the answer is probably, yes, keep calm and carry on. At least until something drastic happens :). EDIT: Should say: I'm probably not the person going to be doing the drastic-changing... "new-build" works well enough for me, so... EDIT#2: As for versioning, I thought my suggestion was eminently sensible, but then I would think that, wouldn't I? |
OK, so this is an RFC for the solver split that I've been working on on-and-off for a while now.
This code is mostly just meant for review -- if we decide this is the way to go, I'll rework it into smaller chunks (such as moving type definitions to their own modules in a separate commit, etc.) such that the final "move all the files" commit will be as small as possible.
The executive summary of what I'm proposing and the diff:
cabal-solver
library which lies "between"Cabal
andcabal-install
in the dependency chain. That is: No changes toCabal
,cabal-solver
depends onCabal
, andcabal-install
depends onCabal
andcabal-solver
.cabal-solver
. The TopDown solver seems too entangled in thecabal-install
code base to be able to sensibly move it. (Considering that it's probably on its way out in some near-future point, it's probably not worth bothering.)PackageIndex
,PackageUtils
,PkgConfigDb
andComponentDeps
. A few type definitions fromDistribution.Client.Types
andDistribution.Client.Dependency.Types
are also moved since they're required for the solver.Distribution.Client.Type
for convenience and to incur fewer "incidental" changes which could obfuscate the diff. If we choose my approach to the solver-split, I think we should probably avoid this re-exporting and just add relevant imports where necessary.cabal-install
functionality present. I don't think this is a show-stopper since the tests can happily live incabal-install
since we can just export needed functionality via an exposedInternal
module.setup-dev.sh
), so I expect travis et al to fail horribly. Obviously I'll get these things fixed if we decide to go ahead.cabal-solver
modules stating that it's not held to the same strict backward-compatibility standard asCabal
. (We'll need to figure something out wrt. stability guarantees, but that's really something that we can address if this gets merged.)My main questions for reviewers:
cabal-solver
make sense for tools that might want to use the solver as a separate entity? If not, what's missing? Are we exposing too much implementation detail? I think it's still a good idea to have a split at this boundary regardless of whether this is immediately useful for 3rd parties[1].I think that's it. Comments welcome!
[1] The main idea here being that moving to a separate
cabal-solver
package can help enforce proper API boundaries so that the solver doesn't accidentally become hopelessly entangled with implementation details ofcabal-install
over time. I view this as simply good software engineering practice, but that's just my opinion. If 3rd party projects need a larger API or more functionality, then we can introduce a further layer betweencabal-solver
andcabal-install
which can export what they (andcabal-install
) needs.EDIT: Formatting.