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

Move most solver code to new module namespace #3381

Merged
merged 1 commit into from
Apr 27, 2016

Conversation

BardurArantsson
Copy link
Collaborator

@BardurArantsson BardurArantsson commented Apr 26, 2016


This should basically be good to go as the first step in moving all the solver bits to a separate namespace. There are a few types left in Client.Dependency.Types -- I'll submit a follow-up for those sometime during the week if we can get this PR merged.

I'm anxious to get this merged before 1.24.x if possible because a) it's generally pretty conflict-prone even though it's trivial code movement, and b) so that we don't spurious conflicts if we're going to cherry-pick bits from master into 1.24.x following 1.24.0. (EDIT: Nevermind that, I don't think it's really feasible unless loads of other stuff from master gets merged.)

I've elected to move some type definitions into smaller modules under Client.Solver.Types. This means:

  • The "turn off orphan warnings" pragma covers a lot less code.
  • Each of those modules has a lot less "noise" (unused imports, intermingling with unrelated types/instances, etc.)
  • Mostly no need to use explicit import lists at use sites.

@BardurArantsson
Copy link
Collaborator Author

/cc @23Skidoo @kosmikus @grayjay

@23Skidoo
Copy link
Member

23Skidoo commented Apr 26, 2016

I'm anxious to get this merged before 1.24.x if possible

Can you also do a PR against the 1.24 branch then?

@23Skidoo
Copy link
Member

In fact I've already asked @rthomas to cut a release, though he haven't responded yet.

Should I do the PR against 1.24 branch and then we cherry-pick to master, or...?

No need to close this one, just make a new one againt 1.24. Though at this stage I can't guarantee it'll make it into the release.

@BardurArantsson
Copy link
Collaborator Author

BardurArantsson commented Apr 26, 2016

Nevermind the 1.24.x bit, I don't think it's really feasible unless loads of other stuff from master gets merged. I wasn't aware of how much they'd diverged already.

@23Skidoo
Copy link
Member

OK, fine.

@BardurArantsson
Copy link
Collaborator Author

Obviously, I'd still like this to get merged to master as soon as feasible -- it's still conflict-prone and I really don't want to have to do this a third(?) time because of conflicts :).

@23Skidoo
Copy link
Member

If @kosmikus is OK with this, I can merge.

@BardurArantsson
Copy link
Collaborator Author

(Sorry for the noise:)

Just a little review note: Obviously there are no changes in any actual logic, it's all just about moving code around and fixing up imports.

@kosmikus
Copy link
Contributor

Yes, AFAICS, this is all ok. I'm not completely sure if Distribution.Solver.Types is the best prefix for all the modules now there, but they're equally non-specific to the "Client" than to the "Solver", so it's not making anything truly worse. I also haven't explicitly checked that every line of code is still there, but it certainly looks good.

I am somewhat worried about the interference with other ongoing developments. @23Skidoo, if we merge this, I think it's important that others (including, but by no means limited to @ezyang, @dcoutts, @grayjay, @hvr, @edsko) are fully aware that some modules now have switched / are about to switch location and can adapt to the new layout as soon as possible. It may also be good to include a full old-new conversion list somewhere where it can be easily referenced online.

Apart from the concern just mentioned, though, I agree with @BardurArantsson that it is better to merge this sooner rather than later, because on one side of the spectrum, maintaining PRs such as this is tedious, and on the other side of the spectrum, others may hold off their own PRs that affect many modules if they know that a larger module reorganization is imminent.

@23Skidoo
Copy link
Member

I think that the only open PR that touches the solver code is #3290/#3232.

@kosmikus
Copy link
Contributor

@23Skidoo Yes, but open PRs are not necessarily a reflection of all ongoing work. And this PR touches more than just the solver code. It moves all modules that the solver depends on into the Solver hierarchy, whether they are "true" solver modules or not. So it'll probably affect other PRs.

@grayjay
Copy link
Collaborator

grayjay commented Apr 27, 2016

#2917 also touches the solver. It will be easier to rebase if this is merged sooner, since I just rebased it today.

@23Skidoo
Copy link
Member

OK, I think the consensus is that it's better to merge this earlier than later, so I'm merging.

@23Skidoo 23Skidoo merged commit d1d503a into haskell:master Apr 27, 2016
@23Skidoo
Copy link
Member

Merged, thanks!

@BardurArantsson
Copy link
Collaborator Author

Great, thank you! I'll get started on the remaining minor bits & pieces.

If there's enough interest, I can try to assemble a little list of "what moved where", but it should mostly hopefully be pretty obvious.

@BardurArantsson BardurArantsson deleted the reorg-solver-modules branch April 27, 2016 14:53
@BardurArantsson
Copy link
Collaborator Author

Ha, just noticed that Cabal now has exactly 7000 commits! :)

@kosmikus
Copy link
Contributor

@BardurArantsson Thank you for your patience, and sorry that this had to go through multiple iterations to make progress.

@BardurArantsson
Copy link
Collaborator Author

@kosmikus NP. (Sure, it's been a bit frustrating, but such is the nature of "Big Bang" changes.)

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.

4 participants