Skip to content

Commit

Permalink
contributing: Add contributing intructions
Browse files Browse the repository at this point in the history
Add detailed contributing instructions from the old docs:
* Licensing
* Testing
* Review Process

Signed-off-by: Stefan Jumarea <[email protected]>
  • Loading branch information
StefanJum committed Sep 23, 2023
1 parent 59c4706 commit fb2ee38
Show file tree
Hide file tree
Showing 7 changed files with 329 additions and 0 deletions.
12 changes: 12 additions & 0 deletions configs/contributing.sidebar.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,18 @@
"title": "Unikraft",
"path": "/docs/contributing/unikraft"
},
{
"title": "Review Process",
"path": "/docs/contributing/review-process"
},
{
"title": "Testing Changes",
"path": "/docs/contributing/testing"
},
{
"title": "Licensing",
"path": "/docs/contributing/licensing"
},
{
"title": "Coding Conventions",
"path": "/docs/contributing/coding-conventions"
Expand Down
93 changes: 93 additions & 0 deletions content/docs/contributing/licensing.mdx
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
---
title: Licensing
description: |
Learn how Unikraft is licenced and what you should have in mind when contributing.
---

## Unikraft License

Unikraft is licensed under [3-Clause BSD license](https://opensource.org/licenses/BSD-3-Clause) (SPDX short identifier: `BSD-3-Clause`), a permissive license which allows for re-use with modification provided that the license header remains.
This ensures that copyright and authorship remain in-tact and credit is given where credit is due, but allows Unikraft's code to be re-used for other purposes.
This license enables the wider community to reuse Unikraft source code for any use case they desire, provided authorship notice is preserved.

### Copyright Notices

Unikraft is organized into libraries where each might be individually licensed.
In general, each source file should declare who is the copyright owner and under which terms and conditions the code is licensed.
The main license of the project is the following `BSD-3-clause`s.
It applies in particular to source code files that do not declare a license and where there is no license information file (e.g., files `LICENSE`, `COPYING`) placed in the same or corresponding `root` folder.

```txt
Copyright (c) YYYY, Copyright Holder.
All rights reserved.
Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions
are met:
1. Redistributions of source code must retain the above copyright
notice, this list of conditions and the following disclaimer.
2. Redistributions in binary form must reproduce the above copyright
notice, this list of conditions and the following disclaimer in the
documentation and/or other materials provided with the distribution.
3. Neither the name of the copyright holder nor the names of its
contributors may be used to endorse or promote products derived from
this software without specific prior written permission.
THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
POSSIBILITY OF SUCH DAMAGE.
```

In order to simplify working with license check tools, we add SPDX License Identifiers to the source files.
For C source or header files, this appears as follows:

```c
/* SPDX-License-Identifier: BSD-3-Clause */
/**
* Authors: ...
*
* Copyright (c) ...
*
* Redistribution...
*/
```

For Makefiles, Python files, and other files which use a `#` symbol for commenting, this appears as follows:

```Makefile
# SPDX-License-Identifier: BSD-3-Clause
#
# Authors: ...
#
# Copyright (c) ...
#
# Redistribution...
```

## Use of GPL Code

Due to license incompatibility, code using GPL (General Public License) can not be added to the Unikraft code base.
Where a BSD licensed version is available, that is preferred.
In case there is no option of BSD-licensed code, the GPL code must be reimplemented and licensed under BSD-3-Clause.

<Warning>
This means that using code from the Linux kernel is **not allowed**.
</Warning>

## External Libraries

Unikraft makes extensive use of external libraries to facilitate the runtime of an application.
External libraries are to be "ported" on top of Unikraft: being able to be built using Unikraft core components.

Certain incompatibilities are to be fixed by patching the upstream version of the library.

A Unikraft library repository will consist of build scripts / recipes and patches, together with required files to build the library.
The upstream version of the library itself will not be part of the repository, rather it will be referenced with a URL.

As with Unikraft components, external libraries need to be implemented using a BSD-compatible license.
166 changes: 166 additions & 0 deletions content/docs/contributing/review-process.mdx
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
---
title: Review Process
description: |
Learn how changes to the Unikraft core or to any official Unikraft micro-library repository are reviewed before they are merged.
---

## Unikraft Code Review Process

Pull Requests (PRs) submitted to the Unikraft core or to any official Unikraft micro-library repositories on Github will go through a rigorous code review process before they are accepted and merged.
This is done to ensure the following goals are met:

* **Consistency**:
Unikraft respects idomatic, consise, clear, and easy to read code.
This makes understanding code easier for new-commers at any expertise level and also makes debugging easier.
To achieve this, code in Unikraft is checked with a built-in linter program called [`checkpatch.uk`](/docs/contributing/testing#using-checkpatch).
This program was derived from the [Linux kernel](https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl).
* **Validity**:
Changes to Unikraft must be valid in that they must solve a specific problem and the way in which it is solved is considered a good or best approach.
* **Efficiency**:
Unikraft aims to be highly performant, as a result, new code introduced into Unikraft must itself be efficient.
Often-times this means that even if the new code is working, the reviewer could and should require changes if the same functionality can be achieved in a more efficient way (regarding both time and memory).
* **Safety & security**:
Unikraft's core code is kernel code and thus acts as a mission critical layer for the runtime of applications which are compiled against it.
To prevent disruption to the operation of application code and to continue on the security themes which Unikraft addresses, all code is checked rigorously.
* **Integrity**:
Authorship, a Developer's Certificate of Origin, and auditing are important to attribution and copyright.

This process can take some time but it ensures the stability and integrity of Unikraft.
More often than not, PRs must either be rebased, updated or undergo some change before they are merged.
This is normal and ensures fixes, new features and anything else introduced into Unikraft ecosystem meet the goals listed above.
This is more likely to occur with new features which will often go through multiple rounds or versions with a maintainer or "sheppard".
If you are a first-time contributor, please do not be discouraged by this lengthy process or additional feedback.
This is mentioned now to prevent any suprises regarding the review-process.
Feedback is provided in good spirit, and often-times allows for all parties to be properly informed with the best solution to a given problem.

On this page, we detail how the process of a review of a PR occurs for both those who wish to make new contributions to the core, or any additional micro-library component, as well as those who conduct the reviews themselves.

<Info>
**Reviewers, before you start a review, please:**

* Be polite, considerate, and helpful.
* Comment on positive aspects of PRs as well as changes.
* Be empathetic and mindful of how your review may be received.
* Assume good intent and ask clarifying questions.
* Experienced reviewers, consider pairing with new reviewers whose work requires extensive changes.
</Info>

### Stages of the code review process

1. After [a new PR is submitted](/docs/contributing/unikraft), the first thing to occur are a number of automatic checks and new builds of the branch-to-be-merged from Unikraft's CI/CD system.
One of the first operations to occur is autolabelling.

1. The operation to occur is the auto-assignment of a reviewer and maintainer (or "sheppard") to the PR.

* The job of the **reviewer** is to comb through the request for change.
They will be providing feedback, checking the functionality locally, performing additional spellchecking, and generarlly advising the PR with regard to the goals mentioned above.
* Theh job of the **maintainer** is to sheppard the PR into a mergable state, [see below](/docs/contributing/review-process#approval-process).

<Image
src="/images/unikraft-bot-assignments.png"
class="w-auto mx-auto"
title="Automated request for review and sheppard assignment"
description="A screenshot from a PR showing the unikraft-bot automatically assigning a reviewer and a maintainer to a PR."
position="center"
/>

<Info>
Both the reviewer and the assignee (a.k.a. maintainer) are derived automatically [based on their workload](https://github.com/unikraft/governance/blob/8442d5b8e36179e4df5fe34e25dbba7aa3fedf8d/cmd/sync_pr.go#L540-L556) (i.e. the number of other reviews they have to do) and their affiliation with a Special Interest Group which oversees the area of change the PR affects.
</Info>

1. The next automatic operation to occur is the running of the [`checkpatch.uk`](/docs/contributing/testing#using-checkpatch) program against the PR's branch applied on top of the `staging` the repository in question to ensure each commit meets relevant style, consistency and validity requirements.

<Image
class="w-auto mx-auto"
src="/images/unikraft-bot-checkpatch.png"
title="Automated checkpatch"
description="An screenshot of a summary from the automatic checkpatch performed on a new PR, this particular checkpatch looks good!"
/>

If the checkpatch has at all failed, the comment from [unikraft-bot](https://github.com/unikraft-bot) will provide a truncated summary.
In this case, the PR must be rebased with changes which meet the requirements of the checkpatch.
For new contributors, please run the `checkpatch.uk` program before creating the PR to streamline the review process.
A reviewer will typically request for a rebase with the recommendations from the checkpatch before continuing with their review.

1. Along with the `checkpatch.uk`, a number of consistency builds are run in parallel for known architectures and platforms against the [helloworld](https://github.com/unikraft/app-helloworld) Unikraft application with the branch of the PR.
These checks will appear at the bottom of the PR, like so:

<Image
class="w-auto mx-auto"
src="/images/unikraft-ci-passed.png"
title="Automated unikernel builds"
description="Every PR has all the targets of the helloworld application built aganst it ontop of the `staging` branch of the Unikraft core repository."
position="center"
/>

1. At this point it is up to the reviewer to comb through the requested change to the repository.
This can be, for example, accomplished by testing the changes locally with [`kraft`](/docs/cli) and by checking out the PR using the [Github CLI](https://github.com/cli/cli) in the relevant directory of a fresh clone of related Unikraft repositories at the `staging` branch:

If the PR is a change to the [Unikraft core repository](https://github.com/unikraft/unikraft), then you can simply update the branch of the local clone from the initialization step detailed above:

```console
$ cd /tmp/unikraft-pr-$PR_ID/.unikraft/unikraft
$ gh pr checkout $PR_ID
```

If the change is to a separate micro-library component, please clone the relevant library or initialize the relevant application.
Once the PR's code has been applied on top of the `staging` branch of the relevant repository, proceed by configuring the unikernel with appropriate build options, compiling and then running.

1. Detailed comments, general feedback, request for changes or approvals of the PR are done via [Github's PR review manager](https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request>).
Generally you can press the "Files changed" tab of a PR which reveals a diff where you can use to start your review.
On this page, you can leave in-line comments.

<Warning>
### On receiving feedback and changes

When a PR has had changes requested to it, it is important that the changes are made as part of a rebase and then [force pushing](https://stackoverflow.com/a/12610763) to the original branch.
Simply applying the changes ontop of the previous commits is not sufficient.
If a force push or changes to the branch will re-trigger the review process (see 1, 2 and 3).
</Warning>

### Reviewer checklist

A reviewer of a PR should generally check the following items with regard to the PR in question:

* [ ] Does the PR's checkpatch return all clear?
Some commits may have a warning instead of an error, can the warning be safely ignored?
* [ ] Do all the tests pass?
* [ ] Is the solution the most efficient and secure?
* [ ] Are there clear comments and [inline documentation](/docs/contributing/coding-conventions/#comments)?
* [ ] Is the code copied or does it include [non-BSD code](/docs/contributing/licensing)?
* [ ] Does this work locally for you?

### Finishing a review

The review may go through a back-and-forth between the authors and the reviewer before the reviewer marks the PR as approved.
As a reviewer, when you consider the process complete you must add an equivilant ["sign off"](/docs/contributing/unikraft#commits) in the same way that the author has.
This is done by adding a "Reviewed-by" Git trailer like so:

```text
Reviewed-by: Your Name <[email protected]>
```

To do this, you can navigate to the "Files changed" tab of a PR which reveals a diff and access the review box.
Then simply select one of the available requests and add along with your comments the trailer.

Adding a `Reviewed-by` tag is important to the CI/CD system, as it signals that a review has been completed which is the first lock of the merge process.

## Approval process

The approval process of a PR is the final step before a PR is merged and is performed by a maintainer or sheppard.
Maintainers are also auto assigned by [unikraft-bot](https://github.com/unikraft-bot) and will check both the PR and the review to ensure consistency with the goals mentioned above as well as this very review process.

### Approving a PR

Much like providing a review to a PR, an approval must be made when the PR is absolutely ready to be merged.
An approval will unlock the CI/CD system and will automatically merge the PR into the desired repository.

An approval must use Github's review tool and mark the PR as state "approved" this can be done in the same way as the review in the figure above, or alternatively it can be done with GitHub's CLI companion tool like so:

```console
$ gh pr review https://github.com/unikraft/app-nginx/pull/2 \
--approve \
--body "Approved-by: Your Name <[email protected]>"
```

The only difference is the trailer, which should be `Approved-by`.
58 changes: 58 additions & 0 deletions content/docs/contributing/testing.mdx
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
---
title: Testing Changes
description: |
Learn how to test the Unikraft changes before submitting a new Pull Request.
---

## Testing Changes Before Submitting a New PR

### Testing your changes

Make sure that you tested your changes on various setups before opening a new PR.
Try several different configuration options (in particular multiple architectures and platforms) and library combinations.

During development, disable `CONFIG_OPTIMIZE_DEADELIM` (Found in themenuconfig via "Build Options" -> "Drop unused functions and data") so that all of your code is covered by the compiler and linker.

### Using `checkpatch.uk`

To support you in checking your coding style, we provide a tailored version of the Linux kernel's `checkpatch.pl` script in [`support/scripts/checkpatch.uk`](https://github.com/unikraft/unikraft/blob/staging/support/scripts/checkpatch.uk).
You should run this from the `root` of the Unikraft repository because it contains a [`.checkpatch.conf`](https://github.com/unikraft/unikraft/blob/staging/.checkpatch.conf) file which disables some tests that we consider irrelevant for Unikraft.

Please run this script on all commits you are about to submit.
To do this, create a patch file for each commit on your working branch by running `git format-patch`.
For example, to create patch files for the last 5 commits:

```console
$ git format-patch HEAD~5
$ ./support/scripts/checkpatch.uk ./000*
```

You can also run the `checkpatch` on one or multiple files before committing by using the `-f` option.
Furthermore, some errors can be automatically solved by using the `--fix-inplace` option:

```console
$ ./support/scripts/checkpatch.uk --no-tree -f --fix-inplace <path/to/the/file>
```

For example, if you have some changes in the `lib/vfscore/vfs.h`, you can check and fix potential coding style issue with:

```console
$ ./support/scripts/checkpatch.uk --no-tree --fix-inplace -f lib/vfscore/vfs.h
ERROR: "foo*bar" should be "foo *bar"
#576: FILE: lib/vfscore/vfs.h:576:
+intsys_chmod(const char*path, mode_t mode);

total: 1 errors, 0 warnings, 828 lines checked
[...]

$ ./support/scripts/checkpatch.uk --no-tree --fix-inplace -f lib/vfscore/vfs.h
total: 0 errors, 0 warnings, 828 lines checked

lib/vfscore/vfs.h has no obvious style problems and is ready for submission.
```

<Warning>
Note that not all the coding style errors can be solved automatically and some of them should be addressed by hand.
</Warning>

Please attempt fixes for all errors and warnings; if you decide to ignore some, be prepared to have a good reason for each warning and error during the [review process](/docs/contributing/review-process).
Binary file added public/images/unikraft-bot-assignments.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added public/images/unikraft-bot-checkpatch.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added public/images/unikraft-ci-passed.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.

0 comments on commit fb2ee38

Please sign in to comment.