Skip to content

Latest commit

 

History

History
231 lines (176 loc) · 8.58 KB

CONTRIBUTING.rst

File metadata and controls

231 lines (176 loc) · 8.58 KB
Raw

Contributing to Graphene

First off, thank you for your interest in contributing to Graphene!

In general, code contributions should be submitted to the Graphene project using a pull request.

Reporting Bugs

In order to report a problem, please open an issue in the issue tracker.

Reporting Security Vulnerabilities

Currently, we aren't aware of any production usage of Graphene, so feel free to report them using GitHub issues. Please note that this guideline may change quite soon, so please always check it before publicly reporting any vulnerabilities.

Architectural Changes

Major reorganizations, architectural changes, or code reorganization are best discussed with the maintainers in advance of writing code. We welcome contributions, and would hate for anyone to waste time implementing a change that will not be accepted for a design flaw. It is much better to reach out for advice first by emailing [email protected].

Or you can see the archives at this google group: https://groups.google.com/forum/#!forum/graphene-support

Please verify that your change doesn't introduce any insecure-by-default functionality. If an option allows users to introduce a security risk, the option should have a name prefixed with insecure__ and be disabled by default.

Simple bugfixes need not have advance discussion, but we welcome queries from newcomers.

Branch Names

For work in progress (for team members), please use your name/userid as a prefix in the branch name. For example, if user jane is adding feature foo, the branch should be named: jane/foo.

For new contributors, the branch will likely be on a fork of the repository.

Otherwise, branches without this prefix should only be created for a specific purpose, as approved by the maintainers.

Pull Requests

The primary mechanism for submitting code changes is with a pull request (PR).

In general, a PR should:

  1. Address a single problem.
  2. Clearly explain the problem and solution in the PR and commit messages, using grammatically correct American English.
  3. Include unit tests for the new behavior or bugfix, except in special circumstances, namely: when designing a unit test is difficult (e.g., the code is deep enough in Graphene that it would require extra hooks for testing) or cannot be easily tested (e.g., a performance fix).
  4. Follow project's style guidelines.

PR Life Cycle

We use git-rebase workflow and Reviewable.io for reviews.

TL;DR: Merge commits are never allowed and force-pushes are not allowed after a review has started. Before merging, commits will be cleaned up, rebased onto the current master and tested again in CI.

Detailed explanation:

  1. A PR is created. If the authors know a good candidate for the review (e.g., the author of the specific component) they should assign a suggested reviewer on GitHub.
  2. From this point on the branch is public, which means that one should ask reviewers' permission before doing a force-push.
  3. Reviewers shouldn't push commits to the PR, only the authors are allowed to do so.
  4. Reviewers add comments to the changes using Reviewable.io integration.
  5. The author discusses the remarks and implements fixes in separate commits (ideally, using git commit --fixup). Loop to point 4. until the PR is approved (see :ref:`merging_policy`).
  6. One of the maintainers squashes fix-up commits with original ones, rebases the branch onto the current master and force-pushes the branch to GitHub to share.

PR Merging Policy

Before a pull request is merged, it must:

  1. Pass all CI tests

  2. Follow project's style guidelines.

  3. Introduce no new compilation errors or warnings

  4. Have all discussions from reviewers resolved

  5. Have a clear, concise and grammatically correct comments and commit messages.

  6. Have a quorum of approving reviews from maintainers and/or waited an appropriate amount of time. This can be:

    • 3 approving maintainers
    • 2 approving maintainers and 5 days since the PR was created

    If the author is a maintainer the limits are lowered by 1.

Additional reviews from anyone are welcome.

Reviewing Guidelines

  1. All commits must be atomic (i.e., no unrelated changes in the same commit, no formatting fixes mixed with features, no moving files and changing them at the same time).
  2. Meaningful commit messages (it's much easier to get them right if commits are really atomic). Should include which component was changed (Pal-{Linux,SGX} / LibOS / Docs / CI) in the format "[component] change description".
  3. Every PR description should include: what's the purpose of the changes, what is changed (and how, in case of redesigning a component), and how to test the changes.
  4. Is it possible to implement this change in a significantly better way?
  5. It's C, so check for common problems: correct buffer sizes, integer overflows, memory leaks, violations of pointer ownership etc.
  6. Verify if all macro parameters are used with additional parentheses.
  7. Check for race conditions.
  8. Check if all errors are checked and properly handled.
  9. Suggest adding assertions (if appropriate). Especially for ensuring invariants after a complex operation.
  10. Check for possibilities of undefined behaviours (e.g. signed overflow).
  11. If the PR fixed a bug, there should be a regression test included in the change. The commit containing it should be committed before the fix, so the reviewer can easily run it before and after the fix.
  12. Code style must follow our guidelines (see below).

Style Guidelines

See style guidelines.

Running Regression Tests by Hand

All of our regression tests are automated in Jenkins jobs (see the Jenkinsfiles directory), and this is the ultimate documentation for application-level regression tests, although most tests can be run with :command:`make regression` or, in the worst case, should have a simple script called by Jenkins.

We also have (and are actively growing) PAL and shim unit tests.

To run the PAL tests:

cd Pal/regression
make regression

For SGX, one needs to do the following:

cd Pal/regression
make SGX=1 regression

If a test fails unexpectedly, one can use the :makevar:`KEEP_LOG=1` option to get the complete output.

One can run tests manually:

PYTHONPATH=path/to/graphene/Scripts
PAL_LOADER=path/to/pal-Linux
LIBPAL_PATH=path/to/libpal-Linux.so
export PYTHONPATH PAL_LOADER LIBPAL_PATH
python3 -m pytest -v -rs test_pal.py

It is also possible to run subset of tests:

# after env export
python3 -m pytest -v -rs test_pal.py::TC_01_Bootstrap
python3 -m pytest -v -rs test_pal.py::TC_01_Bootstrap::test_100_basic_boostrapping

The shim unit tests work similarly, and are under :file:`LibOS/shim/test/regression`.

LTP

Graphene passes a subset of the LTP tests. New changes should not break currently passing LTP tests (and, ideally, might add new passing tests). LTP is currently only supported on the Linux PAL.

To run these tests:

cd LibOS/shim/test/ltp
make
make ltp.xml
# or
make SGX=1 ltp-sgx.xml
# or manually run the tool with options you need:
./runltp_xml.py -c ltp.cfg -v src/runtest/syscalls

Management Team

The current members of the management team are:

  • Michał Kowalczyk (Invisible Things Lab/Golem)
  • Dmitrii Kuvaiskii (Intel)
  • Borys Popławski (Invisible Things Lab/Golem)
  • Don Porter (UNC)
  • Chia-Che Tsai (Texas A&M University)
  • Rafał Wojdyła (Invisible Things Lab/Golem)
  • Mona Vij (Intel)
  • Isaku Yamahata (Intel)

The procedure for adding and removing maintainers

  • Joining: # of PRs submitted & merged + # of PRs reviewed + # of issues closed >= 20 (this means that a PR which fixes 3 issues counts as 4). Only complete and thorough reviews count.
  • Leaving: a member may be removed if not active or notoriously breaking rules from this document.
  • Additionally, at least 60% (rounded up) of current members have to agree to make any change to the team membership.