Re: Automatic changes during the dev workflow [was: Re: Upcoming changes to our C++ Coding Style]

[Ehsan Akhgari]

On Mon, Dec 17, 2018 at 1:22 PM Steve Fink  wrote:

> In theory, I would definitely prefer if all the code were auto-formatted
> during regular development(*), but in practice the performance means
> it's not as transparent as it needs to be -- I have noticed that since
> enabling the format-source extension, rebasing my patch stacks is
> significantly slower.
>

Please if you see performance issues, file them with some steps to
reproduce and CC Andi, he may be able to help.  We should definitely not
have to avoid running our tools due to performance issues.  :-)

Cheers,
-- 
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Automatic changes during the dev workflow [was: Re: Upcoming changes to our C++ Coding Style]

[Ehsan Akhgari]

On Fri, Dec 14, 2018 at 4:55 PM Kartikaya Gupta  wrote:

> On Fri, Dec 14, 2018 at 1:58 PM Sylvestre Ledru 
> wrote:
> > We have more and more tools at review phase (clang-format, flake8,
> eslint, clang-tidy, codespell, etc) which propose some auto-fixes.
>
> Honestly I find it quite annoying when I'm trying to review a patch
> and the phabricator diff is filled up with bot comments about
> formatting. As a reviewer, I shouldn't have to think about formatting,
> and it shouldn't fill up my screen and prevent me from looking at the
> actual code.
>
> > Currently, the turn around time of the tools is 14m on average which is
> usually faster than the reviewer looking at the patch.
>
> Usually, but not always. And 14 minutes is still long enough that the
> person writing the patch has context-switched away and is in the
> middle of doing something else when the bot comes a-knocking.
>

What if we *rejected* misformatted patches in Phabricator as opposed to
just comment on them?  That is, what if the bot would mark the reviews as
"Request Changes" when it found any problems, IOW it would review the patch
alongside with normal reviewers.  As Sylvestre mentioned the diffs on
individual lines is about to go away, but this way we'd also make sure that
misformatting caught at review time would actually block the landing of the
patch.


> > If Phabricator provides the capability, we could have the bot
> automatically proposing a new patchset on top on the normal one.
> > The reviewer would look at the updated version.
>
> This would be fine for the reviewer, but...
>
> > The main drawback would be that developer would have to retrieve the
> updated patch
> > before updating it.
>
> That's a chore too. For the developer writing the patch I feel like
> there should be a way to just say "please format this patch in-place
> for me" (basically what I expected `./mach clang-format` to do, except
> it doesn't quite - see bug 1514279) and even that should be opt-in.
> IMO the default behaviour should just be to automatically apply
> formatting prior to submission, without anybody having to think about
> it except in cases where the developer needs to apply "//clang-format
> off" blocks.
>

Right.  I think the ideal state would be for everyone to use editor
integration and local VCS hooks.  The VCS hooks we can enable in ./mach
bootstrap but editor integration is something people will need to opt into
turning on.  Once we get there, then I think formatting will be mostly
taken care of transparently for you without needing to think about it
consciously.

Doing things like rejecting misformatted code at review time may
incentivize people updating their local configuration.  I'm thinking of
reasons why that may break people's workflow but can't think of any right
now, curious to know if anyone else can?

-- 
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Automatic changes during the dev workflow [was: Re: Upcoming changes to our C++ Coding Style]

[Steve Fink]

In theory, I would definitely prefer if all the code were auto-formatted 
during regular development(*), but in practice the performance means 
it's not as transparent as it needs to be -- I have noticed that since 
enabling the format-source extension, rebasing my patch stacks is 
significantly slower. That's turning out to be a problem because it 
tends to take long enough to trigger a context switch, which is 
especially problematic when attempting to land something, wandering off 
during the rebase, and then realizing that too much time has elapsed and 
I'm going to need to start over and pull again because new changes have 
come in. (Maybe this just means I should get friendly with Lando, but 
even then I tend to have quite a few rebases that I want to tend to 
manually while in the testing/debugging phase.)


I know we collect build time statistics when running `mach build` that 
we can opt into uploading; would it be possible to collect timings for 
eg `hg rebase` operations? Some fancy new gps extension? :-) You 
probably shouldn't trust my gut "it feels slower" impressions too far.


 - sfink


(*) Now that we've taken the hit of uglifying the code, it seems like we 
should get as much mileage as we can out of it.


(I still think it was the right decision to go to a single style, and it 
was better to just pick one rather than blocking on arguing it down to a 
some "globally optimal" style first.)



On 12/14/2018 10:57 AM, Sylvestre Ledru wrote:
I think we should aim at option b) (updated automatically by bots 
after submission to Phabricator)


We have more and more tools at review phase (clang-format, flake8, 
eslint, clang-tidy, codespell, etc) which propose some auto-fixes.


Currently, the turn around time of the tools is 14m on average which 
is usually faster than the reviewer looking at the patch.
If Phabricator provides the capability, we could have the bot 
automatically proposing a new patchset on top on the normal one.

