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

Import hygiene and miscellaneous fixes. #3537

Merged
merged 1 commit into from
Jul 11, 2016

Conversation

ezyang
Copy link
Contributor

@ezyang ezyang commented Jul 11, 2016

Signed-off-by: Edward Z. Yang [email protected]

@phadej
Copy link
Collaborator

phadej commented Jul 11, 2016

@ezyang Are there reasons

  • for a hurry to get everything into master (you got graph stuff in, then reverted, then got it again in)
  • and for not using GitHub merge?

E.g. #3476 the #3476 (comment) comment is not addressed. Please, if you rush commit in, address obvious issues yourself.

I don't think there is written process, but IMHO it worked quite well when @23Skidoo works as a gatekeeper, i.e. merges stuff (thanks for that!)


Apart from that, this PR LGTM.

@ezyang
Copy link
Contributor Author

ezyang commented Jul 11, 2016

@phadej

  • The revert was because I botched the push (I accidentally also grabbed some local changes for a non-ready patch which broke the build.)
  • I can start using GitHub merge (don't like it but for each project its own, I suppose.)
  • Re Generalise 'AllowNewer'-types' names #3476 that was an oversight, I've pushed the updated comment.
  • Yes, I don't know if we expect @23Skidoo to merge all pulls to master (bus factor?), or if developers judge that their PR is stabilized they can merge themselves. Every bit helps—we're not very good at shepherding PRs to completion.

Let me say a little more about rushing. There is an inherent tension in all software projects between stability and forward progress. And so the question I would ask you is this: should Cabal value stability or forward progress more?

If we value forward progress, we need to also value refactoring. And the reality of refactoring is that these are large, non-local changes, which take disproportionately more time to code review, and are most harmed by a long lead-time into Cabal. Take #3525. Not only is this going to take a lot of time to review, I'm pretty sure that it conflicts with a refactor that @dcoutts is working on to fix exception handling in new-build. So it is really harmful to keep refactor PRs in the queue for too long. We really do need to rush!

@phadej
Copy link
Collaborator

phadej commented Jul 11, 2016

This is precisely why we use Github merge: Travis the CI verifies that we don't have something which breaks the build in the commit / branch you push / merge. It's not perfect, but still better than (even rare) accidental breakages.

I'm also quite sure that people are lazy to verify with every GHC we tend to support, that boring job is left for CI on purpose.

@ezyang
Copy link
Contributor Author

ezyang commented Jul 11, 2016

(FYI: I edited my above comment, in case you didn't see.)

@23Skidoo
Copy link
Member

I agree with @ezyang's points and appreciate his efforts. In general, I think we should move in the direction of less bureaucracy, which among other things means trusting developers with a commit bit to merge open PRs (after all, that's what the commit bit is for).

I also think that non-GitHub merges are fine, since our build bots will catch any eventual errors anyway, in which case the offending commits can be reverted.

@23Skidoo 23Skidoo merged commit 9a4bfc3 into haskell:master Jul 11, 2016
@23Skidoo
Copy link
Member

Merged, thanks!

@phadej
Copy link
Collaborator

phadej commented Jul 11, 2016

And so the question I would ask you is this: should Cabal value stability or forward progress more?

This is a hard question. I do value forward progress, do value your efforts, and I have bitten myself by long-lead time and numerous rebasing due conflicts (not in Cabal though). But I do value stability too.

To return to #3525. I do agree that it's welcomed change. But I'm a bit confused. There is an issue to introduce Cabal.Distribution.Compat.Graph #3521, referencing #3525. Yet, I cannot find any discussion about either change. I think that I follow all media where discussion could have happen, but apparently I don't. Hopefully there was some discussion somewhere, but I totally missed it.

About large refactorings. I have no silver-bullet for this. Try to split them into smaller PRs (as you did with Graph), squash / fixup history so there aren't numerous fix small thing -commits: you'll help both the reviewer and yourself, if you need to rebase. For example I'd try to split #3525, into a separate PR for rewrite of PackageIndex, even that would require redundant fixing of cabal-install in between.

I do dislike that we step over CI, and push to master directly. I greatly appreciate, that if I have a bit a time, and I want use it to make Cabal better, I can pull newest master, and I have the repository in good shape to start with. Travis builds don't take that long, you cannot wait for them.

If I can propose a rule: let someone else than the author merge the PR.

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