Merge pull request #600 from Lzzz666/python-formatter

Enhance CI formatting checks for Python files

Author: jserv

.ci/check-format.sh

@@ -21,4 +21,14 @@ for file in ${SH_SOURCES}; do
 done
 SH_MISMATCH_FILE_CNT=$(shfmt -l ${SH_SOURCES})
 
-exit $((C_MISMATCH_LINE_CNT + SH_MISMATCH_FILE_CNT))
+PY_SOURCES=$(find "${REPO_ROOT}" | egrep "\.py$")
+for file in ${PY_SOURCES}; do
+    echo "Checking Python file: ${file}"
+    black --diff "${file}"
+done
+PY_MISMATCH_FILE_CNT=0
+if [ -n "${PY_SOURCES}" ]; then
+    PY_MISMATCH_FILE_CNT=$(echo "$(black --check ${PY_SOURCES} 2>&1)" | grep -c "^would reformat ")
+fi
+
+exit $((C_MISMATCH_LINE_CNT + SH_MISMATCH_FILE_CNT + PY_MISMATCH_FILE_CNT))

.github/workflows/main.yml

@@ -505,7 +505,8 @@ jobs:
     - uses: actions/checkout@v4
     - name: coding convention
       run: |
-            sudo apt-get install -q=2 clang-format-18 shfmt
+            sudo apt-get install -q=2 clang-format-18 shfmt python3-pip
+            pip3 install black==25.1.0
             .ci/check-newline.sh
             .ci/check-format.sh
       shell: bash

CONTRIBUTING.md

@@ -39,6 +39,7 @@ However, participation requires adherence to fundamental ground rules:
   While there is some flexibility in basic style, it is crucial to stick to the current coding standards.
   Complex algorithmic constructs without proper comments will not be accepted.
 * Shell scripts must be formatted before submission. Use consistent flags across the project to ensure uniform formatting.
+* Python scripts must be formatted before submission. Use consistent flags across the project to ensure uniform formatting.
 * External pull requests should include thorough documentation in the pull request comments for consideration.
 * When composing documentation, code comments, and other materials in English,
   please adhere to the American English (`en_US`) dialect.
