diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 73886f3f7e..7b2f081988 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -5,22 +5,26 @@ We want to make contributing to this project as easy and transparent as possible Please refer to the steps described on our website: [How to contribute to OAI](https://www.openairinterface.org/?page_id=112). 1. Sign and return a Contributor License Agreement to OAI team. -2. Register on [Eurecom GitLab Server](https://gitlab.eurecom.fr/users/sign_up) if you do not have any. +2. Register on [Eurecom GitLab Server](https://gitlab.eurecom.fr/users/sign_up) + if you do not have any. - We recommend that you register with a professional or student email address. - - If your email domain (`@domain.com`) is not whitelisted, please contact us (mailto:contact@openairinterface.org). + - If your email domain (`@domain.com`) is not whitelisted, please contact us + (mailto:contact@openairinterface.org). - Eurecom GitLab does NOT accept public email domains. -3. Provide the OAI team with the **username** of this account to (mailto:contact@openairinterface.org) ; we will give you the developer rights on this repository. -4. The policies are described in these wiki pages: [OAI Policies](https://gitlab.eurecom.fr/oai/openairinterface5g/wikis/oai-policies-home). - - PLEASE DO NOT FORK the OAI repository on your own Eurecom GitLab account. It just eats up space on our servers. - - You can fork onto another hosting system. But we will NOT accept a merge request from a forked repository. +3. Provide the OAI team with the **username** of this account to + (mailto:contact@openairinterface.org) ; we will give you the developer + rights on this repository. +4. The contributing policies are described in the [corresponding documentation + page](doc/code-style-contrib.md). + - PLEASE DO NOT FORK the OAI repository on your own Eurecom GitLab account. + It just eats up space on our servers. + - You can fork onto another hosting system. But we will NOT accept a merge + request from a forked repository. * This decision was made for the license reasons. * The Continuous Integration will reject your merge request. - - All merge requests SHALL have `develop` branch as target branch. -## Coding Styles ## +# License # -There are described [here](https://gitlab.eurecom.fr/oai/openairinterface5g/wikis/guidelines/guidelines-home). - -## License ## - -By contributing to OpenAirInterface, you agree that your contributions will be licensed under the LICENSE file in the root directory of this source tree. +By contributing to OpenAirInterface, you agree that your contributions will be +licensed under the license described in the file [`LICENSE`](./LICENSE) in the +root directory of this source tree. diff --git a/doc/README.md b/doc/README.md index 9789726fc8..ef72249725 100644 --- a/doc/README.md +++ b/doc/README.md @@ -19,6 +19,7 @@ - [FEATURE_SET.md](./FEATURE_SET.md): lists supported features - [GET_SOURCES.md](./GET_SOURCES.md): how to download the sources - [BUILD.md](./BUILD.md): how to build the sources +- [code-style-contrib.md](./code-style-contrib.md): overall working practices, code style, and review process - [cross-compile.md](./cross-compile.md): how to cross-compile OAI for ARM - [clang-format.md](./clang-format.md): how to format the code - [sanitizers.md](./dev_tools/sanitizers.md): how to run with ASan/UBSan/MemSAN/TSan @@ -51,9 +52,11 @@ There is some general information in the [OpenAirInterface Gitlab Wiki](https:// - [How to run with E2 agent](../openair2/E2AP/README.md) Legacy unmaintained files: -- `L2NFAPI_NOS1.md`, `L2NFAPI_S1.md`: old L2simulator, not valid anymore -- `SystemX-tutorial-design.md` -- `UL_MIMO.txt` +- [`L2NFAPI_NOS1.md`](./L2NFAPI_NOS1.md), [`L2NFAPI_S1.md`](./L2NFAPI_S1.md): + old L2simulator, not valid anymore +- [`SystemX-tutorial-design.md`](./SystemX-tutorial-design.md): old, high-level + documentation +- [`UL_MIMO.txt`](./UL_MIMO.txt): UL-MIMO specific notes # Designs @@ -65,7 +68,7 @@ Legacy unmaintained files: - [L1 threads in NR-UE](./nr-ue-design.md) Legacy unmaintained files: -- 5Gnas.md +- [`5Gnas.md`](./5Gnas.md) # Building and running from images @@ -73,7 +76,7 @@ Legacy unmaintained files: - [How to run 5G with the RFsimulator from images](../ci-scripts/yaml_files/5g_rfsimulator/README.md) - [How to run 4G with the RFsimulator from images](../ci-scripts/yaml_files/4g_rfsimulator_fdd_05MHz/README.md) - [How to run 5G with the L2simulator from images](../ci-scripts/yaml_files/5g_l2sim_tdd/README.md) -- [How to run images in OpenShift](../openshift/README.md) +- [How to run physical simulators in OpenShift](../openshift/README.md) # Libraries @@ -87,7 +90,7 @@ Legacy unmaintained files: - The [threadpool](../common/utils/threadPool/thread-pool.md) used in L1 - The [LDPC implementation](../openair1/PHY/CODING/DOC/LDPCImplementation.md) is a shared library -## SDRs +## Radios Some directories under `radio` contain READMEs: diff --git a/doc/code-style-contrib.md b/doc/code-style-contrib.md new file mode 100644 index 0000000000..e1966a2510 --- /dev/null +++ b/doc/code-style-contrib.md @@ -0,0 +1,263 @@ +This document lays out the basic contribution policies for developers. It +describes the generel workflow, describes some coding rules, and how code +review is performed. + +[[_TOC_]] + +# General + +OpenAirInterface employs both human review and automated CI tests to judge +whether a code contribution is ready to be merged. + +The contributor has to sign a contributor license agreement (CLA) as described +in [`CONTRIBUTING.md`](../CONTRIBUTING.md). After creating an account on the +Eurecom Gitlab, the contributor can open a merge request: he becomes the +"author" of such code contribution. A senior OAI member will review this work, +and make suggestions for possible improvements. Each week, we discuss the +progress of the merge requests in a [weekly external developer +call](https://gitlab.eurecom.fr/oai/openairinterface5g/-/wikis/OpenAirDevMeetings), +and discuss which merge requests can be merged. + +The CI consists in various Jenkins pipelines that run on each merge request. +See [`TESTBenches.md`](./TESTBenches.md) for more details about the CI setup. + +There is the official [Gitlab Help](https://docs.gitlab.com/) that can help you +with any questions regarding Gitlab. We recommend reading the [Git +Book](https://git-scm.com/book/en/v2) to use Git properly. + +# Basic coding rules + +You should respect the `.clang-format` file in the root of the repository. The +`clang-format` tool will pick up this file when being applied to code in the +repository. Please also refer to the [corresponding +documentation](./clang-format.md). + +A number of high-level comments: + +- Indentation is two spaces, no tabs; try to limit the number of indentations. +- Line length is 132, not more than one statement per line; no whitespace at + the end of lines +- The opening brace after a function is on a new line; after control flow + statements (`if`, `while`, `switch`, ...), it is on the same line +- Pointer or reference operators (`*`, `&`) are right-aligned +- Do not commit code that is commented out +- Use strong typing (no `void *`, use complex data types such as `c16_t` over + `uint32_t` in L1, ...) +- Do not use [magic numbers](https://en.wikipedia.org/wiki/Magic_number_(programming)#Unnamed_numerical_constants) + for unnamed numerical constants and do not hardcode values +- Don't cast the result of `malloc()`: it is not needed, and can lead to bugs. +- Use `AssertFatal()` and `DevAssert()` to check for invariants, not for error + handling: Assertions are for preventing bugs (e.g., unforeseen state), + not to sanitize input. +- Use `const` on pointer function arguments that are input to that function; + put output variables (via a pointer) last +- Do not do premature optimization; measure the code before writing SIMD + instructions by hand, and measure again to show it is faster. + +If in doubt, check out code that has been recently written (e.g., use the merge +requests page to check for code that has recently been added) and follow that +style. Checking surrounding code is usually not the best idea, as OAI has a +long history in which coding rules were not really enforced. + +There is an old [OAI coding guidelines +document](https://gitlab.eurecom.fr/oai/openairinterface5g/-/wikis/documents/openair_coding_guidelines_v0.3.pdf) +that might be useful; if this document and `.clang-format` contradict, +`.clang-format` takes precedence. + +# Main Workflow and Versioning + +## Workflow + +You should be familiar with git branching, merging, and rebasing. + +The target branch for every contribution, and the general development branch, +is `develop`. Typically every week, we collect multiple merge requests in an +"integration branch" that gets tested by the CI individually. If everything is +fine, we merge to develop and tag it `YYYY.wXX` with `YYYY` the current year, +and `XX` the week number. + +Note that the above includes a lot of merging, making the Git history difficult +to understand. To not needlessly increase the complexity, please keep your +branch history linear (i.e., no merges). Each commit should be able to +compile, and ideally be able to run an End-to-End test of gNB/UE using RFsim. +This can be achieved by making each commit a **logical change** to be applied to +the code base, which also facilitates the review of your changes. The Linux +kernel has some [documentation on what a logical change +is](https://www.kernel.org/doc/html/latest/process/submitting-patches.html#separate-your-changes). +From a practical point of view, this means that your history should not have +commits that "clean up" a previous commit (indicated by commit messages such as +`Fix bug` or `Review addressed`). They don't describe what the fix is about, +and make review more difficult because the changes are not self-contained, but +spread across commits, incurring mental overhead. Instead, the commit series +should be written from the first commit as if you knew how the final code looks +like. In other words, you should guide the reader of your commits. This includes +that every commit message describes _why_ a particular change is necessary and +correct(!). Note that the rule of making commits small still applies! In +summary: + +- Make logical changes, which are **small, self-contained** commits, +- Don't fix up changes later: **rewrite the history** to guide the + reader/reviewer, and +- In the commit message, **explain why** changes are necessary and correct. + +A commit message can (and often should) take several lines. One-line commit +messages should be reserved for very simple changes. If in doubt, prefer to +explain your work more than less. + +The workflow of the integration branch has its weaknesses; we might revise this +in the future towards a workflow using rebase to integrate work of different +people, in order to simplify the history, and allow the usage of tools such as +`git bisect` to search for bugs. + +After some time, we make a stable release. For this, we simply merge develop +into master, and give a semantic versioning number, e.g., `v1.1`. + +## How to manage your own branch + +Before starting to work, please make sure to branch off the latest `develop` +branch. Make commits as appropriate. +```bash +$ git fetch origin +$ git checkout develop +$ git checkout -b my-new-feature # name as appropriate +$ git add -p # add changes for change set 1, use `-p` to review what to include +$ git commit # in the editor, describe your changes +$ git add -p # add changes for change set 2 +$ git commit # in the editor, describe your changes +``` + +Again, commit message should take multiple lines; after the initial title, a +blank line should follow. Read the `DISCUSSION` section in `man git commit` for +more information. + +If your development takes longer, make sure to synchronize regularly with +`origin/develop` using `git rebase`: +```bash +$ git fetch origin +$ git rebase -i origin/develop +``` + +If you do logical changes, you should not have to resolve the same conflicts +over and over again. Note that if you jumped over multiple develop tags, you +can also rebase in intermediate steps, in case you fear the differences might +be too big. +``` +$ git rebase -i 2023.w38 +$ git rebase -i 2023.w41 +$ git rebase -i develop +``` + +Once you rebased, push the changes to the remote +``` +$ git push origin my-new-feature --force-with-lease # force with lease let's you only overwrite what you also have locally in origin/my-new-feature +``` + +# Merge Requests + +A merge request (MR) can be submitted as soon as the code is considered stable +and reviewable. The idea is to start the review early enough so that the code +author (the MR owner) can incorporate fixes while the reviewer is giving +feedback. Note that while it should not be common, a refusal of a merge request +is a valid outcome of a merge request review (subject to proper justification). + +When preparing a contribution that is large, the developer is responsible for +warning the OAI team, so that the review work can start as early as possible +and run in parallel to the contribution finalization. Failing to do so, there +is a risk that the work will take a long time to be merged or might even not be +merged at all if judged too complex by the OAI team. Also, note that big +contributions should be cut into small commits each containing a logical +change, as described above. Finally, as a rule of thumb, the smaller the merge +request, the easier it will be to review and merge. + +The reviewer comments on code changes ("open comments") that should be +addressed by the author. Most reviewers prefer to mark open comments as +resolved by themselves to double check the modifications and close such +comments. As an author, please don't resolve open comments (don't click the +"Resolve thread" button) unless explicitly instructed by the reviewer. + +Note that the _merge request author_ asks for inclusion of code, so _they +should make the review easy_; in particular, if facilitating review incurs +extra work to make a simpler code review (e.g., rewriting entire commits or +their order), this extra work is justified. This particularly(!) applies for +big merge requests. + +When opening a merge requests, the author should select `develop` as the target +branch, and add at least one of these labels when opening the merge request: + +- ~documentation: don't perform any testing stages, for documentation +- ~BUILD-ONLY: execute only build stages, for code improvements without impact + on 4G or 5G code +- ~4G-LTE: perform 4G tests +- ~5G-NR: perform 5G tests + +Failure to add a label will prevent the CI from running. You can add both +~4G-LTE and ~5G-NR together; if in doubt about the right label, add both. The +CI posts the results in the comments section of the merge request. Both merge +request authors and reviewers are responsible for manual inspection and +pre-filtering of the CI results. An overview of the CI tests is in +[`TESTBenches.md`](./TESTBenches.md). + +To communicate the review progress both between author and reviewer, as well as +to the outside world, we (ab-)use the milestones feature of Gitlab to track the +current progress. The milestone can be set when opening the merge request, and +during its lifetime in the sidebar on the right. Following options: + +- _no milestone_: not ready for review yet and is generally used to wait for a + first CI run that the author will inspect and fix problems detected by the CI + (please limit the time in which your code is in that phase) +- %REVIEW_CAN_START: the reviewer can start the review +- %REVIEW_IN_PROGRESS: the reviewer is currently doing review, and might + request changes to the code that the author should include (or refute with + justification) +- %REVIEW_COMPLETED_AND_APPROVED: the reviewer is happy with code changes + (*open comments still have to be addressed!*) +- %OK_TO_BE_MERGED: the OAI team plans to merge this; *do not push any changes + anymore at this point*. + +# Review Form + +The following is a check list that might be used by a reviewer to check that +code contribution fulfils minimum standard w.r.t. formatting, data types, +assertions, etc. The reviewer might copy/paste this form into a merge request, +or simply check that all have been filled. + +All points should be marked to complete a review. + +```md +### Review by @username + +- [ ] `.clang-format` respected +- [ ] No merges, i.e., the branch has a linear history; every commit compiles (and ideally runs in RFsim) +- [ ] For L1: uses complex data types. In general: prefers strong/adapted types/typedefs over `void`/generic `int`, or otherwise primitive types. +- [ ] Documentation updated (doxygen summary of functions; in `doc/`, or the corresponding folder; `FEATURE_SET.md`) +- [ ] Uses assertions were appropriate +- [ ] No commented/dead code (or such code has been removed), no function duplication +- [ ] No magic numbers: use defines, enums, and variables to make the meaning of a number clear. +- [ ] The changes don't have patch noise (no unnecessary whitespace changes unrelevant to the changed code; reformatting is ok) +- [ ] No unnecessary/excessive logs introduced. Prefer LOG_D for frequent logs + +Additional remarks, if applicable: +``` + +Additional optional questions in case they apply: +- Has a new tool/dependency been introduced? Needs to be discussed if to be added. +- Is a new CI test necessary? Can it be done in simulators? + +# Reporting bugs + +Please report only true bugs in the [issue tracker](../../issues). Do not +report general user problems; use the [mailing +lists](https://gitlab.eurecom.fr/oai/openairinterface5g/-/wikis/MailingList) +instead. If in doubt, prefer the mailing lists and if needed and requested by +the OAI team, an issue will be opened. + +When reporting a bug, please clearly +- explain the problem, +- note what you expected to happen (what should happen), +- show what happens instead (what did happen), and +- give steps on how to reproduce (including, if needed, configuration files and + command lines). + +You are encouraged to use these bullet points to structure your issue for easy +understanding. Use code tags (the "insert code" button with symbol `` in +the gitlab editor) for logs and small code snippets.