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

Remove top-down resolver #3598

Merged
merged 1 commit into from Jul 23, 2016

Conversation

Projects
None yet
5 participants
@BardurArantsson
Copy link
Collaborator

BardurArantsson commented Jul 22, 2016

Removing 'topdown' as a resolver choice means that 'choose' is also
obsolete and so it is removed too.

@BardurArantsson BardurArantsson force-pushed the BardurArantsson:remove-topdown-solver-v2 branch from bad2eb9 to e5a3fb1 Jul 22, 2016

@@ -16,6 +16,7 @@
(#3429).
* The bootstrap script now works correctly when run from a Git
clone (#3439).
* Removed the top-down solver (#3598).

This comment has been minimized.

@hvr

hvr Jul 22, 2016

Member

wrong kind of whitespace :-)

This comment has been minimized.

@BardurArantsson

BardurArantsson Jul 22, 2016

Collaborator

Ah, yes, thank you :)

@BardurArantsson BardurArantsson force-pushed the BardurArantsson:remove-topdown-solver-v2 branch 2 times, most recently from 228f7de to f0bbcf1 Jul 22, 2016

@ezyang

This comment has been minimized.

Copy link
Contributor

ezyang commented Jul 22, 2016

Yay, so much deleted code!

But are you sure you want to remove the solver parameter? @phadej wants to add external solver support; ostensibly that would use that plumbing. I guess one could put it back when the time comes.

optionSolver :: (flags -> Flag PreSolver)
-> (Flag PreSolver -> flags -> flags)
-> OptionField flags
optionSolver get set =

This comment has been minimized.

@ezyang

ezyang Jul 22, 2016

Contributor

Ugh I wish we had some library function that let us turn this into a "this was removed flag, modular solver only" to let users who try to use this flag know what to do.

This comment has been minimized.

@BardurArantsson

BardurArantsson Jul 22, 2016

Collaborator

Yeah, I'd have really liked to keep reading the "solver" option and just emit a warning/error saying "you might have wanted X, but I'm going to do Y", but: In the default configuration the "solver: XXX" line is commented, so it's not going to be a problem for the vast majority of cabal-install users. If they try to change "solver" then, of course, they'll get an error message, but I think that's basically as it should be.

Of course there might be some people out there who are actually using "solver: topdown" in their configurations, but they should have had plenty of time to speak up since it's emitted a deprecation warning for quite some time.

@BardurArantsson BardurArantsson force-pushed the BardurArantsson:remove-topdown-solver-v2 branch from f0bbcf1 to 1a2eef5 Jul 22, 2016

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Jul 22, 2016

But are you sure you want to remove the solver parameter? @phadej wants to add external solver support; ostensibly that would use that plumbing. I guess one could put it back when the time comes.

Yeah, I was sort of on the fence about that[1], but as you say it can always be brought back and if we're going to be realistic I think the 'external solver' is quite far off at this point[2], so in the end I decided that it's probably better to avoid the maintenance burden of the "redundant" code. (Basically: Applying the YAGNI principle.)

[1] This is, in fact, the reason that I have separate commits for these things. That'll make it even easier to bring back "solver choice" if it's ever going to be needed again.

[2] In practice... AFAICT it's going to be a lot of work to standardize on a format, etc. (I'm actually not really sold on the idea of an external solver, but whatever.)

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Jul 22, 2016

I'll merge the first part of this tomorrow (that bit should be uncontroversial at this point), but I'll hold off for a few days on the follow-ups -- just to let people chime in.

@phadej : Do you think this will cause trouble for you or do you agree with my assessment on the external solver support?

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Jul 22, 2016

+67 −2,060

👍

@kosmikus

This comment has been minimized.

Copy link
Contributor

kosmikus commented Jul 22, 2016

I'm in favour of keeping the solver parameter. I have been experimenting with other solvers in the past and think it's likely that I or others will do so in the future. This is much easier if the parameter is there, and I don't think there's a huge maintenance burden attached to it.

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Jul 23, 2016

OK.

For clarification: Do you want to retain the configuration bits per se? With the changes I've posted it's still pretty easy to experiment by changing the a single line in resolveDependencies -- change modularResolver to myResolver. That's the only place it's mentioned.

I suppose we could also change chooseSolver to just return a IO (DependencyResolver ...) instead -- that'll mean that you can easily use IO when initializing the solver and all the "solver" parameter plumbing will take care of the rest.

WDYT?

@BardurArantsson BardurArantsson force-pushed the BardurArantsson:remove-topdown-solver-v2 branch from 1a2eef5 to 5d475b0 Jul 23, 2016

Remove top-down resolver
Removing 'topdown' as a resolver choice means that 'choose' is also
obsolete and so it is removed too.

@BardurArantsson BardurArantsson force-pushed the BardurArantsson:remove-topdown-solver-v2 branch from 5d475b0 to f357f77 Jul 23, 2016

@BardurArantsson BardurArantsson merged commit f357f77 into haskell:master Jul 23, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@BardurArantsson BardurArantsson deleted the BardurArantsson:remove-topdown-solver-v2 branch Jul 23, 2016

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