The reviewer would look at the updated version.

By doing that, we would save time for everyone. The main drawback 
would be that developer would have to retrieve the updated patch

before updating it.

Sylvestre


Le 13/12/2018 à 23:53, Ehsan Akhgari a écrit :
Previously I had shied away from ideas similar to (a) or (b) since I 
wasn't sure if that would be what developers would expect and accept, 
given that it would cause the change the reviewer sees to no longer 
match their local change, which could cause hicups e.g. when trying 
to find the code that a reviewer has commented on locally, or when 
rebasing on top of a newer version of mozilla-central after Lando has 
landed your patch (which may cause conflicts with your local patch 
due to formatting differences).


Do we think these types of issues aren't something we should be 
concerned about?


Thanks,
Ehsan

On Fri, Dec 7, 2018 at 3:09 PM Andrew Halberstadt > wrote:


    I think we should implement a) and do the formatting prior to
    submission. This prevents us from wasting reviewer time on format
    issues, and also makes sure that "what you see in phab, is what
    gets landed".

    On Fri, Dec 7, 2018, 2:04 PM Gregory Szorc, mailto:g...@mozilla.com>> wrote:

    On Fri, Dec 7, 2018 at 1:57 PM Botond Ballo
    mailto:bba...@mozilla.com>> wrote:

    > On Fri, Dec 7, 2018 at 11:36 AM Sylvestre Ledru
    mailto:sle...@mozilla.com>>
    > wrote:
    > > In the meantime, we will be running a bot weekly to
    reformat the
    > > mistakes and add the changeset into the ignore lists.
    > > But in the long run this won’t be sustainable, so once we 
gain

    > > confidence that a good number of developers have
    successfully integrated
    > > clang-format into their local workflow, we will look into
    enabling a
    > > Mercurial hook on hg.mozilla.org 
    to reject misformatted code upon push
    > > time.  That will be the ultimate solution to help ensure
    that our code
    > > will be properly formatted at all times.
    >
    > Have you considered something like running clang-format
    automatically
    > during landing (i.e. as part of what Lando does to the
    patch)? That
    > seems like it would achieve the best of both worlds, by not
    placing
    > any requirements on what developers do locally, while also
    ensuring
    > the tree is always properly formatted without cleanup commits.
    >

    I chatted with Sylvestre earlier today. While I don't want to
    speak for
    him, I believe we both generally agree that the formatting
    should happen
    "automagically" as part of the patch review and landing
    lifecycle, even if
    the client doesn't have their machine configured for
    formatting on save.
    This would mean that patches are either:

    a) auto-formatted 

Re: Automatic changes during the dev workflow [was: Re: Upcoming changes to our C++ Coding Style]

[Sylvestre Ledru]

Le 14/12/2018 à 22:55, Kartikaya Gupta a écrit :

On Fri, Dec 14, 2018 at 1:58 PM Sylvestre Ledru  wrote:

We have more and more tools at review phase (clang-format, flake8, eslint, 
clang-tidy, codespell, etc) which propose some auto-fixes.

Honestly I find it quite annoying when I'm trying to review a patch
and the phabricator diff is filled up with bot comments about
formatting. As a reviewer, I shouldn't have to think about formatting,
and it shouldn't fill up my screen and prevent me from looking at the
actual code.


We received this feedback from several folks and we agree (initially, we were 
showing the diff itself but it wasn't
bringing much).
This is fixed in staging and the fix should go in the next release of the 
platform (Dec 22nd)

Cheers

S


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Automatic changes during the dev workflow [was: Re: Upcoming changes to our C++ Coding Style]

[Jean-Yves Avenard]

Hi

> On 14 Dec 2018, at 7:57 pm, Sylvestre Ledru  wrote:
> 
> I think we should aim at option b) (updated automatically by bots after 
> submission to Phabricator)
> 
> 

I don’t particularly fancy this idea. Finding yourself with different code on 
Phabricator and locally is a good way to shoot yourself in the foot.

Preventing pushing non-properly formatted code, it’s so easy to properly format 
your code. Also make you have on extra check on what you’re about to review.

Similar to the Google review process. IIRC before anyone is asked to review 
anything, the submission must pass a set of tests, one of them includes 
checking the coding style.

> We have more and more tools at review phase (clang-format, flake8, eslint, 
> clang-tidy, codespell, etc) which propose some auto-fixes.
> 
> Currently, the turn around time of the tools is 14m on average which is 
> usually faster than the reviewer looking at the patch.
> If Phabricator provides the capability, we could have the bot automatically 
> proposing a new patchset on top on the normal one.
> The reviewer would look at the updated version.
> 
> 

It does feel longer that that :)

> By doing that, we would save time for everyone. The main drawback would be 
> that developer would have to retrieve the updated patch 
> before updating it.
> 
> 

A big drawback

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Automatic changes during the dev workflow [was: Re: Upcoming changes to our C++ Coding Style]

[Sylvestre Ledru]

