Skip to content

Pull Requests

Jump to bottom
Jonathan Lifflander edited this page Jul 21, 2020 · 10 revisions

DARMA\vt developers must submit a pull request (PR) to change code in the DAMRA repositories. The following describes the workflow for code development:

Open an issue

An issue must be open before any work starts on DARMA. The issue might be a bug report or a task to implement a feature or enhance a feature. Please include details such as the motivation for a feature request or detailed information on how to reproduce a bug.

Once the issue is created, a unique number will be generated to refer to the issue. For example, an issue might be created with the number #123. In a PR or another issue, one may refer to that issue by typing #123, which will cause GitHub to automatically create a link to that issue.

If the work associated with an issue is planned to go in a particular release, it should be tagged with that release. There are labels that correspond to each release version that may be added to an issue and PR.

Create a branch labeled with the issue

Once an issue is open, developers should create a branch in the repository that corresponds with the aforementioned issue. The branch name follows the format of <IssueNumber>-<short>-<description>. For the previous example, the branch would be named: 123-new-feature.

A branch in development may be pushed incrementally as development commences to the GitHub repository remote and force-pushed (history rewritten) if appropriate for the work.

Commit formatting

Every commit on the branch must follow a specified format for it to be acceptable to merge. The commit must start with an issue number so it can be referenced easily later or during bisection. Followed by the commit number and a colon, a developer can optionally provide a category tag followed by a colon that indicates which component or part of the code that is being worked on. After this, a short message about the commit must be provided.

After the first line of the commit message that contains the issue number, optional tag, and short description, a long description may be provided that provides much more detail about the commit.

Here's a reference that discusses commit message etiquette.

Example commit message:

#123: active: implement tags for message delivery

Implement two types of tags on the active message envelope that
can be used to control message delivery in the active messenger...

Open a PR in draft mode

Once the new branch is in a state where it might be useful to get feedback, create a PR in "draft mode" that corresponds to the issue. Name the issue starting with the issue number, such as: "123 Implement new feature". The labeling system is very important to follow because the automated release tracker link PRs with issues using the description of the PR.

Opening a PR early can be useful to solicit feedback from other developers before the implementation is complete. Also, once a PR is open, the automatic CI testing will be applied to that branch as it gets updated which may be very helpful for development.

Move PR out of draft mode

Once the implementation is complete, take the PR out of draft mode. At this point, the developer should select reviewers for the code on the branch. GitHub will automatically suggest some reviewers depending on who has recently worked on that part of the code.

Merging the PR

To merge the PR, there are several requirements which are enforced by GitHub.

  1. At least one approving review must exist
    • Each PR is must be reviewed by a developer on the DARMA team. Without an approving review, a PR cannot be merged onto a release/development branch (GitHub is configured to disallow this). A good review must consider a range of qualities in the PR:
      • A clear description of the motivation for "why" the work was performed---referenced from the associated issue.
      • Check that all code style guidelines are followed (for the ones that are not automatically checked)
      • Ensure code works as described and fits in the codebase
      • Ensure that all licenses are obeyed and properly included if new external packages are included
      • Ensure that the concurrent programming model matches with the rest of the codebase
      • Ensure that all interfaces/APIs are documented clearly along with updates to the manual/wiki if relevant for the change
      • Ensure that the code is scalable---has very good isoefficiency. Must not include any unscalable O(P) data structures.
      • Ensure code is readable, extendable, and does not cause bad interactions with other components or use cases
      • Unit/integration tests must be included with the PR that clearly tests the functionality and exercises it sufficiently to be confident that the functionality is being tested.
      • If the PR is addressing a bug report, a test must be included that reproduces the bug (fails or triggers the bug). Without a failing test it isn't clear whether a "fix" is actually correct.
  2. All required tests must pass
    • There are now two levels of tests: basic and extended
      • Basic tests are only run on longer test targets for PRs (like Intel compilers and Nvidia compilers)
      • Once a PR is merged, extended tests run on develop
      • To dispatch extended tests on a PR, add the tag runextendedtests and the extra tests will be triggered on the PR
    • The CI testing is extensive and covers many targets. All the targets marked "required" must pass before the PR can be merged
      1. Intel icpc 18.03 with mpich (ubuntu), basic level of testing
      2. GNU gcc 5 with mpich (ubuntu)
      3. GNU gcc 6 with mpich (ubuntu)
      4. GNU gcc 7 with mpich with tracing runtime enabled on all tests (ubuntu)
      5. GNU gcc 8 with mpich and address sanitizer (ubuntu)
      6. Clang 3.9 with mpich (ubuntu)
      7. Clang 5.0 with mpich (ubuntu)
      8. Clang 8.0 with mpich (alpine)
      9. Clang 8.0 with mpich (Mac OS X)
    • New CI tests being added soon
      1. Intel icpc 19 with mpich (ubuntu)
      2. Nvidia nvcc 10.1 with mpich (ubuntu)
      3. Nvidia nvcc 11.0 with mpich (ubuntu)
  3. All required checks must pass
    • Three required formatting checks
      1. git --check must pass on all new commits
      2. The trailing whitespace checker must pass indicating that no new trailing space has been added
      3. The commit style will check that all commits messages conform to the required style (see above for commit message requirements)
    • Codacy analyzes the commits to check for quality problems in the new code
    • Code coverage for the new development will be analyzed and must remain close to the level of coverage before this PR

Once all these checks/tests pass and an approved review exists, anyone is allowed press the "merge" button to finish the PR. GitHub is configured to only allow code that is up-to-date with the target branch (such as "develop") to be merged. If the code is out-of-date, it must be rebased on the current branch (with all tests being re-run) before it can be merged.

Clone this wiki locally