There is also this proposed change to the work flow that I did not
include only because I don't understand it.
Proposed Steps from Contribution to Commit
I think the work flow should be like this:
1.
Use git Pre-commit: on developers machine. To run Code-Style (CS)
and formatting (EOL checks)
1.
This git script is called on commit that runs the formatting
tool on all the changed files.
A formatting tool is needed. It has to be absolute or categorise
violations as warnings and errors. The class of warnings could be long
line length.
The formatting tool should be able to show a diff of the working
directory and suggested changes. Ideally it offers a command line
suggestion to fix tie formatting of the subject file.
The configuration or tool must live in the repo. In the case of the tool
it must be built prior to commit by the build or the pre-comit hook.
Note: In the absence of such a tool a y/n prompt can be issued. But this
will lead to push hecks failing.
2.
PR or patch received (where? how? Via incubator email?
1.
If using github with git box sync - this would be a PR.
3.
The PR and/or pushes to the branch Ttriggers automated checking:
1.
Verify that it follows the coding standard, and
2.
1.
This is the same tool used locally for the pre-commit.
3.
vVerify that the build is not broken.This is a parallel
distributed build.
1.
Compile: Linux
2.
Compile: Windows
3.
Compile: MacOX
4.
???
Ideally the push front end script will Auto detect the affected
subsystem and arch/board and build all the configs for it first.
If either failThese failures, ask the contributor to fix the problem and
resubmit push the changes. (This is automatic in the tooling of github.)
On 12/21/2019 1:26 PM, Gregory Nutt wrote:
As you can see, I have been tried to forward relevant emails from this
thread. There are at least two and maybe three that I cannot find.
First there is the text which I appended to Nathan's workflow:
Proposed Work Flow
Proposed Steps from Contribution to Commit
I think the work flow should be like this:
1.
PR or patch received (where? how? Via incubator email?
2.
Triggers automated checking:
1.
Verify that it follows the coding standard, and
2.
verify that the build is not broken.
If either fail, ask the contributor to fix the problem and resubmit
the change.
3.
PMC should triage and assign the change to a committer. PMC may
also review for conformance with the Inviolables If this review
fails, the change is declined.
4.
Committer performs final review for technical correctness and
conformance to the Inviolables. If this review fails, the change
is declined otherwise the committer commits the change.
5.
2:42 PM <https://nuttx.slack.com/archives/GM2JH0P3M/p1575146565176000>
Step 1
Changes should include some information about how to test the change.
For modifications to code that is tested by existing configurations,
we would need to know the relevant configurations settings. From that
we should be able to select a set of relevant test configurations.
If the change is a new feature, then it may not be testable using any
existing configuration. In that case, we will have to insist that the
change be accompanied by a configuration that can be used for testing.
Step 2
For now we just need this minimum, but this should extend in the
future as we aim for a higher level of quality assurance.
Step 2a)The NuttX style verification tool, nxstyle, should be used to
check coding style. If the submission does not follow the NuttX
coding style, we will need to ask the contributor to update the change
so that it does.
Nxstyle is an imperfect tool, however. We probably need to manually
check any failed output to verify that the failures are not false alarms.
Step 2b) The brute force Jenkins-style testing is not useful here.
Rather, we need a smarter build. We need to build configurations that
ACTUALLY build the code that is changed by the contribution. Per step
1) the contributor has either provided a new test configuration (which
should be included) as a part of the change, or 2) the contributor has
provided the relevant configuration settings for testing the change.
In the latter case, we should be able to build a test configuration
list by selecting existing configuration that include these
configuration settings.
Step 2c) This is where we may want to add hardware-in-the-loop
testing with a reference board in the future.
Step 3
There should be a list of all committers with the areas in which they
have the best expertise. Assigning a change to a committer should be
simply picking the person with the best expertise, but also accounting
for any backlog. Other committers may need to take up the slack.
Step 4
Ultimately, it is the committer who is responsible for assuring that
(1) the change is technically correct, complete, and of the highest
quality. And that (2) the change is consistent with all of the
principles of the Inviolables: The change must not violate the
portable POSIX interface, the change must conform to the architectural
principles of the OS, the change must not expose any platform
dependencies that would have any impact on other users of NuttX.
At this point, the committer should be confident that the change is in
full compliance with the coding standard and will not break the build.
Then there is Justin Mclean's response to this. I cannot find it, but
it is where the phrase "Scratch what itches" comes from. In Step 4,
Justin recommend not being so former. In most Apache projects,
commiters just "scratch what itches" meaning that they review and
merge commits that are interesting to them.
There is possibly a third email that I cannot find from David Sidrane
that had some thoughts about the work flow. I can't find it and I
don't recall the content. If someone else knows the email that I am
referring to, it would be good to have on this thread as well.
Greg
