diff --git a/.github/workflows/draft_release.yml b/.github/workflows/draft_release.yml index e3bd37f..2908866 100644 --- a/.github/workflows/draft_release.yml +++ b/.github/workflows/draft_release.yml @@ -13,7 +13,16 @@ jobs: new-release: name: "Draft a new release" runs-on: ubuntu-latest + env: + RELEASE_TAG: ${{ github.event.inputs.tag }} steps: + - name: Validate release tag format + run: | + if ! echo "$RELEASE_TAG" | grep -qE '^v[0-9]+\.[0-9]+\.[0-9]+$'; then + echo "Error: Tag must be in format vXX.YY.ZZ" + exit 1 + fi + - uses: actions/checkout@v4 - name: Setup Python @@ -26,7 +35,7 @@ jobs: - name: Update CHANGELOG run: | python3 -m pip install mdformat-gfm 'git+https://github.com/Takishima/keepachangelog@v1.0.1' - python3 -m keepachangelog release "${{ github.event.inputs.tag }}" + python3 -m keepachangelog release "$RELEASE_TAG" python3 -m mdformat CHANGELOG.md - name: Commit changes diff --git a/.github/workflows/format.yml b/.github/workflows/format.yml index 7fdbcc9..0ab4f7e 100644 --- a/.github/workflows/format.yml +++ b/.github/workflows/format.yml @@ -28,16 +28,18 @@ jobs: curl -LsSf "${source_url}/${parse_changelog_tag}/parse-changelog-${target}.tar.gz" | tar xzf - - name: Extract version from branch name (for release branches) if: github.event_name == 'pull_request' && startsWith(github.event.pull_request.head.ref, 'release/') + env: + BRANCH_NAME: ${{ github.event.pull_request.head.ref }} run: | - BRANCH_NAME="${{ github.event.pull_request.head.ref }}" VERSION=${BRANCH_NAME#release/v} echo "RELEASE_VERSION=$VERSION" >> $GITHUB_ENV git tag v${RELEASE_VERSION} - name: Extract version from branch name (for hotfix branches) if: github.event_name == 'pull_request' && startsWith(github.event.pull_request.head.ref, 'hotfix/') + env: + BRANCH_NAME: ${{ github.event.pull_request.head.ref }} run: | - BRANCH_NAME="${{ github.event.pull_request.head.ref }}" VERSION=${BRANCH_NAME#hotfix/v} echo "RELEASE_VERSION=$VERSION" >> $GITHUB_ENV git tag v${RELEASE_VERSION} diff --git a/.github/workflows/publish_release.yml b/.github/workflows/publish_release.yml index 1e8f683..e04701b 100644 --- a/.github/workflows/publish_release.yml +++ b/.github/workflows/publish_release.yml @@ -39,16 +39,18 @@ jobs: - name: Extract version from branch name (for release branches) if: github.event_name == 'pull_request' && startsWith(github.event.pull_request.head.ref, 'release/') + env: + BRANCH_NAME: ${{ github.event.pull_request.head.ref }} run: | - BRANCH_NAME="${{ github.event.pull_request.head.ref }}" VERSION=${BRANCH_NAME#release/} VERSION=${VERSION#v} echo "RELEASE_VERSION=$VERSION" >> $GITHUB_ENV - name: Extract version from branch name (for hotfix branches) if: github.event_name == 'pull_request' && startsWith(github.event.pull_request.head.ref, 'hotfix/') + env: + BRANCH_NAME: ${{ github.event.pull_request.head.ref }} run: | - BRANCH_NAME="${{ github.event.pull_request.head.ref }}" VERSION=${BRANCH_NAME#hotfix/} VERSION=${VERSION#v} echo "RELEASE_VERSION=$VERSION" >> $GITHUB_ENV diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index affd387..c981ba1 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -31,8 +31,10 @@ jobs: python3 -m pip install 'git+https://github.com/Takishima/pre-commit-changelog-auto-update@v1.0.0' - name: Run Python script + env: + PR_BODY: ${{ github.event.pull_request.body }} run: | - python3 -m update_changelog --pr-body "${{ github.event.pull_request.body }}" + python3 -m update_changelog --pr-body "$PR_BODY" - name: Commit changes id: commit diff --git a/CHANGELOG.md b/CHANGELOG.md index 0445305..7a57394 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Repository +- Fix SonarCloud violations: + - Fix return type annotation for `_load_from_toml` method in `_argparse.py` + - Reduce cognitive complexity in `_cmake.py` by extracting helper methods + - Reduce cognitive complexity in `_cmake_test.py` by extracting test helpers + - Fix variable naming conventions in test files + - Fix GitHub Actions security issues by using environment variables for user-controlled data + - Add input validation for workflow_dispatch parameters + - Fix shell script issues in `run_tests.sh` (use `[[` instead of `[`, add return statements) - Fixed Windows CI by using pip to install cppcheck (Chocolatey package is broken) - Modernize ruff configuration to follow latest guidelines - Remove deprecated settings (`target-version`, `ANN101`, `ASYNC1`, etc.) diff --git a/cmake_pc_hooks/_argparse.py b/cmake_pc_hooks/_argparse.py index 205940d..b3ba62f 100644 --- a/cmake_pc_hooks/_argparse.py +++ b/cmake_pc_hooks/_argparse.py @@ -293,7 +293,7 @@ def _load_from_toml( # noqa: PLR0913 path_must_exist: bool = True, section_must_exist: bool = True, overridable_keys: set | None = None, - ) -> None: + ) -> argparse.Namespace: """ Load a TOML file and set the attributes within the argparse namespace object. diff --git a/cmake_pc_hooks/_cmake.py b/cmake_pc_hooks/_cmake.py index 7d0e553..b241bb4 100644 --- a/cmake_pc_hooks/_cmake.py +++ b/cmake_pc_hooks/_cmake.py @@ -298,40 +298,8 @@ def resolve_build_directory(self, build_dir_list=None, *, automatic_discovery=Tr self.build_dir, ) - def setup_cmake_args(self, cmake_args): # noqa: C901 - """ - Setup CMake arguments. - - Args: - cmake_args: Dictionary-like data structure with following keys: - - 'defines': list[str] - - 'undefined': list[str] - - 'errors': list[str] - - 'no_errors': list[str] - - 'generator': str - - 'toolset': str - - 'platform': str - - 'preset': str - - 'cmake': str - - 'dev_warnings': bool - - 'no_dev_warnings': bool - - 'linux': list[str] - - 'mac': list[str] - - 'win': list[str] - """ - self.source_dir = Path(cmake_args.source_dir).resolve() - if cmake_args.cmake: - self.command = [Path(cmake_args.cmake).resolve()] - self.no_cmake_configure = cmake_args.no_cmake_configure - - self.resolve_build_directory( - build_dir_list=cmake_args.build_dir, - automatic_discovery=cmake_args.automatic_discovery, - ) - - if cmake_args.detect_configured_files and self.build_dir: - self.cmake_trace_log = self.build_dir / self.DEFAULT_TRACE_LOG - + def _process_keyword_cmake_args(self, cmake_args): + """Process keyword-style CMake arguments.""" keyword_args = { 'defines': ([], '-D{}'), 'undefines': ([], '-U{}'), @@ -352,6 +320,8 @@ def setup_cmake_args(self, cmake_args): # noqa: C901 for element in value: self.cmake_args.append(format_str.format(element)) + def _process_flag_cmake_args(self, cmake_args): + """Process flag-style CMake arguments.""" flag_args = { 'dev_warnings': (False, '-Wdev'), 'no_dev_warnings': (False, '-Wno_dev'), @@ -360,18 +330,58 @@ def setup_cmake_args(self, cmake_args): # noqa: C901 if getattr(cmake_args, key, default): self.cmake_args.append(flag_str) - platform_args = { + def _process_platform_cmake_args(self, cmake_args): + """Process platform-specific CMake arguments.""" + platform_args_config = { 'linux': ([], 'Linux'), 'mac': ([], 'Darwin'), 'win': ([], 'Windows'), } - for key, (default, platform_name) in platform_args.items(): - platform_args = getattr(cmake_args, key, default) - if platform.system() == platform_name and platform_args: - for arg in platform_args: + for key, (default, platform_name) in platform_args_config.items(): + platform_arg_values = getattr(cmake_args, key, default) + if platform.system() == platform_name and platform_arg_values: + for arg in platform_arg_values: self.cmake_args.append(arg.strip('"\'')) + def setup_cmake_args(self, cmake_args): + """ + Setup CMake arguments. + + Args: + cmake_args: Dictionary-like data structure with following keys: + - 'defines': list[str] + - 'undefined': list[str] + - 'errors': list[str] + - 'no_errors': list[str] + - 'generator': str + - 'toolset': str + - 'platform': str + - 'preset': str + - 'cmake': str + - 'dev_warnings': bool + - 'no_dev_warnings': bool + - 'linux': list[str] + - 'mac': list[str] + - 'win': list[str] + """ + self.source_dir = Path(cmake_args.source_dir).resolve() + if cmake_args.cmake: + self.command = [Path(cmake_args.cmake).resolve()] + self.no_cmake_configure = cmake_args.no_cmake_configure + + self.resolve_build_directory( + build_dir_list=cmake_args.build_dir, + automatic_discovery=cmake_args.automatic_discovery, + ) + + if cmake_args.detect_configured_files and self.build_dir: + self.cmake_trace_log = self.build_dir / self.DEFAULT_TRACE_LOG + + self._process_keyword_cmake_args(cmake_args) + self._process_flag_cmake_args(cmake_args) + self._process_platform_cmake_args(cmake_args) + def configure(self, command, *, clean_build=False): """ Run a CMake configure step (multi-process safe). diff --git a/tests/python/_cmake_test.py b/tests/python/_cmake_test.py index d81e8c0..d60a814 100644 --- a/tests/python/_cmake_test.py +++ b/tests/python/_cmake_test.py @@ -164,20 +164,27 @@ def test_resolve_build_directory(tmp_path, dir_list, build_dir_tree, ref_path): assert cmake.build_dir == tmp_path / cmake.DEFAULT_BUILD_DIR if dir_list is None else dir_list[0] -@pytest.mark.parametrize('system', ['Linux', 'Darwin', 'Windows']) -@pytest.mark.parametrize('no_cmake_configure', [False, True]) -def test_setup_cmake_args( # noqa: PLR0912, PLR0915, C901 - mocker, system, no_cmake_configure -): - original_system = platform.system() - - def system_stub(): - return system - - mocker.patch('platform.system', system_stub) - - cmake = CMakeCommand() - +def _assert_args_in_cmake_args(cmake_args, items, format_str='{}'): + """Helper to verify items are present in cmake_args.""" + for item in items: + expected = format_str.format(item) + assert any(expected in arg for arg in cmake_args), f'Expected {expected} in cmake_args' + + +def _assert_platform_args(cmake_args, args): + """Helper to verify platform-specific args.""" + platform_config = { + 'Linux': args.linux, + 'Darwin': args.mac, + 'Windows': args.win, + } + current_platform = platform.system() + if current_platform in platform_config: + _assert_args_in_cmake_args(cmake_args, platform_config[current_platform]) + + +def _create_test_args(original_system, no_cmake_configure): + """Create test arguments namespace.""" args = argparse.Namespace() args.detect_configured_files = True args.no_cmake_configure = no_cmake_configure @@ -202,6 +209,17 @@ def system_stub(): args.linux = ['LNX_A', 'LNX_B'] args.mac = ['MAC_A', 'MAC_B'] args.win = ['WIN_A', 'WIN_B'] + return args + + +@pytest.mark.parametrize('system', ['Linux', 'Darwin', 'Windows']) +@pytest.mark.parametrize('no_cmake_configure', [False, True]) +def test_setup_cmake_args(mocker, system, no_cmake_configure): + original_system = platform.system() + mocker.patch('platform.system', return_value=system) + + cmake = CMakeCommand() + args = _create_test_args(original_system, no_cmake_configure) assert cmake.source_dir is None assert cmake.build_dir is None @@ -220,7 +238,6 @@ def system_stub(): assert '-GNinja' in cmake.cmake_args assert '-A64' in cmake.cmake_args assert '-Tclang-toolset' in cmake.cmake_args - assert '-Wdev' in cmake.cmake_args assert '-Wno_dev' in cmake.cmake_args @@ -228,24 +245,11 @@ def system_stub(): assert '--trace-expand' not in cmake.cmake_args assert '--trace-format=json-v1' not in cmake.cmake_args - for define in args.defines: - assert any(define in arg for arg in cmake.cmake_args) - for undefine in args.undefines: - assert any(undefine in arg for arg in cmake.cmake_args) - for error in args.errors: - assert any(f'-Werror={error}' in arg for arg in cmake.cmake_args) - for no_error in args.no_errors: - assert any(f'-Wno-error={no_error}' in arg for arg in cmake.cmake_args) - - if platform.system() == 'Linux': - for linux in args.linux: - assert any(linux in arg for arg in cmake.cmake_args) - elif platform.system() == 'Darwin': - for mac in args.mac: - assert any(mac in arg for arg in cmake.cmake_args) - elif platform.system() == 'Windows': - for win in args.win: - assert any(win in arg for arg in cmake.cmake_args) + _assert_args_in_cmake_args(cmake.cmake_args, args.defines) + _assert_args_in_cmake_args(cmake.cmake_args, args.undefines) + _assert_args_in_cmake_args(cmake.cmake_args, args.errors, '-Werror={}') + _assert_args_in_cmake_args(cmake.cmake_args, args.no_errors, '-Wno-error={}') + _assert_platform_args(cmake.cmake_args, args) @pytest.mark.parametrize('detect_configured_files', [False, True], ids=['no_trace', 'w_trace']) @@ -261,12 +265,10 @@ def test_configure_cmake( # noqa: PLR0917 detect_configured_files, ): sys_exit = mocker.patch('sys.exit') - FileLock = mocker.MagicMock(filelock.FileLock) # noqa: N806 - mocker.patch(filelock_module_name, FileLock) - InterProcessReaderWriterLock = mocker.MagicMock( # noqa: N806 - fasteners.InterProcessReaderWriterLock - ) - mocker.patch(interprocess_rw_lock_module_name, InterProcessReaderWriterLock) + mock_file_lock = mocker.MagicMock(filelock.FileLock) + mocker.patch(filelock_module_name, mock_file_lock) + mock_rw_lock = mocker.MagicMock(fasteners.InterProcessReaderWriterLock) + mocker.patch(interprocess_rw_lock_module_name, mock_rw_lock) configure = mocker.Mock(return_value=returncode) mocker.patch(internal_cmake_configure_name, configure) parse_log = mocker.patch('cmake_pc_hooks._cmake.CMakeCommand._parse_cmake_trace_log') @@ -289,17 +291,17 @@ def test_configure_cmake( # noqa: PLR0917 # ---------------------------------- if no_cmake_configure: - FileLock.assert_not_called() - InterProcessReaderWriterLock.assert_not_called() + mock_file_lock.assert_not_called() + mock_rw_lock.assert_not_called() configure.assert_not_called() return - FileLock.assert_called_once_with(build_dir / '_cmake_configure_try_lock') - InterProcessReaderWriterLock.assert_called_once_with(build_dir / '_cmake_configure_lock') + mock_file_lock.assert_called_once_with(build_dir / '_cmake_configure_try_lock') + mock_rw_lock.assert_called_once_with(build_dir / '_cmake_configure_lock') configure.assert_called_once_with( lock_files=( - InterProcessReaderWriterLock.call_args[0][0], - FileLock.call_args[0][0], + mock_rw_lock.call_args[0][0], + mock_file_lock.call_args[0][0], ), clean_build=clean_build, ) @@ -324,14 +326,12 @@ def timeout(blocking): # noqa: ARG001 args = {'acquire.side_effect': timeout} file_lock = mocker.MagicMock(filelock.FileLock, **args) - FileLock = mocker.MagicMock(filelock.FileLock, return_value=file_lock) # noqa: N806 - mocker.patch(filelock_module_name, FileLock) + mock_file_lock = mocker.MagicMock(filelock.FileLock, return_value=file_lock) + mocker.patch(filelock_module_name, mock_file_lock) interprocess_lock = mocker.MagicMock() - InterProcessReaderWriterLock = mocker.MagicMock( # noqa: N806 - return_value=interprocess_lock - ) - mocker.patch(interprocess_rw_lock_module_name, InterProcessReaderWriterLock) + mock_rw_lock = mocker.MagicMock(return_value=interprocess_lock) + mocker.patch(interprocess_rw_lock_module_name, mock_rw_lock) configure = mocker.Mock(return_value=0) mocker.patch(internal_cmake_configure_name, configure) @@ -349,8 +349,8 @@ def timeout(blocking): # noqa: ARG001 # ---------------------------------- - FileLock.assert_called_once() - InterProcessReaderWriterLock.assert_called_once() + mock_file_lock.assert_called_once() + mock_rw_lock.assert_called_once() interprocess_lock.read_lock.assert_called_once() configure.assert_not_called() @@ -358,12 +358,10 @@ def timeout(blocking): # noqa: ARG001 def test_configure_invalid(mocker): mocker.patch('sys.exit', side_effect=ExitError) - FileLock = mocker.MagicMock(filelock.FileLock) # noqa: N806 - mocker.patch(filelock_module_name, FileLock) - InterProcessReaderWriterLock = mocker.MagicMock( # noqa: N806 - fasteners.InterProcessReaderWriterLock - ) - mocker.patch(interprocess_rw_lock_module_name, InterProcessReaderWriterLock) + mock_file_lock = mocker.MagicMock(filelock.FileLock) + mocker.patch(filelock_module_name, mock_file_lock) + mock_rw_lock = mocker.MagicMock(fasteners.InterProcessReaderWriterLock) + mocker.patch(interprocess_rw_lock_module_name, mock_rw_lock) configure = mocker.Mock(return_value=1) mocker.patch(internal_cmake_configure_name, configure) diff --git a/tests/run_tests.sh b/tests/run_tests.sh index db28ce7..ae59f75 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -15,31 +15,27 @@ function run_hook() mkdir -p "$build_dir" rm -rf "${build_dir:?}"/* - if [ "$gen_compiledb" -eq 1 ]; then + if [[ "$gen_compiledb" -eq 1 ]]; then $hook -S "$src_dir" -B "$build_dir" "$@" else $hook "$@" fi ret=$? - if [ "$expect_success" -eq 1 ]; then - if [ $ret -ne 0 ]; then - echo "Hook $hook failed on source directory $src_dir with exit code $ret!" - exit 1 - fi - else - if [ "$ret" -eq 0 ]; then - echo "Hook $hook unexpectedly succeeded on source directory $src_dir with exit code $ret!" - exit 1 - fi + if [[ "$expect_success" -eq 1 && $ret -ne 0 ]]; then + echo "Hook $hook failed on source directory $src_dir with exit code $ret!" + exit 1 + elif [[ "$expect_success" -ne 1 && "$ret" -eq 0 ]]; then + echo "Hook $hook unexpectedly succeeded on source directory $src_dir with exit code $ret!" + exit 1 fi - if [ "$gen_compiledb" -eq 1 ]; then - if [ ! -f $build_dir/compile_commands.json ]; then - echo "Hook $hook failed to generate the compile database for $src_dir!" - exit 1 - fi + if [[ "$gen_compiledb" -eq 1 && ! -f $build_dir/compile_commands.json ]]; then + echo "Hook $hook failed to generate the compile database for $src_dir!" + exit 1 fi + + return 0 } # ============================================================================== @@ -56,7 +52,7 @@ mkdir -p "$build_dir" rm -rf "${build_dir:?}"/* cmake -B other_build -S tests/cmake_good cmake-pc-cppcheck-hook -S tests/cmake_good -B build -B other_build tests/cmake_good/good.cpp -if [ ! -f other_build/compile_commands.json ]; then +if [[ ! -f other_build/compile_commands.json ]]; then echo "Failed to generate compile database in other_build" exit 1 fi @@ -74,7 +70,7 @@ run_hook cmake-pc-lizard-hook tests/cmake_bad 0 0 -C4 tests/cmake_bad/bad.cpp export CC=clang CXX=clang++ -if [ -z "$GITHUB_SHA" ]; then +if [[ -z "$GITHUB_SHA" ]]; then LATEST_SHA=$(git rev-parse HEAD) else LATEST_SHA=$GITHUB_SHA @@ -87,6 +83,7 @@ call_sed() else sed "$@" fi + return $? } @@ -100,7 +97,7 @@ git init \ && pre-commit run --all-files && success=1 || success=0 rm -rf .git -if [ "$success" -eq 0 ]; then +if [[ "$success" -eq 0 ]]; then exit 1 fi popd || exit @@ -115,7 +112,7 @@ git init \ && pre-commit run --all-files && success=0 || success=1 rm -rf .git -if [ "$success" -eq 0 ]; then +if [[ "$success" -eq 0 ]]; then exit 1 fi popd || exit