From 7c73943374239268b68237f5ca2959edca987cc2 Mon Sep 17 00:00:00 2001 From: Nicolas Denoyelle Date: Mon, 26 Jul 2021 15:48:26 -0500 Subject: [PATCH] [cleanup ] Improve contribution guide and repository organization. --- CONTRIBUTING.markdown | 345 ++++++++++++++++-- tests/Makefile.am | 8 +- tests/higher/{ => mapper}/test_mapper.c | 0 tests/{ => higher}/replicaset/common.c | 0 tests/{ => higher}/replicaset/test_hwloc.c | 0 .../{ => higher}/replicaset/test_replicaset.h | 0 6 files changed, 318 insertions(+), 35 deletions(-) rename tests/higher/{ => mapper}/test_mapper.c (100%) rename tests/{ => higher}/replicaset/common.c (100%) rename tests/{ => higher}/replicaset/test_hwloc.c (100%) rename tests/{ => higher}/replicaset/test_replicaset.h (100%) diff --git a/CONTRIBUTING.markdown b/CONTRIBUTING.markdown index 45d89107..1cc4b1d0 100644 --- a/CONTRIBUTING.markdown +++ b/CONTRIBUTING.markdown @@ -3,15 +3,27 @@ AML Contribution Guide {#contributing_page} Welcome to AML contribution guide. -This page walk through the main library expectations. When contributing to -AML, contributors should write features aligned with AML goal and AML spirit. +### Table Of Contents + +1. [General Guidelines](#guidelines) +2. [Coding Conventions](#conventions) +3. [Versioning](#versioning) +4. [Submission Workflow](#workflow) +5. [Testing](#testing) +6. [Documenting AML](#documentation) +7. [Check-List Before Submitting Code](#checklist) + +This page walks through the library expectations. When contributing to AML, +contributors should write features aligned with AML goal and AML spirit. Additionally, several good practices are requested, some being enforced through the continuous integration (CI) pipeline. For readability and maintainability, coding conventions need to be respected, code needs to be tested, documented and integrated with the main AML branch and its build tool-chain. -## General Guidelines +## General Guidelines + +### Building Blocks of AML At its core, AML is able to provide access to established, well known abstractions inside a common toolbox, with as much flexibility as possible. @@ -25,13 +37,13 @@ abstraction, the main abstraction structure containing a such a table and an opaque handle to store implementation specific data. ``` +struct aml_building_block_data; + struct aml_building_block_ops{ - aml_building_block_op0(); + type (*aml_building_block_op0)(struct aml_building_block_data *, args); ... }; -struct aml_building_block_data; - struct aml_building_block{ struct aml_building_block_ops *ops; struct aml_building_block_data *data; @@ -71,10 +83,78 @@ Usually a building blocks implementation would declare an exportable (`extern`) `struct aml_building_block_ops`, and `struct aml_building_block`, to provide users with a default static implementation of the block. -Finally, building blocks headers, sources and tests go to a dedicated folder -located respectively in `include`, `src` and `tests`. - -## Coding Convention +### AML Directory Organization + ++ [include](./include): + The headers directory. All library sources headers and installed headers + go here. + * [include/aml.h](./include/aml.h): + The main library header. + This header must include all building block definition and includes all + `utils` headers. + * [include/aml/](./include/aml/): + This directory includes abstraction implementation specific headers, + higher level abstraction headers and library utils headers. + * `include/aml//
.h`: + For each building blocks and there implementation, a header file is defined. + * [include/aml/utils](./include/aml/utils): + This directory contains library and user utility headers. + These headers must all be included in [include/aml.h](./include/aml.h). + - `include/aml/utils/
.h`: + Library util headers (non backend dependent). + - `include/aml/utils/backend/
.h`: + Library backend dependent headers. These header should be included with + appropriate macro guard as found in `include/aml/utils/features.h`. + * [include/aml/higher/](./include/aml/higher/): + Directory for aml high level features that are not building blocks. + - `include/aml/higher/.h`: + Higher level abstraction declares a generic high level interface. + - `include/aml/higher//
.h`: + Higher level abstraction declares an implementation of their high level + interface. ++ [src](./src): + The library sources directory. + * [src/aml.c](./src/aml.c): + Library initialization/cleanup source. + * `src//.c`: + Building blocks implementation. + * `src/backend/.c`: + Backends initialization/cleanup and related utility functions. + * `src/utils/.c`: + Implementation of library boilerplate code. + * `src//.c`: + Implementations of non building block abstractions. ++ [tests](./tests): + This directory contains the library unit tests triggered in the CI by + `make check`. + * `tests//test_.c`: + Building blocks implementation specific tests. + * `tests//test_.c`: + Generic building block tests usable by all/most implementation tests. + * `tests///test_.c`: + High level (non building blocks) implementation specific and generic tests. + * `tests/utils/test_.c`: + Utils tests. ++ [doc](./doc): + AML Documentation directory. + See ["Documenting AML"](#documentation) section for more information on how + to document AML properly. + * [doc/pages](./doc/pages): + Textual pages of aml main header [include/aml.h](./include/aml.h). + - `doc/pages/__api.rst` + Textual pages of each aml building block. + * [doc/tutorials](./doc/tutorials): + Code tutorials directory. + - `doc/tutorials//_.c`: + Solution source code of tutorials on how to use basic blocks. + - `doc/tutorials//_.trs`: + Matching textual exercises on how to use basic blocks. + - `doc/tutorials//_.c`: + Solution source code of tutorials on how to use higher level abstractions. + - `doc/tutorials//_.trs`: + Matching textual exercises on how to use higher level abstractions. + +## Coding Conventions AML code respects several coding conventions, most of them enforced in the CI pipeline using clang-format. @@ -88,9 +168,103 @@ The list of coding convention is the following: * curly braces are on the same line as the statement or function, * A new line must appear after curly braces. * macros are UPPERCASE_WITH_UNDERSCORES, function lowercase_with_underscores -* All functions/macros should start with aml/AML followed by the name of the building block involved. +* All functions/macros should start with aml/AML followed by the name of the +building block involved. + +### Editors Configuration + +#### Emacs + +Emacs can be configured to meet most formatting specified +in [`.clang-format`](./.clang-format) file by adding the following +configuration to your `.emacs`. + +``` +;; Definition of aml C style. +(c-add-style "aml" + '((c-basic-offset . 8) + (c-indent-tabs-mode . t) + (c-hanging-semi&comma-criteria (c-semi&comma-inside-parenlist)) + (c-hanging-colon-alist . + ((case-label . (after)) + (label . (after)) + (statement-cont) + (statement))) + (c-hanging-braces-alist . + ((defun-open . (before)) + (defun-open . (after)) + (defun-block-intro . (after)) + (statement-block-intro . (after)) + (arglist-cont-nonempty . (after)) + (arglist-intro . (after)) + (topmost-intro . (after)) + (statement . (after)) + (substatement . (after)) + (else-clause . (after)) + (brace-list-open . (after)) + (brace-list-close . (after)) + (block-open . (after)) + (block-close . (after)))) + (add-to-list 'c-cleanup-list 'brace-else-brace + 'brace-elseif-brace 'empty-defun-braces + 'defun-close-semi) + (c-offsets-alist . ((topmost-intro . 0) + (comment-intro . 0) + (inclass . +) + (case-label . 0) + (else-clause . 0) + (inextern-lang . 0) + (label . [0]) + (arglist-cont-nonempty . c-lineup-arglist) + (class-close . c-lineup-close-paren) + (defun-close . c-lineup-close-paren) + (brace-list-close . c-lineup-close-paren) + (block-open . 0) + (block-close . c-lineup-close-paren) + (defun-block-intro . +) + (defun-open . 0) + (statement-case-intro . +) + (statement-block-intro . +) + (statement . 0) + (brace-list-intro . +) + (brace-list-entry . 0) + (substatement-open . 0) + (substatement . +))))) + +;; Automatically set aml style when working with aml C files. +(defun aml-style () + (require 'cc-mode) + (when + (and buffer-file-name + (string-match "aml/" buffer-file-name)) + (c-set-style "aml"))) +(add-hook 'c-mode-hook 'aml-style) +``` + +However the best method to have CI compliant formatting is to install +the `clang-format` package and to use it to format your buffer. +Below sample code uses `use-package` package to load `clang-format` and +bind `C-x ` to format selected region. + +``` +(use-package clang-format + :commands + (clang-format-region clang-format-buffer) + :bind (("C-x " . 'clang-format-region))) +``` + +## Versioning -## Versioning and Submission Workflow +AML uses the [semver](https://semver.org/) versionning standard to version the +library. In a nutshell, AML versionning is composed of three non negative +itegers `..` increasing as more code gets merged into the master +branch. New features, bug fixes, performance and improvements shall raise +the `` number. Additions to the API which do not break the existing API +shall raise the `` number. Whereas modifications of the library that alter +existing API shall raise the `` number. AML do not define yet an ABI. +The version of AML can be modified in [configure.ac](./configure.ac). + +## Submission Workflow AML code is hosted on [github](https://github.com/anlsys/aml) and uses github actions as a CI. External collaborators (i.e. not members of the group at @@ -98,7 +272,8 @@ Argonne National Laboratory) should rely on forking and pull requests from outside the main repository. Internal members should push to new branches from master and use pull requests to merge their work in it. -We previously followed the [Gitlab Flow](https://docs.gitlab.com/ee/workflow/gitlab_flow.html) +We previously followed the +[Gitlab Flow](https://docs.gitlab.com/ee/workflow/gitlab_flow.html) models, so the git history might still look like it if you look at previous versions. We now follow a simplified process, but not the popular git-flow. Basically: @@ -160,25 +335,48 @@ the `LICENSE` file) and that you agree to the DCO. To signoff commits, use: `git commit --signoff`. To signoff a branch after the fact: `git rebase --signoff` -## Testing +## Testing The AML project includes a set of unit and integration tests. -Each AML building block has its own set of tests in a separate directory. -Tests are launched together with automake test suite with the `make check` command. -Building blocks should be tested as thoroughly as possible and whenever -relevant, integration tests with other components should be available. -Tests compilation and launch are implemented together in `tests/Makefile.am`. -The CI pipeline must be successful for a merge request to be accepted. -## Documentation +### Unit tests: + +AML unit tests are located in the [tests](./tests) directory. +Each AML building block has its own set of tests in a separate directory +(`tests//test_.c`). The same apply to utils +(`tests//test_.c`) and high level abstractions +(`tests//test_.c`). Any building block implementation, +high level abstraction implementation, utils, etc. should be tested with +a unit test. Tests are launched together with automake test suite with +the `make check` command. Building blocks should be tested as thoroughly as +possible and whenever relevant, integration tests with other components should +be available. Tests compilation and launch are implemented together +in `tests/Makefile.am`. + +### Integration Tests + +* Tutorials: +AML Tutorials are linked with the compiled library and run basic library uses. +Tutorial should be implemented at least for building blocks and high level +abstractions. These tests also run with the `make check` tests and the CI. + +* XSBench: +Some AML features have been integrated in the +[XSBench](https://github.com/ANL-CESAR/XSBench) proxy application and the +application itself is built and ran in a Continuous Integration check. + + +**The CI pipeline must be successful for a merge request to be accepted.** + +## Documenting AML For obvious reasons, the code and building block must be documented. -The whole documentation is based doxygen tool. +The whole documentation is based on doxygen tool. All the public code (data structures, functions, macro, variables) must be -documented as precisely as possible. Additionally, new building blocks implementation -must create a group (with `\@defgroup`) and provide a detailed description of -the features it provides. Syntax for code documentation can be found -[here](http://www.doxygen.nl/manual/docblocks.html), +documented as precisely as possible. Additionally, new building blocks +implementation must create a group (with `\@defgroup`) and provide a detailed +description of the features it provides. Syntax for code documentation can be +found [here](http://www.doxygen.nl/manual/docblocks.html), and most useful elements can be found in the example below. ``` @@ -246,9 +444,94 @@ int aml_building_block_create(struct aml_building_block **bb, ...); ``` If the contribution is an implementation of a building block and header has -been placed in `include/aml/building_block` then its documentation will automatically added to -the project documentation. However, it is necessary to point to this particular group -with `@ref` or `@see` command in the [main aml header](\ref aml.h). -If the contribution creates new include directories, the latter have to be included -in `doc/aml.doxy` after the field `INPUT`. +been placed in `include/aml//` then its documentation will +automatically added to the project documentation. However, it is necessary to +point to this particular group with `@ref` or `@see` command in +the [main aml header](\ref aml.h). If the contribution creates new include +directories, the latter have to be included in `doc/aml.doxy` after the field +`INPUT`. + +## Before Submitting Code + +The checklist below should solve most of the submission issues related to +automatic testing and enforcement of contribution guidelines. + +* Rebase your work on master branch, +* Squash your branch and split it in one or several commits representing +logical units of work. Do not include formatting of code you did not intend to +modify in your pull-request because it will clutter the diff and make the review +cumbersome. +* Make sure the license is included in all the new source files. +This can be automated as follow: +``` +FILES=$(git ls-files | grep -E ".*\.c$|.*\.h$") +LICENSE_FILE=SPDX.txt +cat > $LICENSE_FILE <=max{exit}END{print c}' $LICENSE_FILE $f) + if [ "$match" != "$license_num_lines" ]; then + head -n ${license_num_lines} $f + printf "\tFix %s ? (y/n): " $f + read ans + if [ "$ans" == "y" ]; then + cp $f $f.tmp + echo "/*******************************************************************************" > $f + cat $LICENSE_FILE >> $f + echo " ******************************************************************************/" >> $f + echo "" >> $f + cat $f.tmp >> $f + rm $f.tmp + fi + fi +done +rm $LICENSE_FILE +``` + +* Make sure all the new code (only) is properly formatted. +This can be automated as follow: +``` +FILES=$(git ls-files | grep -E ".*\.c$|.*\.h$") +diff_file=clang-format-diff +target=$(git rev-parse master) + +for f in ${FILES[@]}; do + rm -f $diff_file + git-clang-format --quiet --diff $target -- $f > $diff_file + lint=$(grep -v --color=never "no modified files to format" $diff_file || true) + rm -f $diff_file + + if [ ! -z "$lint" ]; then + printf "Fix $f ? (y/n): " + read ans + if [ "$ans" == "y" ]; then + git-clang-format --patch --force $target -- $f + fi + fi +done +``` +* Make sure your code compiles with extra error checking: +``` +./configure CC=gcc +make CFLAGS="-Wextra -Wall -Werror -pedantic" +``` + +* Make sure your code is tested and existing and new unit-tests run and pass: +``` +make CFLAGS="-Wextra -Wall -Werror -pedantic" check +``` + +* Make sure your code is documented the documentation builds without errors: +``` +make html +``` diff --git a/tests/Makefile.am b/tests/Makefile.am index 91e64424..1dcadd37 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -49,8 +49,8 @@ libdma_test_la_SOURCES = \ dma/test_dma.c libreplicaset_test_la_SOURCES = \ - replicaset/test_replicaset.h \ - replicaset/common.c + higher/replicaset/test_replicaset.h \ + higher/replicaset/common.c LDADD = libdma_test.la \ liblayout_test.la \ @@ -67,7 +67,7 @@ TILING_TESTS = \ DMA_TESTS = dma/test_linux -HIGHER_TESTS = higher/test_mapper +HIGHER_TESTS = higher/mapper/test_mapper if HAVE_OPENCL AREA_TESTS += area/test_opencl @@ -80,7 +80,7 @@ endif if HAVE_HWLOC AREA_TESTS += area/test_hwloc -HIGHER_TESTS += replicaset/test_hwloc +HIGHER_TESTS += higher/replicaset/test_hwloc endif # unit tests diff --git a/tests/higher/test_mapper.c b/tests/higher/mapper/test_mapper.c similarity index 100% rename from tests/higher/test_mapper.c rename to tests/higher/mapper/test_mapper.c diff --git a/tests/replicaset/common.c b/tests/higher/replicaset/common.c similarity index 100% rename from tests/replicaset/common.c rename to tests/higher/replicaset/common.c diff --git a/tests/replicaset/test_hwloc.c b/tests/higher/replicaset/test_hwloc.c similarity index 100% rename from tests/replicaset/test_hwloc.c rename to tests/higher/replicaset/test_hwloc.c diff --git a/tests/replicaset/test_replicaset.h b/tests/higher/replicaset/test_replicaset.h similarity index 100% rename from tests/replicaset/test_replicaset.h rename to tests/higher/replicaset/test_replicaset.h