Adding more CODEOWNERS #6422

TGWDB · 2021-11-01T13:50:21Z

This is a first draft of adding some more CODEOWNERS in different places.
The underlying goal here is to ensure that Diffblue has two (or more)
in house CODEOWNERS for all regularly updated parts of the code. This
is to enable Diffblue to respond to the vast majority of the code base
and also reduce reliance upon a single person (within or without of
Diffblue).

NOTE: this is put up now for discussion and refinement. Consider this a provisional starting point.

Each commit message has a non-empty body, explaining why the change was made.
Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
My commit message includes data points confirming performance improvements (if claimed).
My PR is restricted to a single feature or bugfix.
White-space or formatting changes outside the feature-related changed lines are in commits of their own.

This is a first draft of adding some more CODEOWNERS in different places. The underlying goal here is to ensure that Diffblue has two (or more) in house CODEOWNERS for all regularly updated parts of the code. This is to enable Diffblue to respond to the vast majority of the code base and also reduce reliance upon a single person (within or without of Diffblue).

tautschnig · 2021-11-01T14:59:04Z

To all those being added as code owners, and for each directory: do you feel confident in deciding whether a change is the right one to make? That is to say: only in parts should others decide whether to approve of someone's code ownership. The main approval is to be provided by the person being added.

tautschnig · 2021-11-01T15:01:05Z

CODEOWNERS

 /src/cpp/ @kroening @tautschnig @peterschrammel
-/src/solvers/smt2 @kroening @martin-cs @tautschnig @peterschrammel @allredj @romainbrenguier


While at it, please remove my name from this one. I do not feel sufficiently competent/confident for SMT2.

Done in pending commit.

OK for me. I'm not active on SMT2 at the moment.

codecov · 2021-11-01T15:08:34Z

Codecov Report

Merging #6422 (9d916e3) into develop (f6fa03c) will increase coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #6422      +/-   ##
===========================================
+ Coverage    75.98%   76.01%   +0.02%     
===========================================
  Files         1524     1527       +3     
  Lines       164299   164463     +164     
===========================================
+ Hits        124843   125011     +168     
+ Misses       39456    39452       -4

