[GHC DevOps Group] State of CI

Fri Jun 8 10:49:25 UTC 2018

With all due respect Manuel I'm afraid I continue to disagree :)

I don't think anyone is claiming that code review is documentation, or
should replace documentation. It's *discussion*, in the same sense as
mailing list discussion. We keep mailing list archives because they provide
useful context in the future and avoid us having to rediscover rationale,
and we shouldn't throw away code reviews for the same reason.  Moreover,
the way that code review discussions are attached to actual code which
lives in the repository makes them arguably even more useful and accessible.

I think the examples below demonstrate that there's valuable content here,
but not just for the people involved in that particular code review. It's a
shared codebase after all, the next person to work on this code might come
along ask "why did we end up with this design?".  If the available
docs/Notes don't tell the whole story, I can point to the code review
discussion and we don't have to recreate the whole thing again. You could
argue that the docs/Notes *should* tell the whole story - this point came
up before - in practice they don't and the discussion is useful context.

Furthermore, code review discussion is really conveniently accessible. Want
to see the discussion that led up to a particular commit? The link is right
there in the commit log!

Finally, the data in Phabricator is no more locked-in than what we have on
Trac: we have the DB and the open-source code that manipulates it. I don't
think code-review data on github is accessible right now (at least I
couldn't find a way to download it). Maybe it will be accessible in the
future, but I think your argument is that we don't care because code
reviews are ephemeral, which I don't agree with.

Cheers
Simon

On 7 June 2018 at 17:37, Manuel M T Chakravarty <manuel.chakravarty at tweag.io
> wrote:

> Sorry if I didn’t clearly state my second point. I do not doubt that these
> reviews are useful to you (or other people involved in the actual code
> reviews). I am saying that they are much less accessible than a discussion
> on Trac or, better even, documentation on the wiki for everybody else.
>
> They are *bad* documentation as (1) they lock us into a review tool with
> no standardised data format and (2) they serve a few rather than many.
>
> Cheers,
> Manuel
>
>
> Am 07.06.2018 um 16:22 schrieb Simon Marlow <marlowsd at gmail.com>:
>
> The first point I think is reasonable, being independent of any one code
> review tool would be an advantage. However, I'm prepared to sacrifice that,
> because I think the conclusion it leads to is suboptimal - that code review
> discussion is ephemeral, and we cannot rely on having access to it after
> the code is committed.
>
> To make this concrete, I picked out some examples of diffs I've been
> involved in recently. I'm sure there are plenty more examples like this:
>
> https://phabricator.haskell.org/D42 (click on "Show Older Comments" to
> see all the discussion)
> https://phabricator.haskell.org/D4327
> https://phabricator.haskell.org/D4324
> https://phabricator.haskell.org/D4679
> https://phabricator.haskell.org/D4644
> https://phabricator.haskell.org/D4722
> https://phabricator.haskell.org/D4302
>
> The discussion here is substantial and I would be really sad to lose it,
> and I think its relevance goes beyond just the people involved in the
> review. Note the cross-referencing between diffs, the links to code, the
> comments on particular code fragments, and links to pastes hosted by
> Phabricator. It's definitely a lot more than just small-scale suggestions
> for improvements or refactoring.
>
> We also have cross-references from Trac tickets to Phabricator diffs, and
> every diff committed contains a link to the code review (I use these links
> a lot). There are links to Phabricator from mailing lists and wiki pages.
>
> So we could stop doing this, and discuss only on Trac tickets or mailing
> lists. (wiki is not good for discussion). After all, that's what we used to
> do before code review, and we managed. But I think to do that would be a
> loss - GHC development has felt a lot more engaging since we added code
> review, and restricting it to ephemeral discussion would curtail its
> potential dramatically. I'd have to think twice before writing a comment on
> a diff - if I want to discuss something about the overall approach, I'd
> have to leave a link to Trac (and create a ticket if there wasn't one),
> which is a huge faff.
>
> Ok, so I've made my case. This is not me stamping my foot and saying "we
> must do it this way" by any means!  If there's consensus to change things
> then I'll happily adjust my workflows.
>
> Cheers
> Simon
>
>
>
>
> On 7 June 2018 at 07:39, Manuel M T Chakravarty <
> manuel.chakravarty at tweag.io> wrote:
>
>> I very much feel like SimonPJ about this.
>>
>> Moreover, I think, there are two important technical reasons to prefer
>> this style. Firstly, I don’t want to be dependent on any one code review
>> tool. The lock-in that Phabricator has been able to create here is quite
>> tiresome (even if Phacility is probably quite happy about the situation).
>>
>> Secondly, I’d be willing to bet that the history in code reviews is
>> useful only to very few people, namely those who participated in the
>> respective code reviews. This is in direct opposition to creating an open
>> project, which tries to level the playing field as far as possible. Hence,
>> IMHO it runs counter to the aims of this group.
>>
>> And, SimonM, as you bring up the differences between open-source and
>> closed source. I agree that this would be less of an issue in a closed
>> source project. However, I always thought, we want to make contributing to
>> GHC as easy as possible.
>>
>> Cheers,
>> Manuel
>>
>> Am 06.06.2018 um 23:47 schrieb Simon Peyton Jones via Ghc-devops-group <
>> ghc-devops-group at haskell.org>:
>>
>> Interestingly, I think this discussion has really teased out a key
>> difference - perhaps the reason people prefer different tools here is
>> because they're starting from different perspectives about what the tools
>> are for?
>>
>> I think you are right here.   Personally, I find that Phab focuses my
>> attention on the **code**, rather than the design or architecture.  Even
>> where discussions about the latter happen, they are buried in a sea of
>> noise about low-level suggestions.   So my personal preference is to keep
>> the higher level stuff on Trac (or even a wiki page) and regard the code
>> review discussion as ephemeral.
>>
>> Simon
>>
>> *From:* Simon Marlow <marlowsd at gmail.com>
>> *Sent:* 06 June 2018 13:44
>> *To:* Simon Peyton Jones <simonpj at microsoft.com>
>> *Cc:* Manuel M T Chakravarty <manuel.chakravarty at tweag.io>;
>> ghc-devops-group at haskell.org
>> *Subject:* Re: [GHC DevOps Group] State of CI
>>
>>
>> I think this reflects a different philosophy about code review. If we say
>> that code-review is only for small-scale suggestions about the actual code
>> changes, then yes I'd agree it's not all that useful to keep that history
>> around. But we can (and sometimes do) have high-level architectural
>> discussions as part of code review too, and indeed I think it's arguably
>> the right thing to have these discussion around concrete code proposals. We
>> keep all the discussion of the code in one place, and the discussion is
>> attached to the evolving code patches.  In this world, the code discussions
>> really are valuable history.
>>
>>
>>
>> Interestingly, I think this discussion has really teased out a key
>> difference - perhaps the reason people prefer different tools here is
>> because they're starting from different perspectives about what the tools
>> are for?  I don't think there's one true way here. Personally I've been
>> exposed to multiple different working styles, especially having one foot in
>> industry and one in the open-source world, and I've seen different
>> philosophies that work.  We could as a project decide that we don't want to
>> go the way of having high-level discussion alongside code review, in which
>> case that's OK (I would slightly prefer to move in the other direction, but
>> that's just my preference).
>>
>>
>>
>> Cheers
>>
>> Simon
>>
>>
>>
>>
>>
>> On 6 June 2018 at 12:58, Simon Peyton Jones <simonpj at microsoft.com>
>> wrote:
>>
>> We should keep in mind, though, is that past code reviews is valuable
>> content that we can't discard,
>>
>> Like Manuel I’m not at all sure about this.
>>
>> I **do** regard the Trac conversation as a long-term asset, and often
>> refer to tickets from Notes, as a way to say “here’s a more extensive
>> discussion of what’s in the Note”.
>>
>> But the Phab discussion of “please refactor this or that” seems far less
>> valuable.  And actually I don’t think it is substantially cross-referenced
>> from elsewhere.  Where there is substantive conversation about the
>> approach, I’d rather see that on Trac.
>>
>> So for me, long term access to the code-review trail is not very important
>>
>> Simon
>>
>> *From:* Ghc-devops-group <ghc-devops-group-bounces at haskell.org> *On
>> Behalf Of *Manuel M T Chakravarty
>> *Sent:* 06 June 2018 12:50
>> *To:* Simon Marlow <marlowsd at gmail.com>
>> *Cc:* ghc-devops-group at haskell.org
>> *Subject:* Re: [GHC DevOps Group] State of CI
>>
>>
>> Am 06.06.2018 um 19:11 schrieb Simon Marlow <marlowsd at gmail.com>:
>>
>>
>> * None of this work is GitHub specific. Nor all that CircleCI or
>> Appveyor specific for that matter (work is currently focused on
>> improving the test suite).
>> * Our GitHub lock-in factor is currently low to pretty much absent,
>> and would remain low even if the review workflow becomes more
>> systematically GitHub centric (it already is for some small
>> contributions).
>> * That's because tickets remain on Trac, and the code along with the
>> entirety of its history remains in a standard Git repository, GitHub
>> or not. Also because GitHub is not a CI provider, those providers we
>> do use integrate with other code hosting solutions (e.g. Appveyor with
>> GitLab), and the surface area of CI provider-specific code is small.
>>
>>
>> We should keep in mind, though, is that past code reviews is valuable
>> content that we can't discard, nor can we easily migrate it to a different
>> code review platform. At this point we have nearly 5K diffs on Phabricator,
>> many of which have non-trivial code-review trails, and these are
>> cross-referenced from Trac, emails, and other places. Even if we moved to
>> github, we would want to keep Phabricator running so that we have access to
>> this content, and people will experience friction though havng to deal with
>> another system.
>>
>> To me, the friction caused by the transition and the inability to do a
>> clean move is more worrying than the missing code review functionality on
>> github.
>>
>>
>> Actually, to me this is a red flag. Core reviews shouldn’t be essential
>> documentation. I wonder what has been going wrong so that we have got to
>> this situation.
>>
>> If important points are uncovered or documented in code reviews, that
>> information should be included in either the source code or associated
>> documentation. (Having git history as an important record for the evolution
>> of the code is fine as it is now standard to not only have one snapshot of
>> the source, but a source control history of the source as the definitive
>> record of a piece of software. And as we painfully discover, much in
>> contrast to code reviews, the source control history is recorded in a
>> widely understood standard format.)
>>
>> Cheers,
>> Manuel
>>
>>
>>
>> _______________________________________________
>> Ghc-devops-group mailing list
>> Ghc-devops-group at haskell.org
>> https://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devops-group
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.haskell.org/pipermail/ghc-devops-group/attachments/20180608/c39e59ad/attachment-0001.html>