@@ -48,9 +49,12 @@ However, participation requires adherence to fundamental ground rules:
 Software requirement:
 * [clang-format](https://clang.llvm.org/docs/ClangFormat.html) version 18 or later.
 * [shfmt](https://github.com/mvdan/sh).
+* [black](https://github.com/psf/black) version 25.1.0.
 
-This repository consistently contains an up-to-date `.clang-format` file with rules that match the explained ones and uses shell script formatting supported by `shfmt`.
-For maintaining a uniform coding style, execute the command `clang-format -i *.{c,h}` and `shfmt -w $(find . -type f -name "*.sh")`.
+To maintain a uniform style across languages, run:
+* `clang-format -i *.{c,h}` to apply the project’s C/C++ formatting rules from the up-to-date .clang-format file.
+* `shfmt -w $(find . -type f -name "*.sh")` to clean and standardize all shell scripts.
+* `black .` to enforce a consistent, idiomatic layout for Python code.
 
 ## Coding Style for Shell Script
 
@@ -65,6 +69,19 @@ Shell scripts must be clean, consistent, and portable. The following `shfmt` rul
 * Add spaces around redirection operators (e.g., `>`, `>>`).
 * Place binary operators (e.g., `&&`, `|`) on the next line when breaking lines.
 
+## Coding Style for Python
+
+Python scripts must be clean, consistent, and adhere to modern Python best practices. The following formatting rules are enforced project-wide using `black`:
+
+* Use 4 spaces for indentation (never tabs).
+* Limit lines to 80 characters maximum.
+* Use double quotes for strings (unless single quotes avoid escaping).
+* Use trailing commas in multi-line constructs.
+* Format imports according to PEP 8 guidelines.
+* Use Unix-style line endings (LF).
+* Remove trailing whitespace at the end of lines.
+* Ensure files end with a newline.
+
 ## Coding Style for Modern C
 
 This coding style is a variant of the [K&R style](https://en.wikipedia.org/wiki/Indentation_style#K&R).

pyproject.toml

@@ -0,0 +1,4 @@
+[tool.black]
+line-length = 80
+target-version = ['py39', 'py310', 'py311','py312','py313']
+include = '\.pyi?$'

tests/arch-test-target/constants.py

@@ -1,19 +1,19 @@
 import os
 
-dutname = 'rv32emu'
-refname = 'sail_cSim'
+dutname = "rv32emu"
+refname = "sail_cSim"
 
 root = os.path.abspath(os.path.dirname(__file__))
 cwd = os.getcwd()
 
-misa_A = (1 << 0)
-misa_C = (1 << 2)
-misa_E = (1 << 4)
-misa_F = (1 << 5)
-misa_I = (1 << 8)
-misa_M = (1 << 12)
+misa_A = 1 << 0
+misa_C = 1 << 2
+misa_E = 1 << 4
+misa_F = 1 << 5
+misa_I = 1 << 8
+misa_M = 1 << 12
 
-config_temp = '''[RISCOF]
+config_temp = """[RISCOF]
 ReferencePlugin={0}
 ReferencePluginPath={1}
 DUTPlugin={2}
@@ -29,4 +29,4 @@
 [{0}]
 pluginpath={1}
 path={1}
-'''
+"""

tests/arch-test-target/rv32emu/riscof_rv32emu.py

@@ -15,14 +15,15 @@
 
 logger = logging.getLogger()
 
+
 class rv32emu(pluginTemplate):
     __model__ = "rv32emu"
     __version__ = "dev"
 
     def __init__(self, *args, **kwargs):
         sclass = super().__init__(*args, **kwargs)
 
-        config = kwargs.get('config')
+        config = kwargs.get("config")
 
         # If the config node for this DUT is missing or empty. Raise an error. At minimum we need
         # the paths to the ispec and pspec files
@@ -34,25 +35,27 @@ def __init__(self, *args, **kwargs):
         # test-bench produced by a simulator (like verilator, vcs, incisive, etc). In case of an iss or
         # emulator, this variable could point to where the iss binary is located. If 'PATH variable
         # is missing in the config.ini we can hardcode the alternate here.
-        self.dut_exe = os.path.join(config['PATH'] if 'PATH' in config else "","rv32emu")
+        self.dut_exe = os.path.join(
+            config["PATH"] if "PATH" in config else "", "rv32emu"
+        )
 
         # Number of parallel jobs that can be spawned off by RISCOF
         # for various actions performed in later functions, specifically to run the tests in
         # parallel on the DUT executable. Can also be used in the build function if required.
-        self.num_jobs = str(config['jobs'] if 'jobs' in config else 1)
+        self.num_jobs = str(config["jobs"] if "jobs" in config else 1)
 
         # Path to the directory where this python file is located. Collect it from the config.ini
-        self.pluginpath=os.path.abspath(config['pluginpath'])
+        self.pluginpath = os.path.abspath(config["pluginpath"])
 
         # Collect the paths to the  riscv-config absed ISA and platform yaml files. One can choose
         # to hardcode these here itself instead of picking it from the config.ini file.
-        self.isa_spec = os.path.abspath(config['ispec'])
-        self.platform_spec = os.path.abspath(config['pspec'])
+        self.isa_spec = os.path.abspath(config["ispec"])
+        self.platform_spec = os.path.abspath(config["pspec"])
 
         # We capture if the user would like the run the tests on the target or
         # not. If you are interested in just compiling the tests and not running
         # them on the target, then following variable should be set to False
-        if 'target_run' in config and config['target_run']=='0':
+        if "target_run" in config and config["target_run"] == "0":
             self.target_run = False
         else:
             self.target_run = True
@@ -72,36 +75,57 @@ def initialise(self, suite, work_dir, archtest_env):
         # Note the march is not hardwired here, because it will change for each
         # test. Similarly the output elf name and compile macros will be assigned later in the
         # runTests function
-        self.compile_cmd = os.getenv("CROSS_COMPILE") + 'gcc -march={0}\
+        self.compile_cmd = (
+            os.getenv("CROSS_COMPILE")
+            + "gcc -march={0}\
         -static -mcmodel=medany -fvisibility=hidden -nostdlib -nostartfiles -g\
-        -T '+self.pluginpath+'/env/link.ld\
-        -I '+self.pluginpath+'/env/\
-        -I ' + archtest_env + ' {2} -o {3} {4}'
+        -T "
+            + self.pluginpath
+            + "/env/link.ld\
+        -I "
+            + self.pluginpath
+            + "/env/\
+        -I "
+            + archtest_env
+            + " {2} -o {3} {4}"
+        )
 
     def build(self, isa_yaml, platform_yaml):
         # load the isa yaml as a dictionary in python.
-        ispec = utils.load_yaml(isa_yaml)['hart0']
+        ispec = utils.load_yaml(isa_yaml)["hart0"]
 
         # capture the XLEN value by picking the max value in 'supported_xlen' field of isa yaml. This
         # will be useful in setting integer value in the compiler string (if not already hardcoded);
-        self.xlen = ('64' if 64 in ispec['supported_xlen'] else '32')
-
-        if 'E' not in ispec['ISA']:
-            self.compile_cmd = self.compile_cmd+' -mabi='+('lp64 ' if 64 in ispec['supported_xlen'] else 'ilp32 ')
+        self.xlen = "64" if 64 in ispec["supported_xlen"] else "32"
+
+        if "E" not in ispec["ISA"]:
+            self.compile_cmd = (
+                self.compile_cmd
+                + " -mabi="
+                + ("lp64 " if 64 in ispec["supported_xlen"] else "ilp32 ")
+            )
         else:
-            self.compile_cmd = self.compile_cmd+' -mabi='+('lp64e ' if 64 in ispec['supported_xlen'] else 'ilp32e ')
-            self.compile_cmd += '-D RV32E '
+            self.compile_cmd = (
+                self.compile_cmd
+                + " -mabi="
+                + ("lp64e " if 64 in ispec["supported_xlen"] else "ilp32e ")
+            )
+            self.compile_cmd += "-D RV32E "
 
     def runTests(self, testList):
         # Delete Makefile if it already exists.
-        if os.path.exists(self.work_dir+ "/Makefile." + self.name[:-1]):
-            os.remove(self.work_dir+ "/Makefile." + self.name[:-1])
+        if os.path.exists(self.work_dir + "/Makefile." + self.name[:-1]):
+            os.remove(self.work_dir + "/Makefile." + self.name[:-1])
         # create an instance the makeUtil class that we will use to create targets.
-        make = utils.makeUtil(makefilePath=os.path.join(self.work_dir, "Makefile." + self.name[:-1]))
+        make = utils.makeUtil(
+            makefilePath=os.path.join(
+                self.work_dir, "Makefile." + self.name[:-1]
+            )
+        )
 
         # set the make command that will be used. The num_jobs parameter was set in the __init__
         # function earlier
-        make.makeCommand = 'make -j' + self.num_jobs
+        make.makeCommand = "make -j" + self.num_jobs
 
         # we will iterate over each entry in the testList. Each entry node will be refered to by the
         # variable testname.
@@ -110,14 +134,14 @@ def runTests(self, testList):
             testentry = testList[testname]
 
             # we capture the path to the assembly file of this test
-            test = testentry['test_path']
+            test = testentry["test_path"]
 
             # capture the directory where the artifacts of this test will be dumped/created. RISCOF is
             # going to look into this directory for the signature files
-            test_dir = testentry['work_dir']
+            test_dir = testentry["work_dir"]
 
             # name of the elf file after compilation of the test
-            elf = 'my.elf'
+            elf = "my.elf"
 
             # name of the signature file as per requirement of RISCOF. RISCOF expects the signature to
             # be named as DUT-<dut-name>.signature. The below variable creates an absolute path of
@@ -127,22 +151,26 @@ def runTests(self, testList):
             # for each test there are specific compile macros that need to be enabled. The macros in
             # the testList node only contain the macros/values. For the gcc toolchain we need to
             # prefix with "-D". The following does precisely that.
-            compile_macros = ' -D' + " -D".join(testentry['macros'])
+            compile_macros = " -D" + " -D".join(testentry["macros"])
 
             # substitute all variables in the compile command that we created in the initialize function
-            cmd = self.compile_cmd.format(testentry['isa'].lower(), self.xlen, test, elf, compile_macros)
+            cmd = self.compile_cmd.format(
+                testentry["isa"].lower(), self.xlen, test, elf, compile_macros
+            )
 
-     	    # if the user wants to disable running the tests and only compile the tests, then
+            # if the user wants to disable running the tests and only compile the tests, then
             # the "else" clause is executed below assigning the sim command to simple no action
             # echo statement.
             if self.target_run:
-            # set up the simulation command. Template is for spike. Please change.
-                simcmd = self.dut_exe + ' -a {0} {1}'.format(sig_file, elf)
+                # set up the simulation command. Template is for spike. Please change.
+                simcmd = self.dut_exe + " -a {0} {1}".format(sig_file, elf)
             else:
                 simcmd = 'echo "NO RUN"'
 
             # concatenate all commands that need to be executed within a make-target.
-            execute = '@cd {0}; {1}; {2};'.format(testentry['work_dir'], cmd, simcmd)
+            execute = "@cd {0}; {1}; {2};".format(
+                testentry["work_dir"], cmd, simcmd
+            )
 
             # create a target. The makeutil will create a target with the name "TARGET<num>" where num
             # starts from 0 and increments automatically for each new target that is added