Impacted Files	Coverage Δ
src/goto-symex/symex_dead.cpp	`95.65% <0.00%> (-4.35%)`	⬇️
src/ansi-c/scanner.l	`61.75% <0.00%> (ø)`
src/solvers/smt2_incremental/smt_commands.h	`100.00% <0.00%> (ø)`
...it/solvers/smt2_incremental/smt_to_smt2_string.cpp	`100.00% <0.00%> (ø)`
src/solvers/smt2_incremental/smt_responses.cpp	`100.00% <0.00%> (ø)`
src/solvers/smt2_incremental/smt_responses.def	`87.50% <0.00%> (ø)`
unit/solvers/smt2_incremental/smt_responses.cpp	`100.00% <0.00%> (ø)`
src/analyses/goto_check.cpp	`88.77% <0.00%> (+0.30%)`	⬆️
...rc/solvers/smt2_incremental/smt_to_smt2_string.cpp	`93.96% <0.00%> (+1.72%)`	⬆️
src/solvers/smt2_incremental/smt_responses.h	`100.00% <0.00%> (+100.00%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce2680b...9d916e3. Read the comment docs.

chrisr-diffblue

Seems reasonable to me.

TGWDB · 2021-11-01T18:09:09Z

To all those being added as code owners, and for each directory: do you feel confident in deciding whether a change is the right one to make? That is to say: only in parts should others decide whether to approve of someone's code ownership. The main approval is to be provided by the person being added.

This is a great point and part of why I marked this RFC, I'd very much appreciate comment/approval/discussion from those effected (@peterschrammel @thomasspriggs @NlightNFotis @allredj @romainbrenguier ).

thomasspriggs · 2021-11-03T11:16:18Z

CODEOWNERS

-/src/ansi-c/ @kroening @tautschnig @chrisr-diffblue
-/src/assembler/ @kroening @tautschnig @chrisr-diffblue
+/src/ansi-c/ @kroening @tautschnig @chrisr-diffblue @peterschrammel @thomasspriggs @NlightNFotis @TGWDB
+/src/assembler/ @kroening @tautschnig @chrisr-diffblue @peterschrammel @thomasspriggs @NlightNFotis @TGWDB


I can't say that I am any kind of authority on our assembly support, having never worked on its implementation in cbmc. However I have enough familiarity with cbmc front-ends in general, that I am not going to strenuously object to being made a codeowner of this, if we need more codeowners.

Dare I question whether the above changes are in line with "regularly updated parts of the code?"

This assembler sub folder has not been updated particularly regularly, as I read the commit history. Therefore adding more code owners for it is not in line with this PR's "regularly updated parts of the code" description. @TGWDB What is your view on this?

I don't know anything about what's in src/assembler myself, so I don't think I'm particularly suited for being a code owner for this.

I'll reduce some of these (commit ~~to come shortly~~ pushed). I agree they may not be "regular" as the PR suggests, on the other hand we had a problem with getting a PR approved by codeowner around the ansi-c part, so I'll add a little more here.

If it helps, my role has very recently changed so that I now have a lot more time to work on CBMC.

thomasspriggs · 2021-11-03T11:16:54Z

CODEOWNERS

 /src/cpp/ @kroening @tautschnig @peterschrammel
-/src/solvers/smt2 @kroening @martin-cs @tautschnig @peterschrammel @allredj @romainbrenguier
+/src/solvers/smt2 @kroening @martin-cs @tautschnig @peterschrammel @thomasspriggs @NlightNFotis @TGWDB


This, I've worked on a bit in the past, however given that the aim is sunsetting it in favour of the new SMT backend, I'm not sure what value code-ownership of this brings (I expect this to be a low traffic area of the code, but then again I might be mistaken).

I'm tempted to keep this since we've had a few fixes and updates here, and with work on the new SMT side I can see some overlap in the future (bug fixes, minor refactors, etc.).

@tautschnig

Reduce a few ownership suggestions based on feedback from various existing/proposed codeowners: @tautschnig @thomasspriggs @NlightNFotis

kroening

I am unhappy with the additions to /util and /goto-programs -- these are very sensitive, and being slow here is good.

TGWDB · 2021-11-04T13:54:44Z

I am unhappy with the additions to /util and /goto-programs -- these are very sensitive, and being slow here is good.

I would like to clarify what would be acceptable here for you (and others)? To clarify, the goal of this PR is NOT to make changes fast, particularly to sensitive areas. The goal is to have redundancy so that we do not rely on 1 person (or less) that Diffblue can reach reliably for review/approval. There appear to be three obvious possible ways forward (and feel free to simply say which is best):

Remove the new codeowners here and hope that we do not have issues on obtaining reviews/approval here in future. This is not ideal from my perspective, but if this is the only acceptable way forward then we can do that for this PR (and if necessary revisit these areas in future).
Reduce the new codeowners here to some agreed/limited people.
Accept the new codeowners here with the commitment from them to act with appropriate care on these parts of the code.

To be clear, I'm willing to proceed with any of these (or others if suggested).

thomasspriggs · 2021-11-04T14:16:20Z

I am unhappy with the additions to /util and /goto-programs -- these are very sensitive, and being slow here is good.

@kroening - There are more than 200 files in /util covering a wide variety of different pieces of functionality. Would you be open to a more fine-grained approach to ownership of files within this directory? It could be setup such that certain files are considered to be high risk with a reduced sub-set of code owners and a larger group of code owners for the directory as a whole, for example.

kroening · 2021-11-04T14:54:46Z

@kroening - There are more than 200 files in /util covering a wide variety of different pieces of functionality. Would you be open to a more fine-grained approach to ownership of files within this directory? It could be setup such that certain files are considered to be high risk with a reduced sub-set of code owners and a larger group of code owners for the directory as a whole, for example.

Sure, happy to consider individual files.

This removes some contentions potential codeowner additions and reverts to the old owners. Discussion suggested finer grained codeowners for /src/util, this should come in a follow-up PR.

TGWDB · 2021-11-04T15:53:51Z

I've pushed an update that removes the contentious additions got /src/goto-programs and /src/util. For the /src/util (at least) I propose this comes in another PR with some exploration/discussion of which files to have special owners for.

thomasspriggs · 2021-11-08T15:07:09Z

I have included some of the SMT2 related changes in my own SMT2 codeowners PR here - #6445

TGWDB · 2021-11-17T10:53:57Z

@kroening Any comment on the updated PR?

tautschnig · 2021-11-17T12:03:55Z

Should we also look at the entries that only have "@diffblue/.*" to ensure the project does not end up being blocked at times where Diffblue cannot commit resources? As a data point, #6367 requires such approval as a new top-level directory was added to the CMake configuration.

TGWDB · 2021-11-22T08:47:23Z

Should we also look at the entries that only have "@diffblue/.*" to ensure the project does not end up being blocked at times where Diffblue cannot commit resources? As a data point, #6367 requires such approval as a new top-level directory was added to the CMake configuration.

Yes, that's a reasonable suggestion. Did you have any in particular in mind?

tautschnig · 2021-11-22T12:15:24Z

Should we also look at the entries that only have "@diffblue/.*" to ensure the project does not end up being blocked at times where Diffblue cannot commit resources? As a data point, #6367 requires such approval as a new top-level directory was added to the CMake configuration.

Yes, that's a reasonable suggestion. Did you have any in particular in mind?

How about just adding the default code owners?

TGWDB added do not merge RFC Request for comment labels Nov 1, 2021

tautschnig reviewed Nov 1, 2021

View reviewed changes

chrisr-diffblue approved these changes Nov 1, 2021

View reviewed changes

Remove @tautschnig from smt2 at their request

d798a83

thomasspriggs reviewed Nov 3, 2021

View reviewed changes

TGWDB marked this pull request as ready for review November 4, 2021 11:08

TGWDB requested review from kroening and peterschrammel as code owners November 4, 2021 11:08

TGWDB removed the do not merge label Nov 4, 2021

Update with feedback from @tautschnig @thomasspriggs @NlightNFotis

deae483

Reduce a few ownership suggestions based on feedback from various existing/proposed codeowners: @tautschnig @thomasspriggs @NlightNFotis

peterschrammel approved these changes Nov 4, 2021

View reviewed changes

kroening requested changes Nov 4, 2021

View reviewed changes

TGWDB changed the title ~~[DRAFT/RFC] Adding more CODEOWNERS~~ [RFC] Adding more CODEOWNERS Nov 4, 2021

Remove contentious codeowner additions on sensitive code

9d916e3

This removes some contentions potential codeowner additions and reverts to the old owners. Discussion suggested finer grained codeowners for /src/util, this should come in a follow-up PR.

TGWDB removed the RFC Request for comment label Nov 17, 2021

TGWDB changed the title ~~[RFC] Adding more CODEOWNERS~~ Adding more CODEOWNERS Nov 17, 2021

tautschnig assigned kroening Nov 17, 2021

TGWDB closed this Jan 19, 2022

		/src/cpp/ @kroening @tautschnig @peterschrammel
		/src/solvers/smt2 @kroening @martin-cs @tautschnig @peterschrammel @allredj @romainbrenguier

Adding more CODEOWNERS #6422

Adding more CODEOWNERS #6422

Uh oh!

Conversation

TGWDB commented Nov 1, 2021

Uh oh!

tautschnig commented Nov 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chrisr-diffblue left a comment

Choose a reason for hiding this comment

Uh oh!

TGWDB commented Nov 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TGWDB Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kroening left a comment

Choose a reason for hiding this comment

Uh oh!

TGWDB commented Nov 4, 2021

Uh oh!

thomasspriggs commented Nov 4, 2021

Uh oh!

kroening commented Nov 4, 2021

Uh oh!

TGWDB commented Nov 4, 2021

Uh oh!

thomasspriggs commented Nov 8, 2021

Uh oh!

TGWDB commented Nov 17, 2021

Uh oh!

tautschnig commented Nov 17, 2021

Uh oh!

TGWDB commented Nov 22, 2021

Uh oh!

tautschnig commented Nov 22, 2021

Uh oh!

Uh oh!

codecov bot commented Nov 1, 2021 •

edited

Loading

TGWDB Nov 4, 2021 •

edited

Loading