LDMX-Software / ldmx-sw

The Light Dark Matter eXperiment simulation and reconstruction framework.
https://ldmx-software.github.io
GNU General Public License v3.0
22 stars 20 forks source link

format source files #1168

Closed tomeichlersmith closed 3 months ago

tomeichlersmith commented 1 year ago

Describe the bug A lot of the C++ source files do not conform to the Google format. This is unsurprising since we don't have any auto-formatting tools and we don't document how to event apply the formatting.

The soon-to-be-released v4.0 of the dev container will contain clang-format which means we could introduce a ldmx subcommand for formatting source files ldmx format.

I have something that works, but the problem is it changes a lot of the files across many submodules and I think a purely formatting change should be done separately.

###############################################################################
# __ldmx_format
#   format the C++ code with clang-format in the container (requires v4.0.0)
#   uses `find` to run it on all *.h and *.cxx files in ldmx-sw
###############################################################################
__ldmx_format() {
  __ldmx_run . 'if command -v clang-format &> /dev/null; then find ${LDMX_BASE}/ldmx-sw -type f -name "*.h" -o -name "*.cxx" -exec clang-format --style=Google -i {} \;; else echo "ERROR: no clang-format in container"; fi'
  return $?
}

We could probably update this to take some directory or file path so that it doesn't always do the entire ldmx-sw code base.

EinarElen commented 1 year ago

Adding a list of things that have definitely been formatted by now

bryngemark commented 11 months ago

did we settle on if we wanted to apply formatting at the end of successful CI tests in a PR? and do we in that case need to check building/rerun tests afterwards?

another option is to add code formatting to the PR checklist.

EinarElen commented 11 months ago

I don't think we settled on anything, but since all the source files in SimCore have been formatted there is a CI check in SimCore that gives an error on unformatted source files https://github.com/LDMX-Software/SimCore/blob/trunk/.github/format-check

When it comes to rebuilding/testing after applying clang-format in a PR, I don't think we need to do this.

EinarElen commented 11 months ago

Here is the current formatting status of the different submodules: I got this by 1: Copying the .clang-format file from SimCore

find include/ -type f >  ${TMPDIR:-/tmp}/files-to-format.list
find src/ -type f     >> ${TMPDIR:-/tmp}/files-to-format.list

# Count number of unformatted files 

clang-format --verbose -Werror --dry-run $(cat ${TMPDIR:-/tmp}/files-to-format.list) 2>&1 | grep -o "^\S*.[ch]" | sort -u | wc -l

Processing directory: SimCore
Number of errors:        0
Number of lines:      121
Processing directory: Hcal
Number of errors:        2
Number of lines:       39
Processing directory: TrigScint
Number of errors:       33
Number of lines:       45
Processing directory: DQM
Number of errors:       26
Number of lines:       34
Processing directory: DetDescr
Number of errors:       33
Number of lines:       37
Processing directory: Ecal
Number of errors:       38
Number of lines:       38
Processing directory: Packing
Number of errors:       36
Number of lines:       20
Processing directory: Recon
Number of errors:       20
Number of lines:       26
Processing directory: Tools
Number of errors:       18
Number of lines:       12
Processing directory: Tracking
Number of errors:       98
Number of lines:       58
Processing directory: Biasing
Number of errors:       39
Number of lines:       32
Processing directory: Conditions
Number of errors:       14
Number of lines:       10
Processing directory: Framework
Number of errors:       35
Number of lines:       45

Sigh, Im getting some double counting here that I don't quite understand... uniq treats

include//Hcal/HcalReconConditions.h
include//Hcal/HcalReconConditions.h

to be the different and I can't tell what the difference is

EinarElen commented 11 months ago

Sigh, the problem was that I was getting lines with the same text but different terminal style characters... After fixing that, here's a script that can be used to check how many of our files are formatted

#!/bin/bash
verbose=false

if [ "$#" -eq 1 ] && [ "$1" == "verbose" ]; then
    verbose=true
fi
. scripts/ldmx-env.sh
for dir in SimCore Hcal TrigScint DQM DetDescr Ecal Packing Recon Tools Tracking Biasing Conditions Framework ; do
    echo "Processing directory: $dir"

    cd "$dir" || exit 1

    # Copy .clang-format from ../SimCore/.clang-format
    cp ../SimCore/.clang-format . > /dev/null

    # Create the list of files to format
    find include/ -type f > "/tmp/files.list" || continue
    find src/ -type f >> "/tmp/files.list"

    ldmx clang-format --verbose -Werror --dry-run $(cat /tmp/files.list) 2>&1 | grep -o "^\S*\.[ch]" | sed -E 's/^[^is]*(s|i)/\1/' | uniq > "/tmp/errors.list"

    error_count=$(wc -l < "/tmp/errors.list")
    line_count=$(wc -l < "/tmp/files.list")

    # Print results
    echo "Number of errors: $error_count"
    echo "Number of lines: $line_count"

    if [ "$verbose" = true ]; then
        echo "Files with errors:"
        cat "/tmp/errors.list"
    fi

    cd - > /dev/null || exit 1
done
Processing directory: SimCore
cp: ./.clang-format and ../SimCore/.clang-format are identical (not copied).
Number of errors:        0
Number of lines:      121
Processing directory: Hcal
Number of errors:        1
Number of lines:       39
Processing directory: TrigScint
Number of errors:       19
Number of lines:       45
Processing directory: DQM
Number of errors:       14
Number of lines:       34
Processing directory: DetDescr
Number of errors:       21
Number of lines:       37
Processing directory: Ecal
Number of errors:       22
Number of lines:       38
Processing directory: Packing
Number of errors:       18
Number of lines:       20
Processing directory: Recon
Number of errors:       12
Number of lines:       26
Processing directory: Tools
Number of errors:       11
Number of lines:       12
Processing directory: Tracking
Number of errors:       53
Number of lines:       58
Processing directory: Biasing
Number of errors:       22
Number of lines:       32
Processing directory: Conditions
Number of errors:        8
Number of lines:       10
Processing directory: Framework
Number of errors:       23
Number of lines:       45
tvami commented 3 months ago

For the records, done in https://github.com/LDMX-Software/ldmx-sw/pull/1362