Merge pull request #1398 from Iximiel/feature/sarif-codecheck

Adding gh integration to the codecheck workflow

Author: GiovanniBussi

.github/workflows/ci.yml

@@ -22,6 +22,9 @@ jobs:
   codecheck:
     uses: ./.github/workflows/codecheckWF.yml
     secrets: inherit
+    permissions:
+      contents: read
+      security-events: write
 
   mac:
     uses: ./.github/workflows/macWF.yml

.github/workflows/codecheckWF.yml

@@ -16,6 +16,9 @@ env:
 jobs:
 
   codecheck:
+    permissions:
+      contents: read
+      security-events: write
     runs-on: ubuntu-22.04
     steps:
     - uses: actions/checkout@v6
@@ -38,6 +41,7 @@ jobs:
       run: |
         make -j 4 -C astyle
     - name: Checking code
+      id: codecheck
       run: |
         # this is required so as to have all the include files in place
         # notice that this is done automatically in build
@@ -46,3 +50,15 @@ jobs:
         # then generate the links
         make -C src/lib/ dirslinks
         make codecheck
+    - name: Upload codecheck report to gh
+      if: failure() && steps.codecheck.outcome == 'failure'
+      uses: github/codeql-action/upload-sarif@v4
+      with:
+        sarif_file: src/codecheck.sarif
+        category: plumedcheck
+    - name: Upload ruff report to gh
+      if: failure() && steps.codecheck.outcome == 'failure'
+      uses: github/codeql-action/upload-sarif@v4
+      with:
+        sarif_file: src/ruff.sarif
+        category: ruff

src/maketools/codecheck

@@ -79,17 +79,21 @@ fi
     # "head" discards the recap and awk reformat the error message
       cd "${basedir}"
       #this uses the same check in the base Makefile
-      if which ruff &> /dev/null; then
+      if which ruff &>/dev/null; then
         ruff format --check \
           | grep 'Would reformat' \
           | awk '{ printf "[%s:0] (error) :ruff_format: python ruff formatting non satisfied\n", $3 }' \
             >&2
+        #human readable
         ruff check --output-format concise \
           | head -n -2 \
           | awk '{ printf "[%s] (error) :ruff_check: %s", substr($1,1,length($1)-1), $2;
                    $1=$2="";
                    print $0 }' \
             >&2
+        #machine readable
+        #ruff is "blazingly fast" and we have a few python files:
+        ruff check --output-format sarif -o src/ruff.sarif >&2 || true
       else
         #local error message
         echo "      codecheck did not found ruff:"
@@ -105,6 +109,7 @@ fi
     # we discard configurations introduced in small_vector because they are many and
     # get included in many files
     function docheck(){
+      # With cppcheck 2.16.1 and later you can get a report already in sarif format and put it on the CI alongside the old style of output
       # setting up a function to not repeat the arguments
       # shellcheck disable=SC2086
       cppcheck --std=c++17 -j 4 --platform=unix64 --language=c++ \
@@ -221,4 +226,20 @@ fi
 echo "++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++"
 echo
 
+#just a small check to see if python is there
+if [[ $do_local == false ]] && python --version &>/dev/null; then
+# --------------------------------------------------------------------
+#   This converst $FATAL_FILE to the sarif format,
+#   so that it can be integrated with the codeQL stuff in the CI
+#   SARIF is a fancy json with some specific fields
+#
+#   As now this converts only things in this form:
+#   [{file}:{line}] ({severity}) :{id}: {message}
+#   As now the errors in the format:
+#   [global_check] ({severity}) :{id}: {message}
+#   are ignored
+
+  python maketools/sarif.py "${FATAL_FILE}" >codecheck.sarif
+fi
+
 exit 1

src/maketools/plumedcheck

@@ -2,7 +2,7 @@
 # vim: ft=awk
 
 # DOC: \page Plumedcheck Validating syntax with plumedcheck
-# DOC: 
+# DOC:
 # DOC: Here is a list of coding rules that are enforced by the plumedcheck script.
 # DOC: These rules are necessary to be sure that the codebase is consistent.
 # DOC: Code not satisfying these rules will make the automatic test on travis-ci fail.
@@ -16,7 +16,7 @@
 
 TEMP=$(mktemp -t plumed.XXXXXX)
 
-cat > $TEMP << \EOF
+cat >$TEMP <<\EOF
 
 # print usage 
 function usage(){
@@ -246,9 +246,9 @@ BEGINFILE{
     s=system("diff -q " FILENAME " " tempfile ">/dev/null 2>/dev/null")
 # check if astyle has been applied correctly
 # DOC: :astyle:
+# DOC: The file has not been formatted correctly.
 # DOC: In order to keep the code readable, we use the astyle program to format
-# DOC: all source files. This error indicates that the reported file has not been
-# DOC: formatted correctly.  It is important that the version of astyle and the options
+# DOC: all source files. It is important that the version of astyle and the options
 # DOC: exactly match the ones we use for testing. To this aim, you should use
 # DOC: a command such as `make astyle` from the plumed root directory.
     if(s!=0) error("astyle","astyle not satisfied")
@@ -264,6 +264,7 @@ BEGINFILE{
     s=system("diff -q " configurefile " " tempfile ">/dev/null 2>/dev/null")
 # check if autoconf has been applied correctly
 # DOC: :autoconf:
+# DOC: `./configure` is not updated.
 # DOC: In our git repository we distribute both `./configure` and `./configure.ac` files.
 # DOC: When you modify the latter, the former should be regenerated using `autoconf` and
 # DOC: committed to git. This error indicates that the `./configure.ac` and `./configure` files
@@ -284,6 +285,7 @@ BEGINFILE{
     s=system("diff -q " plumed_c " " tempfile ">/dev/null 2>/dev/null")
 # check if cython has been applied correctly
 # DOC: :python_module:
+# DOC: `python/plumed.c` is not updated.
 # DOC: In our git repository we distribute a compiled python interface
 # DOC: as `plumed.c` and `plumed.pyx` files.
 # DOC: When you modify the latter, the former should be regenerated using `cython` and
@@ -320,6 +322,7 @@ BEGINFILE{
       sub("^# *include +\"\\.\\./","",included_module_name);
       sub("/.*$","",included_module_name);
 # DOC: :include_dots:
+# DOC: Dots (`..`) in and include statement.
 # DOC: It is considered an error to include a file using a path starting with `../dir`,
 # DOC: where `dir` is not the name of a module explicitly declared as used in the Makefile.
 # DOC: This might result in undetected dependences between the modules, which means that
@@ -360,13 +363,15 @@ BEGINFILE{
       sub(">.*$","",h)
 # check if deprecated headers are included
 # DOC: :deprecated_headers:
+# DOC: Use of C++ deprecated headers.
 # DOC: There is a list of headers that have been deprecated in C++ and should not be
 # DOC: included. These headers should be replaced with their equivalent from the stdlib.
 # DOC: E.g., use `#include <cstdlib>` instead of `#include <stdlib.h>`. Notice that
 # DOC: using the modern header all the functions are declared inside the `std` namespace.
 # DOC: Also notice that we have a special exception to allow them in Plumed.h.
       if(h in deprecated_headers && !(module in outer_modules)) error("deprecated_header","including deprecated header " h);
 # DOC: :external_header: (style)
+# DOC: Included external headers.
 # DOC: Header files should possibly not include other header files from external libraries.
 # DOC: Indeed, once PLUMED is installed, if paths are not set correctly the include files of the other libraries
 # DOC: might not be reachable anymore. This is only a stylistic warning now. Notice that
@@ -383,13 +388,15 @@ BEGINFILE{
 # check if a .inc file, which is temporary and not installed, is included in a header file.
 # this might create inconsistencies in the installed plumed header files.
 # DOC: :non_h_header:
+# DOC: `.inc` file included in header.
 # DOC: Files with `.inc` extension are generated by PLUMED while it is compiled and are
 # DOC: not installed. They should not be directly included in header files, otherwise
 # DOC: those header files could be not usable once PLUMED is installed.
       if(filetype=="header" && h!~"\\.h") error("non_h_header","including non '.h' file " h " in a header file");
       if(h~"^[A-Za-z0-9_][A-Za-z0-9_]*/") {
         sub("/.*$","",h)
 # DOC: :include_non_used_module:
+# DOC: Included a non used module.
 # DOC: When including a file in the form `"dir/file.h"`, `dir` should be an used module.
 # DOC: This makes sure that we do not include system files that by chance are named as PLUMED modules.
 # DOC: Indeed, when including `"file"` if the file is not found in the current path it is searched in
@@ -457,8 +464,8 @@ BEGINFILE{
     if(match($0,"^//[+]PLUMEDOC ")){
       doc=$NF;
 # DOC: :multi_doc:
-# DOC: An action or cltool should be documented once and only once. Notice that because of the
-# DOC: way the manual is generated, a cltool cannot have the same name of an action.
+# DOC: An action or cltool should be documented once and only once.
+# DOC: Notice that because of the way the manual is generated, a cltool cannot have the same name of an action.
       if(doc in plumed_doc) error("multi_doc","doc " doc " already at "plumed_doc[action]);
       plumed_doc[doc]=FILENAME ":" FNR;
       n=split($(NF-1),array,"_");
@@ -516,6 +523,7 @@ BEGINFILE{
 
 # check that "using namespace" statements are not used in header files
 # DOC: :using_namespace_in_header:
+# DOC: `using namespace` statement in header file.
 # DOC: A statement `using namespace` in header files is considered bad practice. Indeed, this would imply that
 # DOC: any other file including that header file would be using the same namespace.
 # DOC: This is true also for `std` namespace, and that's why you typically find full specifications
@@ -525,13 +533,15 @@ BEGINFILE{
     if(module_type[module]=="default-off") {
 # check that "using namespace" statements are not used in source files
 # DOC: :using_namespace_std_in_source:
+# DOC: `using namespace std` in source file.
 # DOC: A statement `using namespace std` in a source files is considered bad practice. Indeed, it makes it difficult to
 # DOC: distinguish classes and functions that are taken from the standard library (such as `std::vector`) from similar
 # DOC: names declared locally. This is currently a style issue, but will become an error in the future.
       if(filetype=="source" && match($0,"using *namespace *std") && !in_plumed_doc && !in_doxygen ) style("using_namespace_std_in_source","using namespace std statement in source file")
     } else {
 # check that "using namespace" statements are not used in source files
 # DOC: :using_namespace_std_in_default_source:
+# DOC: `using namespace std` in default source file.
 # DOC: A statement `using namespace std` in a source files is considered bad practice. Indeed, it makes it difficult to
 # DOC: distinguish classes and functions that are taken from the standard library (such as `std::vector`) from similar
 # DOC: names declared locally. This is currently an error for default modules.
@@ -552,6 +562,7 @@ BEGINFILE{
 # take note of used HAS macros
       if(plumed_usehas_file[string,FILENAME]!="used") information("used_has",string);
 # DOC: :used_has_in_header: (style)
+# DOC: Use of  `__PLUMED_HAS_SOMETHING` macros in header.
 # DOC: Using `__PLUMED_HAS_SOMETHING` macros in headers should be avoided since
 # DOC: it could make it difficult to make sure the same macros are correctly defined
 # DOC: when using PLUMED as a library. This is only a stylistic warning now. Notice that
@@ -579,11 +590,13 @@ ENDFILE{
 # check for namespaces
   if(found_non_preprocessor_lines && (filetype=="source" || filetype=="header") && !(module in outer_modules) ){
 # DOC: :missing_namespace_plmd:
+# DOC: Missing namespace `PLMD`.
 # DOC: Every source file should contain a `PLMD` namespace.
 # DOC: Notice that files in the "outer  modules" `wrapper` and `main`, that is those that
 # DOC: are not included in the kernel library, are exempted from this rule.
     if(!found_namespace_plmd) error("missing_namespace_plmd","missing PLMD namespace");
 # DOC: :missing_namespace_module:
+# DOC: Missing module namespace.
 # DOC: Every source file should contain a sub namespace with the same name of the module itself.
 # DOC: Notice that there are important exceptions to this rule:
 # DOC: - "outer modules" `wrapper` and `main`, that are not included in the kernel library.
@@ -595,6 +608,7 @@ ENDFILE{
   if(filetype=="header"){
 # this can be done only if module name is known:
 # DOC: :missing_guard:
+# DOC: Missing header guard.
 # DOC: Every header file should contain a proper guard to avoid double inclusion.
 # DOC: The format of the guard is fixed and should be `__PLUMED_modulename_filename_h`, where
 # DOC: `modulename` is the name of the module and `filename` is the name of the file, without suffix.
@@ -615,6 +629,7 @@ ENDFILE{
      }
      if(notfound){
 # DOC: :non_existing_cpp:
+# DOC: Header file lacks compantion source file.
 # DOC: For every header file there should exist a corresponding source file with the same name.
 # DOC: Notice that dummy headers that only include another header or which only define preprocessor
 # DOC: macros are exempted from this rule.
@@ -626,6 +641,7 @@ ENDFILE{
         noIncludes=0;
          sub("^# *include","");
 # DOC: :non_included_h:
+# DOC: Source files must include the corrisponding header file.
 # DOC: The source file corresponding to a header file (that is: with the same name) should include it as the first included file.
 # DOC: This is to make sure that all the header files that we install can be individually included and compiled.
          if($1!="\"" file "\"") error("non_included_h","file " file " is a header but " cppfile " does not include it as first include");
@@ -649,8 +665,7 @@ END{
   for(action in registered_actions){
     if(!(action in plumed_doc_action)){
 # DOC: :undocumented_action:
-# DOC: Every action that is registered with `PLUMED_REGISTER_ACTION` should also be documented
-# DOC: in a PLUMEDOC page.
+# DOC: Every action that is registered with `PLUMED_REGISTER_ACTION` should also be documented in a PLUMEDOC page.
       style("undocumented_action","action " action " at " registered_actions[action] " is not documented")
     } else if(!(action in provide_examples)){
 # DOC: :action_without_examples:
@@ -700,17 +715,17 @@ END{
   for(has in plumed_usehas){
     if(!(has in plumed_definehas)){
 # DOC: :undefined_has:
-# DOC: Every macro in the form `__PLUMED_HAS_SOMETHING` that is used in the code
-# DOC: should be defined in configure.ac. This check is made to avoid errors with mis-typed macros.
+# DOC: Every macro in the form `__PLUMED_HAS_SOMETHING` that is used in the code should be defined in configure.ac.
+# DOC: This check is made to avoid errors with mis-typed macros.
       error("undefined_has","has " has " at " plumed_usehas[has] " is not defined")
     }
   }
 
   for(has in plumed_definehas){
     if(!(has in plumed_usehas)){
 # DOC: :unused_has:
-# DOC: Every macro in the form `__PLUMED_HAS_SOMETHING` that is defined in configure.ac
-# DOC: should be used at least once in the code. This check is made to avoid errors with mis-typed macros.
+# DOC: Every macro in the form `__PLUMED_HAS_SOMETHING` that is defined in configure.ac should be used at least once in the code.
+# DOC: This check is made to avoid errors with mis-typed macros.
       error("unused_has","has " has " at " plumed_definehas[has] " is not used")
     }
   }