[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149

Amar Tumballi  changed:

   What|Removed |Added

   Priority|unspecified |high



--- Comment #14 from Amar Tumballi  ---
> On the other, it's not the way things are in Gluster now and hence requires a 
> change in habits.

The whole point of this exercise is that we don't need to change habit, tool
does it. Well, you have to get used to the change while reading code.

All the different formats, with 4 space indent is at
https://github.com/nigelbabu/clang-format-sample 

I will request Nigel to run a sample with above argument before maintainers
meeting today to have them compared to the same file.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=TQSClkEIUm=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1568488] Is fstat.gluster.org capturing all the regression failures?

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1568488

Nigel Babu  changed:

   What|Removed |Added

 CC||nig...@redhat.com



--- Comment #1 from Nigel Babu  ---
We don't seem to catch timeout related test failure. However, the other point
is that you would not have seen this yesterday in any case. The cron runs at
0100 UTC. This is 0530 IST, so the updates are not real time. They're offset by
about 24h. I'll keep this bug open to track timeout related failures.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=LHBtFCtYw9=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #13 from Jeff Darcy  ---
IndentCaseLabels: personally I don't like indenting case labels because they
create a gap with no close brace - unless people put braces around cases, which
is a bad habit for other reasons - but I know I'm probably outvoted on that
one.

SpaceBeforeParens: I like the space between function name and arguments. The
fact that none of the "modern" styles is not very compelling by itself (appeal
to popularity) but I'm probably outvoted there too.

AlwaysBreakAfterReturnType: Yes. That's the option I was looking for. For
people who use IDEs it probably doesn't matter, but having the function name
start at the beginning of the line is very handy for those using non-IDE
editors like vi(m). It also leaves more space for parameters, making long
parameter lists - which we tend to have - more readable.

I almost don't have an opinion on the alignment options. For much of my career
I would group the * with the type rather than the variable (i.e. PAS_Left)
because it made more intuitive sense. On one hand, it seems like most of the
world now agrees. On the other, it's not the way things are in Gluster now and
hence requires a change in habits.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=zHAKTcfHCj=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #12 from Xavi Hernandez  ---
Using Chromium as base, basically the changes you propose seem ok. I would like
to propose these additional changes:

* I prefer to use 4 spaces for indent. 2 seems too few to me.

* IndentCaseLabels = true. If we use 4 spaces for indent, we could consider
indenting case labels to make them more readable and similar to other code
blocks enclosed between braces.

* SpaceBeforeParens = ControlStatements. I think the additional space before
parens on function calls doesn't provide any advantage (in fact none of the
default configurations uses it).

* BinPackParameters = true.

* AlwaysBreakAfterReturnType = true. I think that separating return type from
function name is useful in some places that use long function names or argument
types.

* AlignEscapedNewLinesLeft=false. Otherwise it fills files with a lot of
unnecessary spaces.

* AlignConsecutiveDeclarations=true. Using this option has the side effect that
pointers are not correctly aligned as we currently do. Pointers will be
rendered like this:

int32_t var1 = 1;
void *  var2 = NULL;

* AlignConsecutiveAssignments=true.

I'm not sure if the last two options provide any interesting advantage. So, if
the pointer alignment is more important, we can disable them.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=NWRhQJ1EHB=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #11 from Jeff Darcy  ---
Whatever we pick is going to require some tweaking. Some of the things I
notice:

 * All use two-space indents, which I personally find atrocious.

 * All put a function's type on the same line as its name.

 * All put the opening brace for a function on the last line of the arguments,
instead of at the beginning of a separate line.

 * Mozilla, Webkit, and LLVM indent 'case' from 'switch'

 * None enforce a space between a function name and opening paren in calls.

 * Mozilla, LLVM, and Google all express pointers as "type* name" instead of
the more familiar (in our code) "type *name"

So Chromium seems closest, but none are particularly close. Looking at the
clang-format pages, here are some options I'd suggest for compatibility with
our existing standard:

 !AllowShortIfStatementsOnASingleLine
 !AllowShortLoopsOnASingleLine
 BraceWrapping(!AfterControlStatement)
 BraceWrapping(AfterFunction)
 BraceWrapping(!BeforeElse)
 ColumnLimit(80)
 !IndentCaseLabels
 IndentWidth(8) or IndentWidth(4) depending on outcome of that conversation
 PointerAlignment(PAS_Right)
 SpaceBeforeParens(SBPO_Always)
 TabWidth(8)
 UseTab(UT_Never)

This fixes most of the warts, though I don't see an option to control function
type and name on the same line and I didn't expect one to enforce braces around
single statements (which would be nice).