I think we should aim at option b) (updated automatically by bots after 
submission to Phabricator)


We have more and more tools at review phase (clang-format, flake8, 
eslint, clang-tidy, codespell, etc) which propose some auto-fixes.


Currently, the turn around time of the tools is 14m on average which is 
usually faster than the reviewer looking at the patch.
If Phabricator provides the capability, we could have the bot 
automatically proposing a new patchset on top on the normal one.

The reviewer would look at the updated version.

By doing that, we would save time for everyone. The main drawback would 
be that developer would have to retrieve the updated patch

before updating it.

Sylvestre


Le 13/12/2018 à 23:53, Ehsan Akhgari a écrit :
Previously I had shied away from ideas similar to (a) or (b) since I 
wasn't sure if that would be what developers would expect and accept, 
given that it would cause the change the reviewer sees to no longer 
match their local change, which could cause hicups e.g. when trying to 
find the code that a reviewer has commented on locally, or when 
rebasing on top of a newer version of mozilla-central after Lando has 
landed your patch (which may cause conflicts with your local patch due 
to formatting differences).


Do we think these types of issues aren't something we should be 
concerned about?


Thanks,
Ehsan

On Fri, Dec 7, 2018 at 3:09 PM Andrew Halberstadt > wrote:


I think we should implement a) and do the formatting prior to
submission. This prevents us from wasting reviewer time on format
issues, and also makes sure that "what you see in phab, is what
gets landed".

On Fri, Dec 7, 2018, 2:04 PM Gregory Szorc, mailto:g...@mozilla.com>> wrote:

On Fri, Dec 7, 2018 at 1:57 PM Botond Ballo
mailto:bba...@mozilla.com>> wrote:

> On Fri, Dec 7, 2018 at 11:36 AM Sylvestre Ledru
mailto:sle...@mozilla.com>>
> wrote:
> > In the meantime, we will be running a bot weekly to
reformat the
> > mistakes and add the changeset into the ignore lists.
> > But in the long run this won’t be sustainable, so once we gain
> > confidence that a good number of developers have
successfully integrated
> > clang-format into their local workflow, we will look into
enabling a
> > Mercurial hook on hg.mozilla.org 
to reject misformatted code upon push
> > time.  That will be the ultimate solution to help ensure
that our code
> > will be properly formatted at all times.
>
> Have you considered something like running clang-format
automatically
> during landing (i.e. as part of what Lando does to the
patch)? That
> seems like it would achieve the best of both worlds, by not
placing
> any requirements on what developers do locally, while also
ensuring
> the tree is always properly formatted without cleanup commits.
>

I chatted with Sylvestre earlier today. While I don't want to
speak for
him, I believe we both generally agree that the formatting
should happen
"automagically" as part of the patch review and landing
lifecycle, even if
the client doesn't have their machine configured for
formatting on save.
This would mean that patches are either:

a) auto-formatted on clients as part of being submitted to
Phabricator
b) updated automatically by bots after submission to Phabricator
c) auto-formatted by Lando as part of landing

Lando rewrites/rebases commits as part of landing, so commit
hashes already
change. So if auto-formatting magically occurs and "just
works" as part of
the review/landing process, there should be little to no developer
inconvenience compared to what happens today. i.e. developers
don't need to
do anything special to have their patches land with proper
formatting.
___
dev-platform mailing list
dev-platform@lists.mozilla.org

https://lists.mozilla.org/listinfo/dev-platform



--
Ehsan

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Automatic changes during the dev workflow [was: Re: Upcoming changes to our C++ Coding Style]

[Kartikaya Gupta]

On Fri, Dec 14, 2018 at 1:58 PM Sylvestre Ledru  wrote:
> We have more and more tools at review phase (clang-format, flake8, eslint, 
> clang-tidy, codespell, etc) which propose some auto-fixes.

Honestly I find it quite annoying when I'm trying to review a patch
and the phabricator diff is filled up with bot comments about
formatting. As a reviewer, I shouldn't have to think about formatting,
and it shouldn't fill up my screen and prevent me from looking at the
actual code.

> Currently, the turn around time of the tools is 14m on average which is 
> usually faster than the reviewer looking at the patch.

Usually, but not always. And 14 minutes is still long enough that the
person writing the patch has context-switched away and is in the
middle of doing something else when the bot comes a-knocking.

> If Phabricator provides the capability, we could have the bot automatically 
> proposing a new patchset on top on the normal one.
> The reviewer would look at the updated version.

This would be fine for the reviewer, but...

> The main drawback would be that developer would have to retrieve the updated 
> patch
> before updating it.

That's a chore too. For the developer writing the patch I feel like
there should be a way to just say "please format this patch in-place
for me" (basically what I expected `./mach clang-format` to do, except
it doesn't quite - see bug 1514279) and even that should be opt-in.
IMO the default behaviour should just be to automatically apply
formatting prior to submission, without anybody having to think about
it except in cases where the developer needs to apply "//clang-format
off" blocks.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform