rework formatting with cmake-format (#4627)

* merge clangify and cmakify into format.sh

update desktop lint workflow to 22.04

print cmake-format version as well

um, rename things?

add extra examples to format.sh --help
add option to not run clang-format
fix version display in .ci/lint_cpp.sh
fix relative paths in format.sh

fix formatting dirs

* run ./format.sh --cmake --branch ""

* revert formatting of cmake comments

.ci/lint_cpp.sh

@@ -10,10 +10,10 @@ if ! git merge-base origin/master HEAD; then
   git fetch --unshallow
 fi
 
-# Check formatting using clangify
+# Check formatting using format.sh
-echo "Checking your code using clang-format..."
+echo "Checking your code using clang-format/cmake-format..."
 
-diff="$(./clangify.sh --diff --cf-version --branch origin/master)"
+diff="$(./format.sh --diff --cmake --cf-version --branch origin/master)"
 err=$?
 
 case $err in
@@ -24,7 +24,7 @@ case $err in
 ***                                                     ***
 ***   Your code does not comply with our style guide.   ***
 ***                                                     ***
-***  Please correct it or run the "clangify.sh" script. ***
+***   Please correct it or run the "format.sh" script.  ***
 ***  Then commit and push those changes to this branch. ***
 ***   Check our CONTRIBUTING.md file for more details.  ***
 ***                                                     ***
@@ -32,12 +32,14 @@ case $err in
 ***                                                     ***
 ***********************************************************
 
-Used clang-format version:
+Used version:
 ${diff%%
+----------
 *}
 
 The following changes should be made:
 ${diff#*
+----------
 }
 
 Exiting...
@@ -62,8 +64,6 @@ EOM
     ;;
 
   *)
-    echo ""
+    echo "Something went wrong in our formatting checks: format.sh returned $err" >&2
-    echo "Something went wrong in our formatting checks: clangify returned $err" >&2
-    echo ""
     ;;
 esac

.cmake-format.json

@@ -102,5 +102,8 @@
       "decisions. See the documentation for more information."
     ],
     "layout_passes": {}
+  },
+  "markup": {
+    "enable_markup": false
   }
 }

.github/CONTRIBUTING.md

@@ -41,8 +41,8 @@ albeit slightly less active.
 
 We use a separate job on the CI to check your code for formatting issues. If
 your pull request failed the test, you can check the output on the checks tab.
-It's the first job called "linter", you can click the "Run clangify" step to
+It's the first job called "linter", you can click the "Check code formatting"
-see the output of the test.
+step to see the output of the test.
 
 The message will look like this:
 ```
@@ -50,7 +50,7 @@ The message will look like this:
 ***                                                     ***
 ***   Your code does not comply with our style guide.   ***
 ***                                                     ***
-***  Please correct it or run the "clangify.sh" script. ***
+***   Please correct it or run the "format.sh" script.  ***
 ***  Then commit and push those changes to this branch. ***
 ***   Check our CONTRIBUTING.md file for more details.  ***
 ***                                                     ***
@@ -81,9 +81,9 @@ The handy tool `clang-format` can format your code for you, it is available for
 almost any environment. A special `.clang-format` configuration file is
 included in the project and is used to format your code.
 
-We've also included a bash script, `clangify.sh`, that will use clang-format to
+We've also included a bash script, `format.sh`, that will use clang-format to
-format all files in your pr in one go. Use `./clangify.sh --help` to show a
+format all files in your pr in one go. Use `./format.sh --help` to show a full
-full help page.
+help page.
 
 To run clang-format on a single source file simply use the command
 `clang-format -i <filename>` to format it in place. (Some systems install

.github/workflows/desktop-lint.yml

@@ -8,8 +8,8 @@ on:
       - '.github/workflows/web-*.yml'
 
 jobs:
-  clang-format:
+  format:
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-22.04
 
     steps:
     - name: Checkout
@@ -17,12 +17,12 @@ jobs:
       with:
         fetch-depth: 20 # should be enough to find merge base
 
-    - name: Install clang-format
+    - name: Install dependencies
       shell: bash
       run: |
         sudo apt-get update
-        sudo apt-get install -y --no-install-recommends clang-format
+        sudo apt-get install -y --no-install-recommends clang-format cmake-format
 
-    - name: Run clangify
+    - name: Check code formatting
       shell: bash
       run: ./.ci/lint_cpp.sh

CMakeLists.txt

@@ -1,7 +1,9 @@
 # Cockatrice's main CMakeLists.txt
 #
-# This is basically a wrapper to enable/disable the compilation of the different projects: servatrice, cockatrice, test
+# This is basically a wrapper to enable/disable the compilation
-# This file sets all the variables shared between the projects like the installation path, compilation flags etc..
+# of the different projects: servatrice, cockatrice, test
+# This file sets all the variables shared between the projects
+# like the installation path, compilation flags etc..
 
 # cmake 3.16 is required if using qt6
 cmake_minimum_required(VERSION 3.10)
@@ -23,7 +25,8 @@ option(WITH_DBCONVERTER "build dbconverter" ON)
 # Compile tests
 option(TEST "build tests" OFF)
 
-# Default to "Release" build type User-provided value for CMAKE_BUILD_TYPE must be checked before the PROJECT() call
+# Default to "Release" build type
+# User-provided value for CMAKE_BUILD_TYPE must be checked before the PROJECT() call
 if(DEFINED CMAKE_BUILD_TYPE)
   set(CMAKE_BUILD_TYPE
       ${CMAKE_BUILD_TYPE}
@@ -69,7 +72,8 @@ if(WIN32)
   endif()
 endif()
 
-# A project name is needed for CPack Version can be overriden by git tags, see cmake/getversion.cmake
+# A project name is needed for CPack
+# Version can be overriden by git tags, see cmake/getversion.cmake
 project("Cockatrice" VERSION 2.8.1)
 
 # Set release name if not provided via env/cmake var
@@ -99,7 +103,9 @@ include(createversionfile)
 # Define a proper install path
 if(UNIX)
   if(APPLE)
-    # macOS Due to the special bundle structure ignore the prefix eventually set by the user.
+    # macOS
+    # Due to the special bundle structure ignore
+    # the prefix eventually set by the user.
     set(CMAKE_INSTALL_PREFIX ${CMAKE_BINARY_DIR}/release)
 
     # Force ccache usage if available
@@ -136,7 +142,8 @@ endif()
 if(MSVC)
   # Visual Studio: Maximum optimization, disable warning C4251, establish C++17 compatibility
   set(CMAKE_CXX_FLAGS_RELEASE "/Ox /MD /wd4251 /Zc:__cplusplus /std:c++17 /permissive-")
-  # Generate complete debugging information set(CMAKE_CXX_FLAGS_DEBUG "/Zi")
+  # Generate complete debugging information
+  #set(CMAKE_CXX_FLAGS_DEBUG "/Zi")
 elseif(CMAKE_COMPILER_IS_GNUCXX)
   # linux/gcc, bsd/gcc, windows/mingw
   include(CheckCXXCompilerFlag)

cmake/FindQtRuntime.cmake

@@ -1,7 +1,13 @@
-# Find a compatible Qt version Inputs: WITH_SERVER, WITH_CLIENT, WITH_ORACLE, WITH_DBCONVERTER, FORCE_USE_QT5 Optional
+# Find a compatible Qt version
-# Input: QT6_DIR -- Hint as to where Qt6 lives on the system Optional Input: QT5_DIR -- Hint as to where Qt5 lives on
+# Inputs: WITH_SERVER, WITH_CLIENT, WITH_ORACLE, WITH_DBCONVERTER, FORCE_USE_QT5
-# the system Output: COCKATRICE_QT_VERSION_NAME -- Example values: Qt5, Qt6 Outputs: SERVATRICE_QT_MODULES,
+# Optional Input: QT6_DIR -- Hint as to where Qt6 lives on the system
-# COCKATRICE_QT_MODULES, ORACLE_QT_MODULES, DBCONVERTER_QT_MODULES, TEST_QT_MODULES
+# Optional Input: QT5_DIR -- Hint as to where Qt5 lives on the system
+# Output: COCKATRICE_QT_VERSION_NAME -- Example values: Qt5, Qt6
+# Output: SERVATRICE_QT_MODULES
+# Output: COCKATRICE_QT_MODULES
+# Output: ORACLE_QT_MODULES
+# Output: DBCONVERTER_QT_MODULES
+# Output: TEST_QT_MODULES
 
 set(REQUIRED_QT_COMPONENTS Core)
 if(WITH_SERVER)

cmake/FindWin32SslRuntime.cmake

@@ -1,4 +1,5 @@
-# Find the OpenSSL runtime libraries (.dll) for Windows that will be needed by Qt in order to access https urls.
+# Find the OpenSSL runtime libraries (.dll) for Windows that
+# will be needed by Qt in order to access https urls.
 if(NOT DEFINED WIN32 OR NOT ${WIN32})
   message(STATUS "Non-Windows device trying to execute FindWin32SslRuntime, skipping")
   return()

cmake/getversion.cmake

@@ -71,7 +71,9 @@ function(get_tag_name commit)
   string(REPLACE "\n" "" GIT_TAG "${GIT_TAG}")
   message(STATUS "Commit is a release or prerelease, git tag: ${GIT_TAG}")
 
-  # Extract information from tag: YYYY-MM-DD-Release-MAJ.MIN.PATCH YYYY-MM-DD-Development-MAJ.MIN.PATCH-beta.X
+  # Extract information from tag:
+  # YYYY-MM-DD-Release-MAJ.MIN.PATCH
+  # YYYY-MM-DD-Development-MAJ.MIN.PATCH-beta.X
   string(REPLACE "-" ";" GIT_TAG_EXPLODED "${GIT_TAG}")
   string(REPLACE "." ";" GIT_TAG_EXPLODED "${GIT_TAG_EXPLODED}")
 
@@ -131,8 +133,10 @@ function(get_tag_name commit)
     return()
   endif()
 
-  # Label 7 = Stable release 8 = Dev release, first beta so only "beta" attached 9 = Dev release, subsequent beta so
+  # Label
-  # "beta.N" attached (N>=2)
+  # 7 = Stable release
+  # 8 = Dev release, first beta so only "beta" attached
+  # 9 = Dev release, subsequent beta so "beta.N" attached (N>=2)
   if(${GIT_TAG_LISTCOUNT} EQUAL 8)
     list(GET GIT_TAG_EXPLODED 7 GIT_TAG_LABEL)
   elseif(${GIT_TAG_LISTCOUNT} EQUAL 9)

cmakeify.sh

@@ -1,14 +0,0 @@
-#!/bin/bash
-
-# go to the project root directory, this file should be located in the project root directory
-cd "${BASH_SOURCE%/*}/" || exit 2 # could not find path, this could happen with special links etc.
-
-# CMake-Format
-cmake_format_cmd="cmake-format"
-
-if ! hash $cmake_format_cmd 2>/dev/null; then
-  echo "could not find $cmake_format_cmd, skipping" >&2
-  return
-fi
-
-$cmake_format_cmd -i cmake/*.cmake */CMakeLists.txt CMakeLists.txt

cockatrice/src/pictureloader.cpp

@@ -6,7 +6,6 @@
 #include "thememanager.h"
 
 #include <QApplication>
-#include <QScreen>
 #include <QCryptographicHash>
 #include <QDebug>
 #include <QDir>
@@ -19,6 +18,7 @@
 #include <QPainter>
 #include <QPixmapCache>
 #include <QRegularExpression>
+#include <QScreen>
 #include <QSet>
 #include <QSvgRenderer>
 #include <QThread>

dbconverter/src/main.cpp

@@ -17,15 +17,16 @@
  *   59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.             *
  ***************************************************************************/
 
+#include "main.h"
+
+#include "mocks.h"
+#include "version_string.h"
+
 #include <QCommandLineParser>
 #include <QCoreApplication>
+#include <QDebug>
 #include <QFileInfo>
 #include <QtGlobal>
-#include <QDebug>
-
-#include "version_string.h"
-#include "mocks.h"
-#include "main.h"
 
 int main(int argc, char *argv[])
 {
@@ -44,8 +45,7 @@ int main(int argc, char *argv[])
     QString oldDbPath;
     QString newDbPath;
     QStringList args = parser.positionalArguments();
-    if (args.count() == 2)
+    if (args.count() == 2) {
-    {
         oldDbPath = QFileInfo(args.at(0)).absoluteFilePath();
         newDbPath = QFileInfo(args.at(1)).absoluteFilePath();
     } else {

dbconverter/src/main.h

@@ -7,11 +7,13 @@
 class CardDatabaseConverter : public CardDatabase
 {
 public:
-	LoadStatus loadCardDatabase(const QString &path) {
+    LoadStatus loadCardDatabase(const QString &path)
+    {
         return CardDatabase::loadCardDatabase(path);
     }
 
-	bool saveCardDatabase(const QString &fileName) {
+    bool saveCardDatabase(const QString &fileName)
+    {
         CockatriceXml4Parser parser;
         return parser.saveToFile(sets, cards, fileName);
     }

dbconverter/src/mocks.cpp

@@ -41,14 +41,9 @@ QString SettingsCache::getSafeConfigFilePath(QString /* configEntry */, QString
     return defaultPath;
 }
 SettingsCache::SettingsCache()
-    : settings{new QSettings("global.ini", QSettings::IniFormat, this)},
+    : settings{new QSettings("global.ini", QSettings::IniFormat, this)}, shortcutsSettings{nullptr},
-      shortcutsSettings{nullptr},
+      cardDatabaseSettings{new CardDatabaseSettings("", this)}, serversSettings{nullptr}, messageSettings{nullptr},
-      cardDatabaseSettings{new CardDatabaseSettings("", this)},
+      gameFiltersSettings{nullptr}, layoutsSettings{nullptr}, downloadSettings{nullptr}
-      serversSettings{nullptr},
-      messageSettings{nullptr},
-      gameFiltersSettings{nullptr},
-      layoutsSettings{nullptr},
-      downloadSettings{nullptr}
 {
 }
 void SettingsCache::setUseTearOffMenus(bool /* _useTearOffMenus */)

format.sh

@@ -1,15 +1,20 @@
 #!/bin/bash
 
 # This script will run clang-format on all modified, non-3rd-party C++/Header files.
+# Optionally runs cmake-format on all modified cmake files.
+# Uses clang-format cmake-format git diff find
 # Never, ever, should this receive a path with a newline in it. Don't bother proofing it for that.
 
+set -o pipefail
 
 # go to the project root directory, this file should be located in the project root directory
+olddir="$PWD"
 cd "${BASH_SOURCE%/*}/" || exit 2 # could not find path, this could happen with special links etc.
 
 # defaults
 include=("common" \
 "cockatrice/src" \
+"dbconverter/src" \
 "oracle/src" \
 "servatrice/src" \
 "tests")
@@ -22,22 +27,30 @@ exclude=("servatrice/src/smtp" \
 exts=("cpp" "h" "proto")
 cf_cmd="clang-format"
 branch="origin/master"
+cmakefile="CMakeLists.txt"
+cmakedir="cmake/.*\\.cmake"
+cmakeinclude=("cmake/gtest-CMakeLists.txt.in")
+color="--"
 
 # parse options
-while [[ $@ ]]; do
+while [[ $* ]]; do
   case "$1" in
     '-b'|'--branch')
       branch=$2
       set_branch=1
       shift 2
       ;;
+    '--cmake')
+      do_cmake=1
+      shift
+      ;;
     '-c'|'--color-diff')
       color="--color=always"
-      mode=diff
+      mode="diff"
       shift
       ;;
     '-d'|'--diff')
-      mode=diff
+      mode="diff"
       shift
       ;;
     '-h'|'--help')
@@ -49,7 +62,6 @@ If <dir>s are given, all source files in those directories of the project root
 path are formatted. To only format changed files in these directories use the
 --branch option in combination. <dir> has to be a path relative to the project
 root path or a full path inside $PWD.
-. can not be specified as a dir, if you really want to format everything use */.
 
 USAGE: $0 [option] [--branch <git branch or object>] [<dir> ...]
 
@@ -68,6 +80,9 @@ OPTIONS:
         When not comparing to a branch, git will not be used at all and every
         source file in the entire project will be parsed.
 
+    --cmake
+        Use cmake-format to format cmake files as well.
+
     -c, --color-diff
         Display a colored diff. Implies --diff.
         Only available on systems which support 'diff --color'.
@@ -94,22 +109,25 @@ EXIT CODES:
     3 if clang-format could not be found.
 
 EXAMPLES:
-    $0 --test \$PWD || echo "code requires formatting"
-        Tests if the source files in the current directory are correctly
-        formatted and prints an error message if formatting is required.
-
     $0 --branch $USER/patch-2 ${include[0]}
         Formats all changed files compared to the git branch "$USER/patch-2"
         in the directory ${include[0]}.
+
+    $0 --test . || echo "code requires formatting"
+        Tests if the source files in the current directory are correctly
+        formatted and prints an error message if formatting is required.
+
+    $0 --cmake --branch "" ""
+        Unconditionally format all cmake files and no source files.
 EOM
       exit 0
       ;;
     '-n'|'--names')
-      mode=name
+      mode="name"
       shift
       ;;
     '-t'|'--test')
-      mode=code
+      mode="code"
       shift
       ;;
     '--cf-version')
@@ -117,20 +135,28 @@ EOM
       shift
       ;;
     '--')
+      dashdash=1
       shift
       ;;
     *)
-      if next_dir=$(cd "$1" && pwd); then
+      if [[ ! $dashdash && $1 =~ ^-- ]]; then
-        if [[ ${next_dir#$PWD/} == /* ]]; then
+        echo "error in parsing arguments of $0: $1 is an unrecognized option" >&2
-          echo "error in parsing arguments of $0: $next_dir is not in $PWD" >&2
         exit 2 # input error
-        elif ! [[ $set_include ]]; then
+      fi
+      if [[ ! $1 ]] || next_dir=$(cd "$olddir" && cd -- "$1" && pwd); then
+        if ! [[ $set_include ]]; then
           include=() # remove default includes
           set_include=1
         fi
-        include+=("${next_dir#$PWD/}")
+        if [[ $1 ]]; then
+          if [[ $next_dir != $PWD/* ]]; then
+            echo "error in parsing arguments of $0: $next_dir is not in $PWD" >&2
+            exit 2 # input error
+          fi
+          include+=("$next_dir")
+        fi
       else
-        echo "error in parsing arguments of $0: $PWD/$1 is not a directory" >&2
+        echo "error in parsing arguments of $0: $1 is not a directory" >&2
         exit 2 # input error
       fi
       if ! [[ $set_branch ]]; then
@@ -153,37 +179,93 @@ if ! hash $cf_cmd 2>/dev/null; then
   fi
 fi
 
+# check availability of cmake-format
+if [[ $do_cmake ]] && ! hash cmake-format 2>/dev/null; then
+  echo "could not find cmake-format" >&2
+  exit 3
+fi
+
 if [[ $branch ]]; then
   # get all dirty files through git
-  if ! base=$(git merge-base ${branch} HEAD); then
+  if ! base=$(git merge-base "$branch" HEAD); then
     echo "could not find git merge base" >&2
     exit 2 # input error
   fi
-  declare -a reg
+  mapfile -t basenames < <(git diff --diff-filter=d --name-only "$base")
-  for ex in ${exts[@]}; do
+  names=()
-    reg+=(${include[@]/%/.*\\.$ex\$})
+  for ex in "${exts[@]}"; do
+    for path in "${include[@]}"; do
+      for name in "${basenames[@]}"; do
+        rx="^$path/.$ex$"
+        if [[ $name =~ $rx ]]; then
+          names+=("$name")
+        fi
       done
-  names=$(git diff --diff-filter=d --name-only $base | grep ${reg[@]/#/-e ^})
+    done
+  done
+  if [[ $do_cmake ]]; then
+    cmake_names=()
+    for name in "${basenames[@]}"; do
+      dirrx="^$cmakedir$"
+      filerx="(^|/)$cmakefile$"
+      if [[ $name =~ $dirrx || $name =~ $filerx ]]; then
+        cmake_names+=("$name")
+      fi
+      for include in "${cmakeinclude[@]}"; do
+        if [[ $name == "$include" ]]; then
+          cmake_names+=("$name")
+        fi
+      done
+    done
+  fi
 else
-  names=$(find ${include[@]} -type f -false ${exts[@]/#/-o -name *\\.})
+  exts_o=()
+  for ext in "${exts[@]}"; do
+    exts_o+=(-o -name "*\\.$ext")
+  done
+  unset "exts_o[0]" # remove first -o
+  mapfile -t names < <(find "${include[@]}" -type f "${exts_o[@]}")
+  if [[ $do_cmake ]]; then
+    mapfile -t cmake_names < <(find . -maxdepth 2 -type f -name "$cmakefile" -o -path "./${cmakedir/.}")
+    cmake_names+=("${cmakeinclude[@]}")
+  fi
 fi
 
 # filter excludes
-names=$(<<<"$names" grep -v ${exclude[@]/#/-e ^})
+for path in "${exclude[@]}"; do
-
+  for i in "${!names[@]}"; do
-if ! [[ $names ]]; then
+    rx="^$path/"
-  exit 0 # nothing to format means format is successful!
+    if [[ ${names[$i]} =~ $rx ]]; then
+      unset "names[$i]"
     fi
+  done
+done
 
 # optionally print version
-[[ $print_version ]] && $cf_cmd -version
+if [[ $print_version ]]; then
+  $cf_cmd -version
+  [[ $do_cmake ]] && echo "cmake-format $(cmake-format --version)"
+  echo "----------"
+fi
+
+if [[ ! ${cmake_names[*]} ]]; then
+  unset do_cmake
+fi
+if [[ ! ( ${names[*]} || $do_cmake ) ]]; then
+  exit 0 # nothing to format means format is successful!
+fi
 
 # format
 case $mode in
   diff)
     declare -i code=0
-    for name in ${names[@]}; do
+    for name in "${names[@]}"; do
-      if ! $cf_cmd "$name" | diff "$name" - -p $color; then
+      if ! $cf_cmd "$name" | diff "$name" - -p "$color"; then
+        code=1
+      fi
+    done
+    for name in "${cmake_names[@]}"; do
+      if ! cmake-format "$name" | diff "$name" - -p "$color"; then
         code=1
       fi
     done
@@ -191,20 +273,34 @@ case $mode in
     ;;
   name)
     declare -i code=0
-    for name in ${names[@]}; do
+    for name in "${names[@]}"; do
       if ! $cf_cmd "$name" | diff "$name" - -q >/dev/null; then
         echo "$name"
         code=1
       fi
     done
+    for name in "${cmake_names[@]}"; do
+      if ! cmake-format "$name" --check; then
+        echo "$name"
+        code=1
+      fi
+    done
     exit $code
     ;;
   code)
-    for name in ${names[@]}; do
+    for name in "${names[@]}"; do
       $cf_cmd "$name" | diff "$name" - -q >/dev/null || exit 1
     done
+    for name in "${cmake_names[@]}"; do
+      cmake-format "$name" --check || exit 1
+    done
     ;;
   *)
-    $cf_cmd -i $names
+    if [[ "${names[*]}" ]]; then
+      $cf_cmd -i "${names[@]}"
+    fi
+    if [[ $do_cmake ]]; then
+      cmake-format -i "${cmake_names[@]}"
+    fi
     ;;
 esac

servatrice/src/smtpclient.cpp

@@ -1,4 +1,5 @@
 #include "smtpclient.h"
+
 #include "settingscache.h"
 #include "smtp/qxtsmtp.h"
 

tests/CMakeLists.txt

@@ -31,7 +31,8 @@ if(NOT GTEST_FOUND)
   # Add gtest directly to our build
   add_subdirectory(${CMAKE_BINARY_DIR}/gtest-src ${CMAKE_BINARY_DIR}/gtest-build EXCLUDE_FROM_ALL)
 
-  # Add the gtest include directory, since gtest doesn't add that dependency to its gtest target
+  # Add the gtest include directory, since gtest
+  # doesn't add that dependency to its gtest target
   target_include_directories(gtest INTERFACE "$<BUILD_INTERFACE:${CMAKE_BINARY_DIR}/gtest-src/include>")
 
   set(GTEST_INCLUDE_DIRS "${CMAKE_BINARY_DIR}/gtest-src/include")