BTW, yes, I know that our coding style is rather archaic. The fact that all of
Clang's defined styles are so different than ours is testament to that. There
are some who would argue that any "big bang" reformatting should modernize as
many aspects as possible, including even function opening braces and
(*shudder*) two-wide indents. I'm not necessarily opposed to such changes, but
I think they need to be considered *individually* and not just adopted by
accident. We already did that once, when we imported a check script from the
Linux kernel without anything resembling proper review and it turned out to be
wildly incompatible with our existing code. Let's not repeat that mistake.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=zW0VzdcJDh=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1568488] New: Is fstat.gluster.org capturing all the regression failures?

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1568488

Bug ID: 1568488
   Summary: Is fstat.gluster.org capturing all the regression
failures?
   Product: GlusterFS
   Version: mainline
 Component: project-infrastructure
  Assignee: b...@gluster.org
  Reporter: amukh...@redhat.com
CC: b...@gluster.org, gluster-infra@gluster.org



Description of problem:

https://build.gluster.org/job/centos7-regression/712/console <-- failed today 
tests/00-geo-rep/00-georep-verify-setup.t failing

but
https://fstat.gluster.org/summary?start_date=2018-04-16_date=2018-04-17=master
doesn't capture it..

I am pretty sure today we have definitely more than 10 regression failures..

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=LTcEAxTyAM=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1549000] line-coverage tests not capturing details properly.

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1549000

Amar Tumballi  changed:

   What|Removed |Added

 Status|MODIFIED|CLOSED
 Resolution|--- |CURRENTRELEASE
Last Closed||2018-04-17 05:29:03



--- Comment #3 from Amar Tumballi  ---
Latest master is all fine. As this doesn't need a specific release, closing the
issue.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=DUWjTpHCZr=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #10 from Amar Tumballi  ---
(Update from Nigel)

Which format should we start with as base? Google/LLVM/Mozilla/Webkit/Chromium
   *
[Chromium](https://gist.github.com/nigelbabu/667f3b626779bb14858aa4b520e67c18#file-chromium-c)
 
*
[Mozilla](https://gist.github.com/nigelbabu/667f3b626779bb14858aa4b520e67c18#file-mozilla-c)
*
[LLVM](https://gist.github.com/nigelbabu/667f3b626779bb14858aa4b520e67c18#file-llvm-c)
*
[Webkit](https://gist.github.com/nigelbabu/667f3b626779bb14858aa4b520e67c18#file-webkit-c)
*
[Google](https://gist.github.com/nigelbabu/667f3b626779bb14858aa4b520e67c18#file-google-c)

When do we want to make this change? Before the 4.1 branching seems like a good
time to make vast changes.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=Dee27RsksB=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #9 from Amar Tumballi  ---
(In reply to Nigel Babu from comment #7)
> Alright. So we have agreement that this is a good idea. How do we want to
> implement this? As a check or as a pre-commit hook?

This is a multi-step process!

0. Team agrees on a style and a config file representing the style.
1. Commit the coding style guide to codebase and make changes in rfc.sh to use
it.
2. 'gluster-ant' commits a single large patch for whole codebase with a
standard clang-format style. (This should be only changes which happened due to
clang-format, and no other changes should be in the patch. This can crash
gerrit if we send it to review. 
  -> NOTE: This change can be as big as moving gluster repo from tla to git, as
we have now 2 repositories, 'historic' and 'glusterfs' to understand the actual
source of a line, if one needs 'git blame'.

3. Have the job ready to check the patch with the config file, on the server
side (along with a pre-check in rfc.sh to warn people), this should be a Voting
job in smoke.

4. We all live happily ever after.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=rNXvC1NMQK=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149



--- Comment #8 from Amar Tumballi  ---
(In reply to Shreyas Siravara from comment #6)
> Whoever came up with this idea is a genius, and I totally ack it :)

All credits for this to Shreyas Siravara! As the original idea was proposed
while we were walking for dinner on Boston's streets.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=dndCr6e7EV=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564149] Agree upon a coding standard, and automate check for this in smoke

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564149

Nigel Babu  changed:

   What|Removed |Added

 CC||nig...@redhat.com



--- Comment #7 from Nigel Babu  ---
Alright. So we have agreement that this is a good idea. How do we want to
implement this? As a check or as a pre-commit hook?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=DAhAqpl7XZ=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra

[Gluster-infra] [Bug 1564115] need option 'run brick-mux-tests' in reviews

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1564115

Nigel Babu  changed:

   What|Removed |Added

 Status|NEW |POST



--- Comment #5 from Nigel Babu  ---
WIP: https://review.gluster.org/#/c/19885/

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug 
https://bugzilla.redhat.com/token.cgi?t=DRNX9cNcCU=cc_unsubscribe
___
Gluster-infra mailing list
Gluster-infra@gluster.org
http://lists.gluster.org/mailman/listinfo/gluster-infra