Skip to content

[RFC] Split modular solver into separate package #2928

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

Closed

Conversation

BardurArantsson
Copy link
Collaborator

This is just an RFC, it's not meant for merging.

Notes:

  • There are a few minor XXX markers that will be fixed if the approach is validated by others.
  • This is based on top of PR Use explicit export lists for modular solver #2924.
  • It's not really possible to move the Modular Solver "unit tests" since they don't actually seem to be unit tests in the traditional sense and seem to be relying on quite a few bits of cabal-install "outside" the modular solver itself.
  • I gave up on moving TopDown after all -- it's not that it's impossible, but it would be quite a lot of make-work having to make type definitions/signatures parametric[1]. If it's deemed absolutely necessary to getting this accepted, I'd be willing to do it, but I'm hoping that the deprecation (and future removal) of the TopDown solver will mean that it won't be necessary.
  • There's a small question in PackageUtils.hs which is ideally like answered by someone else. (Search for XXX in that file.)
  • travis build not updated, nor bootstrap scripts, etc. Only setup-dev.sh has been updated, so please use that if you want to try building for yourself.

I think that's it for notes.

[1] As with the modular solver it revolves around the use of PackageLocation. I originally moved PackageLocation into the cabal-solver package, but gradually migrated it back to cabal-install by making type signatures in cabal-solver more parametric.

Using explicit export lists allows for trivial detection of dead code,
and this commit also removes quite a bit of said dead code.
@BardurArantsson
Copy link
Collaborator Author

Oh, yes: This represents a much more conservative approach than #2768, obviously. I figure this is a reasonable first step towards figuring out which bits (if any) should be integrated into Cabal, which bits (if any) could/should be moved to an entirely separate package (that doesn't depend on Cabal), and which bits should remain in the "realm" of the Cabal/cabal-install project. My hunch would be that most projects that are interested in the solver are interested in depending on Cabal too, so we'll see, I guess...

I do want to land this + #2924 as soon as possible to avoid further conflicts with @kosmikus 's work, so review and comments would be much appreciated -- I don't want this to linger too long in the "patch queue" and bitrot beyond repair. :)

@grayjay
Copy link
Collaborator

grayjay commented Nov 17, 2015

I'm concerned that this pull request has major conflicts with #2917. It also conflicts with #2907, #2732, and #2731. How about we handle the open pull requests first and then all agree to stop working on the solver until after the refactoring?

@BardurArantsson
Copy link
Collaborator Author

Hopefully the conflicts shouldn't actually be too bad -- this really is mostly just moving stuff around, but I'm OK with that, I guess as long as it doesn't mean that this has to wait for several weeks (or something like that).

However, I'd really like #2924 to get in first, if possible. Have you tried to see if that one conflicts with your stuff?

@grayjay
Copy link
Collaborator

grayjay commented Nov 17, 2015

@BardurArantsson Git compared the moved files properly, but there were large blocks of conflicting code. Merging #2924 doesn't look too difficult. There are large conflicts in Explore.hs and Log.hs, but I think git just misaligned the code, and it would be easy to manually merge those two files.

@BardurArantsson
Copy link
Collaborator Author

Yeah, for this PR, I've sprinkled the "loc" type parameter all over the place in the "extracted" solver code. Essentially PackageLocation has become a type parameter to PackageSource and (transitively) a few other types. It's pretty mechanical, but still requires a bit of care to make sure the types line up. If you have a diff viewer which can show only the bit of each line that changed, it should all be pretty simple. No logic whatsoever has changed, only type signatures.

As I say, I'm OK with redoing it -- it's just a bit annoying.

As for #2924 : Yup, no changes to any logic whatsoever. An easy way to resolve the conflicts would be to just keep all your changes except grabbing the export lists (+ perhaps any additions you've made) from my branch and then deleting any code which GHC says is dead.

Still, I'm not sure who is going to review and how we can manage this in an orderly fashion... maybe ping the mailing list?

@BardurArantsson
Copy link
Collaborator Author

I'll just close this in favor of #2768 which I'll try to follow up on in the next few weeks. No need to spam the PR list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants