diff --git a/.config/nextest.toml b/.config/nextest.toml index 00d072c..bc5838d 100644 --- a/.config/nextest.toml +++ b/.config/nextest.toml @@ -15,7 +15,7 @@ slow-timeout = "10s" # This is all tests in tests/ folder + unit test for --extra-args. default-filter = "kind(test) + test(#*use_extra_args)" -# show wich tests were skipped +# show which tests were skipped status-level = "skip" # show log output from each test diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml new file mode 100644 index 0000000..f6dd2c2 --- /dev/null +++ b/.github/workflows/benchmark.yml @@ -0,0 +1,55 @@ +name: Benchmark + +on: + push: + branches: [main] + paths: + - cpp-linter/src/ + - cpp-linter/benches/ + - cpp-linter/Cargo.toml + - Cargo.toml + - Cargo.lock + - .github/workflows/benchmark.yml + tags-ignore: ['*'] + pull_request: + branches: [main] + paths: + - cpp-linter/src/ + - cpp-linter/benches/ + - cpp-linter/Cargo.toml + - Cargo.toml + - Cargo.lock + - .github/workflows/benchmark.yml + # `workflow_dispatch` allows CodSpeed to trigger back-test + # performance analysis in order to generate initial data. + workflow_dispatch: + +jobs: + benchmark: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + # using the generated compilation database, + # we will use cpp-linter to scan libgit2 src/libgit2/**.c files. + - name: Checkout libgit2 + uses: actions/checkout@v4 + with: + repository: libgit2/libgit2 + ref: v1.8.1 + path: cpp-linter/benches/libgit2 + - name: Generate compilation database + working-directory: cpp-linter/benches/libgit2 + run: | + mkdir build && cd build + cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=ON .. + - name: Install cargo-binstall + uses: cargo-bins/cargo-binstall@main + - name: Install cargo-codspeed + run: cargo binstall -y cargo-codspeed + - name: Build the benchmark target(s) + run: cargo codspeed build + - name: Run benchmarks + uses: CodSpeedHQ/action@v3 + with: + run: cargo codspeed run + token: ${{ secrets.CODSPEED_TOKEN }} diff --git a/.github/workflows/perf-test.yml b/.github/workflows/perf-test.yml deleted file mode 100644 index f6ac7ad..0000000 --- a/.github/workflows/perf-test.yml +++ /dev/null @@ -1,146 +0,0 @@ -name: Performance Regression - -on: - push: - branches: [main] - paths: - - cpp-linter/src/** - - cpp-linter/Cargo.toml - - Cargo.toml - - Cargo.lock - - .github/workflows/perf-test.yml - - .github/workflows/bench.py - tags-ignore: ['*'] - pull_request: - branches: [main] - paths: - - cpp-linter/src/** - - cpp-linter/Cargo.toml - - Cargo.toml - - Cargo.lock - - .github/workflows/perf* -jobs: - build: - name: Build ${{ matrix.name }} - runs-on: ubuntu-latest - strategy: - matrix: - include: - - commit: ${{ github.sha }} - name: current - - commit: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || github.event.before }} - name: previous - outputs: - cached-previous: ${{ steps.is-cached-previous.outputs.is-cached == 'true' && steps.validate.outputs.cache-valid != 'false' }} - cached-current: ${{ steps.is-cached-current.outputs.is-cached == 'true' && steps.validate.outputs.cache-valid != 'false' }} - env: - BIN: target/release/cpp-linter - steps: - - name: Checkout ${{ matrix.name }} - uses: actions/checkout@v4 - with: - ref: ${{ matrix.commit }} - - name: Cache base ref build - uses: actions/cache@v4 - id: cache - with: - key: bin-cache-${{ hashFiles('cpp-linter/src/**', 'Cargo.toml', 'Cargo.lock', 'cpp-linter/Cargo.toml') }} - path: ${{ env.BIN }} - - name: Is previous cached? - if: matrix.name == 'previous' - id: is-cached-previous - run: echo "is-cached=${{ steps.cache.outputs.cache-hit }}" >> "$GITHUB_OUTPUT" - - name: Is current cached? - if: matrix.name == 'current' - id: is-cached-current - run: echo "is-cached=${{ steps.cache.outputs.cache-hit }}" >> "$GITHUB_OUTPUT" - - name: Validate cached binary - if: steps.cache.outputs.cache-hit == 'true' - id: validate - run: | - chmod +x ${{ env.BIN }} - if ! ${{ env.BIN }} version; then - echo "Cached binary is invalid, rebuilding..." - echo "cache-valid=false" >> "$GITHUB_OUTPUT" - fi - - run: rustup update --no-self-update - if: steps.cache.outputs.cache-hit != 'true' || steps.validate.outputs.cache-valid == 'false' - - run: cargo build --bin cpp-linter --release - if: steps.cache.outputs.cache-hit != 'true' || steps.validate.outputs.cache-valid == 'false' - - name: Upload build artifact - uses: actions/upload-artifact@v4 - with: - name: ${{ matrix.name }} - path: ${{ env.BIN }} - - benchmark: - name: Measure Performance Difference - needs: [build] - if: ${{ !needs.build.outputs.cached-current || !needs.build.outputs.cached-previous }} - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - name: Checkout libgit2 - uses: actions/checkout@v4 - with: - repository: libgit2/libgit2 - ref: v1.8.1 - path: libgit2 - - name: Download built binaries - uses: actions/download-artifact@v4 - - name: Make binaries executable - run: chmod +x ./*/cpp-linter - - name: Generate compilation database - working-directory: libgit2 - run: | - mkdir build && cd build - cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=ON .. - - name: Install cargo-binstall - uses: cargo-bins/cargo-binstall@main - - name: Install hyperfine - run: cargo binstall -y hyperfine - - uses: actions/setup-python@v5 - with: - python-version: 3.x - - run: pip install 'cpp-linter < 2.0' - - name: Warmup and list files - env: - CPP_LINTER_COLOR: true - working-directory: libgit2 - # Use previous build for stability. This will - # - create the .cpp-linter_cache folder - # - list the files concerning the benchmark test - # NOTE: This does not actually invoke clang tools. - run: ../previous/cpp-linter -l 0 -p build -i='|!src/libgit2' -s="" -c="-*" -e c - - name: Run hyperfine tool - # using the generated compilation database, - # we will use cpp-linter (both builds) to scan libgit2 src/libgit2/**.c files. - working-directory: libgit2 - run: >- - hyperfine - --runs 2 - --style color - --export-markdown '${{ runner.temp }}/benchmark.md' - --export-json '${{ runner.temp }}/benchmark.json' - --command-name=previous-build - "../previous/cpp-linter -l 0 -p build -i='|!src/libgit2' -e c" - --command-name=current-build - "../current/cpp-linter -l 0 -p build -i='|!src/libgit2' -e c" - --command-name=pure-python - "cpp-linter -l false -j 0 -p build -i='|!src/libgit2' -e c" - - name: Append report to job summary - run: cat ${{ runner.temp }}/benchmark.md >> "$GITHUB_STEP_SUMMARY" - - name: Upload JSON results - uses: actions/upload-artifact@v4 - with: - name: benchmark-json - path: ${{ runner.temp }}/benchmark.json - - name: Annotate summary - run: python .github/workflows/perf_annotate.py "${{ runner.temp }}/benchmark.json" - - report-no-src-changes: - runs-on: ubuntu-latest - needs: [build] - if: needs.build.outputs.cached-current && needs.build.outputs.cached-previous - steps: - - run: echo "::notice title=No benchmark performed::No changes to cpp-linter source code detected." diff --git a/.github/workflows/perf_annotate.py b/.github/workflows/perf_annotate.py deleted file mode 100644 index 072c9b0..0000000 --- a/.github/workflows/perf_annotate.py +++ /dev/null @@ -1,68 +0,0 @@ -import argparse -import json -from os import environ -from pathlib import Path -from typing import List, Any, Dict, cast - - -class Args(argparse.Namespace): - json_file: Path - - -def main(): - arg_parser = argparse.ArgumentParser() - arg_parser.add_argument("json_file", type=Path) - arg_parser.parse_args(namespace=Args) - - bench_json = Args.json_file.read_text(encoding="utf-8") - bench: List[Dict[str, Any]] = json.loads(bench_json)["results"] - - assert len(bench) == 3 - old_mean, new_mean = (None, None) - for result in bench: - mean = cast(float, result["mean"]) - if result["command"] == "previous-build": - old_mean = mean - elif result["command"] == "current-build": - new_mean = mean - - assert old_mean is not None, "benchmark report has no result for previous-build" - assert new_mean is not None, "benchmark report has no result for current-build" - - diff = round(new_mean - old_mean, 2) - scalar = int((new_mean - old_mean) / old_mean * 100) - - output = [] - if diff > 2: - output.extend( - [ - "> [!CAUTION]", - "> Detected a performance regression in new changes:", - ] - ) - elif diff < -2: - output.extend( - [ - "> [!TIP]", - "> Detected a performance improvement in new changes:", - ] - ) - else: - output.extend( - [ - "> [!NOTE]", - "> Determined a negligible difference in performance with new changes:", - ] - ) - output[-1] += f" {diff}s ({scalar} %)" - annotation = "\n".join(output) - - if "GITHUB_STEP_SUMMARY" in environ: - with open(environ["GITHUB_STEP_SUMMARY"], "a") as summary: - summary.write(f"\n{annotation}\n") - else: - print(annotation) - - -if __name__ == "__main__": - main() diff --git a/.gitignore b/.gitignore index 4c683a4..f79fbe0 100644 --- a/.gitignore +++ b/.gitignore @@ -346,3 +346,4 @@ docs/site cpp-linter-py/docs/cli_args.rst lcov.info coverage.json +cpp-linter/benches/libgit2/ diff --git a/Cargo.lock b/Cargo.lock index b09bf4a..8acf807 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -41,6 +41,12 @@ dependencies = [ "libc", ] +[[package]] +name = "anes" +version = "0.1.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4b46cbb362ab8752921c97e041f5e366ee6297bd428a31275b9fcf1e380f7299" + [[package]] name = "anstream" version = "0.6.15" @@ -163,6 +169,12 @@ version = "1.7.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "428d9aa8fbc0670b7b8d6030a7fadd0f86151cae55e4dbbece15f3780a3dfaf3" +[[package]] +name = "cast" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "37b2a672a2cb129a2e41c10b1224bb368f9f37a2b16b612598138befd7b37eb5" + [[package]] name = "cc" version = "1.1.22" @@ -194,6 +206,33 @@ dependencies = [ "windows-targets 0.52.6", ] +[[package]] +name = "ciborium" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "42e69ffd6f0917f5c029256a24d0161db17cea3997d185db0d35926308770f0e" +dependencies = [ + "ciborium-io", + "ciborium-ll", + "serde", +] + +[[package]] +name = "ciborium-io" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "05afea1e0a06c9be33d539b876f1ce3692f4afea2cb41f740e7743225ed1c757" + +[[package]] +name = "ciborium-ll" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "57663b653d948a338bfb3eeba9bb2fd5fcfaecb9e199e87e1eda4d9e8b240fd9" +dependencies = [ + "ciborium-io", + "half", +] + [[package]] name = "clap" version = "4.5.23" @@ -229,6 +268,30 @@ dependencies = [ "pyo3", ] +[[package]] +name = "codspeed" +version = "2.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "450a0e9df9df1c154156f4344f99d8f6f6e69d0fc4de96ef6e2e68b2ec3bce97" +dependencies = [ + "colored", + "libc", + "serde_json", +] + +[[package]] +name = "codspeed-criterion-compat" +version = "2.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8eb1a6cb9c20e177fde58cdef97c1c7c9264eb1424fe45c4fccedc2fb078a569" +dependencies = [ + "codspeed", + "colored", + "criterion", + "futures", + "tokio", +] + [[package]] name = "colorchoice" version = "1.0.2" @@ -277,6 +340,7 @@ dependencies = [ "anyhow", "chrono", "clap", + "codspeed-criterion-compat", "colored", "fast-glob", "futures", @@ -319,6 +383,75 @@ dependencies = [ "tokio", ] +[[package]] +name = "criterion" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f2b12d017a929603d80db1831cd3a24082f8137ce19c69e6447f54f5fc8d692f" +dependencies = [ + "anes", + "cast", + "ciborium", + "clap", + "criterion-plot", + "futures", + "is-terminal", + "itertools", + "num-traits", + "once_cell", + "oorandom", + "plotters", + "rayon", + "regex", + "serde", + "serde_derive", + "serde_json", + "tinytemplate", + "tokio", + "walkdir", +] + +[[package]] +name = "criterion-plot" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6b50826342786a51a89e2da3a28f1c32b06e387201bc2d19791f622c673706b1" +dependencies = [ + "cast", + "itertools", +] + +[[package]] +name = "crossbeam-deque" +version = "0.8.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9dd111b7b7f7d55b72c0a6ae361660ee5853c9af73f70c3c2ef6858b950e2e51" +dependencies = [ + "crossbeam-epoch", + "crossbeam-utils", +] + +[[package]] +name = "crossbeam-epoch" +version = "0.9.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5b82ac4a3c2ca9c3460964f020e1402edd5753411d7737aa39c3714ad1b5420e" +dependencies = [ + "crossbeam-utils", +] + +[[package]] +name = "crossbeam-utils" +version = "0.8.21" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d0a5c400df2834b80a4c3327b3aad3a4c4cd4de0629063962b03235697506a28" + +[[package]] +name = "crunchy" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7a81dae078cea95a014a339291cec439d2f232ebe854a9d672b796c6afafa9b7" + [[package]] name = "ctor" version = "0.2.8" @@ -542,6 +675,16 @@ dependencies = [ "tracing", ] +[[package]] +name = "half" +version = "2.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6dd08c532ae367adf81c312a4580bc67f1d0fe8bc9c460520283f4c0ff277888" +dependencies = [ + "cfg-if", + "crunchy", +] + [[package]] name = "hashbrown" version = "0.14.5" @@ -560,6 +703,12 @@ version = "0.3.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d231dfb89cfffdbc30e7fc41579ed6066ad03abda9e567ccafae602b97ec5024" +[[package]] +name = "hermit-abi" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fbf6a919d6cf397374f7dfeeea91d974c7c0a7221d0d0f4f20d859d329e53fcc" + [[package]] name = "home" version = "0.5.9" @@ -743,12 +892,32 @@ version = "2.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "187674a687eed5fe42285b40c6291f9a01517d415fad1c3cbc6a9f778af7fcd4" +[[package]] +name = "is-terminal" +version = "0.4.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "261f68e344040fbd0edea105bef17c66edf46f984ddb1115b775ce31be948f4b" +dependencies = [ + "hermit-abi 0.4.0", + "libc", + "windows-sys 0.52.0", +] + [[package]] name = "is_terminal_polyfill" version = "1.70.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7943c866cc5cd64cbc25b2e01621d07fa8eb2a1a23160ee81ce38704e97b8ecf" +[[package]] +name = "itertools" +version = "0.10.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b0fd2260e829bddf4cb6ea802289de2f86d6a7a690192fbe91b3f46e0f2c8473" +dependencies = [ + "either", +] + [[package]] name = "itoa" version = "1.0.11" @@ -922,7 +1091,7 @@ version = "1.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "80e04d1dcff3aae0704555fe5fee3bcfaf3d1fdf8a7e521d5b9d2b42acb52cec" dependencies = [ - "hermit-abi", + "hermit-abi 0.3.9", "libc", "wasi", "windows-sys 0.52.0", @@ -1054,6 +1223,12 @@ dependencies = [ "portable-atomic", ] +[[package]] +name = "oorandom" +version = "11.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b410bbe7e14ab526a0e86877eb47c6996a2bd7746f027ba551028c925390e4e9" + [[package]] name = "openssl" version = "0.10.68" @@ -1155,6 +1330,34 @@ version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "953ec861398dccce10c670dfeaf3ec4911ca479e9c02154b3a215178c5f566f2" +[[package]] +name = "plotters" +version = "0.3.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5aeb6f403d7a4911efb1e33402027fc44f29b5bf6def3effcc22d7bb75f2b747" +dependencies = [ + "num-traits", + "plotters-backend", + "plotters-svg", + "wasm-bindgen", + "web-sys", +] + +[[package]] +name = "plotters-backend" +version = "0.3.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "df42e13c12958a16b3f7f4386b9ab1f3e7933914ecea48da7139435263a4172a" + +[[package]] +name = "plotters-svg" +version = "0.3.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "51bae2ac328883f7acdfea3d66a7c35751187f870bc81f94563733a154d7a670" +dependencies = [ + "plotters-backend", +] + [[package]] name = "portable-atomic" version = "1.9.0" @@ -1281,6 +1484,26 @@ dependencies = [ "getrandom", ] +[[package]] +name = "rayon" +version = "1.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b418a60154510ca1a002a752ca9714984e21e4241e804d32555251faf8b78ffa" +dependencies = [ + "either", + "rayon-core", +] + +[[package]] +name = "rayon-core" +version = "1.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1465873a3dfdaa8ae7cb14b4383657caab0b3e8a0aa9ae8e04b044854c8dfce2" +dependencies = [ + "crossbeam-deque", + "crossbeam-utils", +] + [[package]] name = "redox_syscall" version = "0.5.6" @@ -1442,6 +1665,15 @@ version = "1.0.18" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f3cb5ba0dc43242ce17de99c180e96db90b235b8a9fdc9543c96d2209116bd9f" +[[package]] +name = "same-file" +version = "1.0.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "93fc1dc3aaa9bfed95e02e6eadabb4baf7e3078b0bd1b4d7b6b0b68378900502" +dependencies = [ + "winapi-util", +] + [[package]] name = "schannel" version = "0.1.24" @@ -1677,6 +1909,16 @@ dependencies = [ "syn", ] +[[package]] +name = "tinytemplate" +version = "1.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "be4d6b5f19ff7664e8c98d03e2139cb510db9b0a60b55f8e8709b689d939b6bc" +dependencies = [ + "serde", + "serde_json", +] + [[package]] name = "tinyvec" version = "1.8.0" @@ -1858,6 +2100,16 @@ version = "0.2.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "accd4ea62f7bb7a82fe23066fb0957d48ef677f6eeb8215f372f52e48bb32426" +[[package]] +name = "walkdir" +version = "2.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "29790946404f91d9c5d06f9874efddea1dc06c5efe94541a7d6863108e3a5e4b" +dependencies = [ + "same-file", + "winapi-util", +] + [[package]] name = "want" version = "0.3.1" @@ -1962,6 +2214,15 @@ dependencies = [ "winsafe", ] +[[package]] +name = "winapi-util" +version = "0.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cf221c93e13a30d793f7645a0e7762c55d169dbb0a49671918a2319d289b10bb" +dependencies = [ + "windows-sys 0.59.0", +] + [[package]] name = "windows-core" version = "0.52.0" diff --git a/bindings/node/Cargo.toml b/bindings/node/Cargo.toml index 1ca03ac..d80c6ff 100644 --- a/bindings/node/Cargo.toml +++ b/bindings/node/Cargo.toml @@ -13,6 +13,7 @@ license.workspace = true [lib] crate-type = ["cdylib"] +bench = false [dependencies] # Default enable napi4 feature, see https://nodejs.org/api/n-api.html#node-api-version-matrix diff --git a/bindings/python/Cargo.toml b/bindings/python/Cargo.toml index f57d3e3..d15775b 100644 --- a/bindings/python/Cargo.toml +++ b/bindings/python/Cargo.toml @@ -14,6 +14,7 @@ license.workspace = true [lib] name = "cpp_linter" crate-type = ["cdylib"] +bench = false [dependencies] pyo3 = { version = "0.23.3", features = ["extension-module"] } diff --git a/cpp-linter/Cargo.toml b/cpp-linter/Cargo.toml index 0e7ed53..9ac777e 100644 --- a/cpp-linter/Cargo.toml +++ b/cpp-linter/Cargo.toml @@ -37,14 +37,22 @@ tokio-stream = "0.1.17" which = "7.0.0" [dev-dependencies] +criterion = { version = "2.7.2", package = "codspeed-criterion-compat", features=["async_tokio"] } mockito = "1.6.1" tempfile = "3.14.0" [features] openssl-vendored = ["dep:openssl", "dep:openssl-probe"] +[lib] +bench = false + [[bin]] name = "cpp-linter" path = "src/main.rs" test = false bench = false + +[[bench]] +name = "run" +harness = false diff --git a/cpp-linter/benches/run.rs b/cpp-linter/benches/run.rs new file mode 100644 index 0000000..f202b55 --- /dev/null +++ b/cpp-linter/benches/run.rs @@ -0,0 +1,31 @@ +use cpp_linter::run::run_main; +use criterion::Criterion; +use criterion::{criterion_group, criterion_main}; + +// This is a struct that tells Criterion.rs to use the tokio crate's multi-thread executor +use tokio::runtime::Runtime; +async fn run() -> Result<(), anyhow::Error> { + let args = vec![ + "cpp-linter".to_string(), + "--lines-changed-only".to_string(), + "false".to_string(), + "--database".to_string(), + "benches/libgit2/build".to_string(), + "--ignore".to_string(), + "|!benches/libgit2/src/libgit2".to_string(), + "--extensions".to_string(), + "c".to_string(), + ]; + run_main(args).await +} + +fn bench1(c: &mut Criterion) { + c.bench_function("scan libgit2", |b| { + // Insert a call to `to_async` to convert the bencher to async mode. + // The timing loops are the same as with the normal bencher. + b.to_async(Runtime::new().unwrap()).iter(run); + }); +} + +criterion_group!(benches, bench1); +criterion_main!(benches); diff --git a/cpp-linter/src/clang_tools/clang_format.rs b/cpp-linter/src/clang_tools/clang_format.rs index a3c82d3..ba0b1d9 100644 --- a/cpp-linter/src/clang_tools/clang_format.rs +++ b/cpp-linter/src/clang_tools/clang_format.rs @@ -135,8 +135,8 @@ pub fn run_clang_format( .to_str() .unwrap_or_default(), cmd.get_args() - .map(|a| a.to_str().unwrap()) - .collect::>() + .map(|a| a.to_string_lossy()) + .collect::>() .join(" ") ), )); @@ -153,8 +153,8 @@ pub fn run_clang_format( "Running \"{} {}\"", cmd.get_program().to_string_lossy(), cmd.get_args() - .map(|x| x.to_str().unwrap()) - .collect::>() + .map(|x| x.to_string_lossy()) + .collect::>() .join(" ") ), )); diff --git a/cpp-linter/src/clang_tools/clang_tidy.rs b/cpp-linter/src/clang_tools/clang_tidy.rs index 988dc77..fd39b75 100644 --- a/cpp-linter/src/clang_tools/clang_tidy.rs +++ b/cpp-linter/src/clang_tools/clang_tidy.rs @@ -264,15 +264,17 @@ pub fn run_clang_tidy( let file_name = file.name.to_string_lossy().to_string(); if clang_params.lines_changed_only != LinesChangedOnly::Off { let ranges = file.get_ranges(&clang_params.lines_changed_only); - let filter = format!( - "[{{\"name\":{:?},\"lines\":{:?}}}]", - &file_name.replace('/', if OS == "windows" { "\\" } else { "/" }), - ranges - .iter() - .map(|r| [r.start(), r.end()]) - .collect::>() - ); - cmd.args(["--line-filter", filter.as_str()]); + if !ranges.is_empty() { + let filter = format!( + "[{{\"name\":{:?},\"lines\":{:?}}}]", + &file_name.replace('/', if OS == "windows" { "\\" } else { "/" }), + ranges + .iter() + .map(|r| [r.start(), r.end()]) + .collect::>() + ); + cmd.args(["--line-filter", filter.as_str()]); + } } let mut original_content = None; if clang_params.tidy_review { @@ -294,8 +296,8 @@ pub fn run_clang_tidy( "Running \"{} {}\"", cmd.get_program().to_string_lossy(), cmd.get_args() - .map(|x| x.to_str().unwrap()) - .collect::>() + .map(|x| x.to_string_lossy()) + .collect::>() .join(" ") ), )); diff --git a/cpp-linter/src/cli/mod.rs b/cpp-linter/src/cli/mod.rs index 1e01ca1..630f62b 100644 --- a/cpp-linter/src/cli/mod.rs +++ b/cpp-linter/src/cli/mod.rs @@ -390,14 +390,13 @@ pub fn convert_extra_arg_val(args: &ArgMatches) -> Vec { let mut val = args.get_many::("extra-arg").unwrap_or_default(); if val.len() == 1 { // specified once; split and return result - return val - .next() + val.next() .unwrap() .trim_matches('\'') .trim_matches('"') .split(' ') .map(|i| i.to_string()) - .collect(); + .collect() } else { // specified multiple times; just return val.map(|i| i.to_string()).collect() @@ -408,13 +407,20 @@ pub fn convert_extra_arg_val(args: &ArgMatches) -> Vec { mod test { use clap::ArgMatches; - use super::{convert_extra_arg_val, get_arg_parser}; + use super::{convert_extra_arg_val, get_arg_parser, Cli}; fn parser_args(input: Vec<&str>) -> ArgMatches { let arg_parser = get_arg_parser(); arg_parser.get_matches_from(input) } + #[test] + fn ignore_blank_extensions() { + let args = parser_args(vec!["cpp-linter", "-e", "c,,h"]); + let cli = Cli::from(&args); + assert!(!cli.extensions.contains(&"".to_string())); + } + #[test] fn extra_arg_0() { let args = parser_args(vec!["cpp-linter"]); diff --git a/cpp-linter/src/cli/structs.rs b/cpp-linter/src/cli/structs.rs index 5bc876b..071ee5c 100644 --- a/cpp-linter/src/cli/structs.rs +++ b/cpp-linter/src/cli/structs.rs @@ -25,6 +25,14 @@ impl LinesChangedOnly { _ => LinesChangedOnly::Off, } } + + pub fn is_change_valid(&self, added_lines: bool, diff_chunks: bool) -> bool { + match self { + LinesChangedOnly::Off => true, + LinesChangedOnly::Diff => diff_chunks, + LinesChangedOnly::On => added_lines, + } + } } impl Display for LinesChangedOnly { @@ -91,7 +99,14 @@ impl From<&ArgMatches> for Cli { let extensions = args .get_many::("extensions") .unwrap() - .map(|s| s.to_string()) + .filter_map(|s| { + if s.is_empty() { + // filter out blank extensions here + None + } else { + Some(s.to_string()) + } + }) .collect::>(); Self { diff --git a/cpp-linter/src/common_fs/file_filter.rs b/cpp-linter/src/common_fs/file_filter.rs index 88a5521..a8ba786 100644 --- a/cpp-linter/src/common_fs/file_filter.rs +++ b/cpp-linter/src/common_fs/file_filter.rs @@ -105,10 +105,15 @@ impl FileFilter { let glob_matched = glob_match(pattern, file_name.to_string_lossy().to_string().as_str()); let pat = PathBuf::from(&pattern); - if glob_matched + if pattern.as_str() == "./" + || glob_matched || (pat.is_file() && file_name == pat) || (pat.is_dir() && file_name.starts_with(pat)) { + log::debug!( + "file {file_name:?} is in {}ignored with domain {pattern:?}.", + if is_ignored { "" } else { "not " } + ); return true; } } @@ -124,23 +129,17 @@ impl FileFilter { /// - Is `entry` specified in the list of explicitly `not_ignored` paths? (supersedes /// specified `ignored` paths) pub fn is_source_or_ignored(&self, entry: &Path) -> bool { - let extension = entry.extension(); - if extension.is_none() { + let extension = entry + .extension() + .unwrap_or_default() // allow for matching files with no extension + .to_string_lossy() + .to_string(); + if !self.extensions.contains(&extension) { return false; } - let mut is_ignored = true; - for ext in &self.extensions { - if ext == &extension.unwrap().to_os_string().into_string().unwrap() { - is_ignored = false; - break; - } - } - if !is_ignored { - let is_in_ignored = self.is_file_in_list(entry, true); - let is_in_not_ignored = self.is_file_in_list(entry, false); - if is_in_not_ignored || !is_in_ignored { - return true; - } + let is_in_not_ignored = self.is_file_in_list(entry, false); + if is_in_not_ignored || !self.is_file_in_list(entry, true) { + return true; } false } diff --git a/cpp-linter/src/git.rs b/cpp-linter/src/git.rs index 030ed65..e777f23 100644 --- a/cpp-linter/src/git.rs +++ b/cpp-linter/src/git.rs @@ -15,7 +15,10 @@ use anyhow::{Context, Result}; use git2::{Diff, Error, Patch, Repository}; // project specific modules/crates -use crate::common_fs::{FileFilter, FileObj}; +use crate::{ + cli::LinesChangedOnly, + common_fs::{FileFilter, FileObj}, +}; /// This (re-)initializes the repository located in the specified `path`. /// @@ -61,6 +64,10 @@ pub fn get_diff(repo: &Repository) -> Result { } } + // RARE BUG when `head` is the first commit in the repo! Affects local-only runs. + // > panicked at cpp-linter\src\git.rs:73:43: + // > called `Result::unwrap()` on an `Err` value: + // > Error { code: -3, class: 3, message: "parent 0 does not exist" } if has_staged_files { // get diff for staged files only repo.diff_tree_to_index(Some(&head), None, None) @@ -100,22 +107,26 @@ fn parse_patch(patch: &Patch) -> (Vec, Vec>) { /// /// The specified list of `extensions`, `ignored` and `not_ignored` files are used as /// filters to expedite the process and only focus on the data cpp_linter can use. -pub fn parse_diff(diff: &git2::Diff, file_filter: &FileFilter) -> Vec { +pub fn parse_diff( + diff: &git2::Diff, + file_filter: &FileFilter, + lines_changed_only: &LinesChangedOnly, +) -> Vec { let mut files: Vec = Vec::new(); for file_idx in 0..diff.deltas().count() { let diff_delta = diff.get_delta(file_idx).unwrap(); let file_path = diff_delta.new_file().path().unwrap().to_path_buf(); - if [ - git2::Delta::Added, - git2::Delta::Modified, - git2::Delta::Renamed, - ] - .contains(&diff_delta.status()) - && file_filter.is_source_or_ignored(&file_path) + if matches!( + diff_delta.status(), + git2::Delta::Added | git2::Delta::Modified | git2::Delta::Renamed, + ) && file_filter.is_source_or_ignored(&file_path) { let (added_lines, diff_chunks) = parse_patch(&Patch::from_diff(diff, file_idx).unwrap().unwrap()); - files.push(FileObj::from(file_path, added_lines, diff_chunks)); + if lines_changed_only.is_change_valid(!added_lines.is_empty(), !diff_chunks.is_empty()) + { + files.push(FileObj::from(file_path, added_lines, diff_chunks)); + } } } files @@ -128,12 +139,20 @@ pub fn parse_diff(diff: &git2::Diff, file_filter: &FileFilter) -> Vec { /// log warning and error are output when this occurs. Please report this instance for /// troubleshooting/diagnosis as this likely means the diff is malformed or there is a /// bug in libgit2 source. -pub fn parse_diff_from_buf(buff: &[u8], file_filter: &FileFilter) -> Vec { +pub fn parse_diff_from_buf( + buff: &[u8], + file_filter: &FileFilter, + lines_changed_only: &LinesChangedOnly, +) -> Vec { if let Ok(diff_obj) = &Diff::from_buffer(buff) { - parse_diff(diff_obj, file_filter) + parse_diff(diff_obj, file_filter, lines_changed_only) } else { log::warn!("libgit2 failed to parse the diff"); - brute_force_parse_diff::parse_diff(&String::from_utf8_lossy(buff), file_filter) + brute_force_parse_diff::parse_diff( + &String::from_utf8_lossy(buff), + file_filter, + lines_changed_only, + ) } } @@ -149,7 +168,10 @@ mod brute_force_parse_diff { use regex::Regex; use std::{ops::RangeInclusive, path::PathBuf}; - use crate::common_fs::{FileFilter, FileObj}; + use crate::{ + cli::LinesChangedOnly, + common_fs::{FileFilter, FileObj}, + }; fn get_filename_from_front_matter(front_matter: &str) -> Option<&str> { let diff_file_name = Regex::new(r"(?m)^\+\+\+\sb?/(.*)$").unwrap(); @@ -212,7 +234,11 @@ mod brute_force_parse_diff { (additions, diff_chunks) } - pub fn parse_diff(diff: &str, file_filter: &FileFilter) -> Vec { + pub fn parse_diff( + diff: &str, + file_filter: &FileFilter, + lines_changed_only: &LinesChangedOnly, + ) -> Vec { log::error!("Using brute force diff parsing!"); let mut results = Vec::new(); let diff_file_delimiter = Regex::new(r"(?m)^diff --git a/.*$").unwrap(); @@ -233,7 +259,11 @@ mod brute_force_parse_diff { let file_path = PathBuf::from(file_name); if file_filter.is_source_or_ignored(&file_path) { let (added_lines, diff_chunks) = parse_patch(&file_diff[hunk_start..]); - results.push(FileObj::from(file_path, added_lines, diff_chunks)); + if lines_changed_only + .is_change_valid(!added_lines.is_empty(), !diff_chunks.is_empty()) + { + results.push(FileObj::from(file_path, added_lines, diff_chunks)); + } } } // } else { @@ -250,6 +280,7 @@ mod brute_force_parse_diff { use super::parse_diff; use crate::{ + cli::LinesChangedOnly, common_fs::{FileFilter, FileObj}, git::parse_diff_from_buf, }; @@ -277,6 +308,7 @@ rename to /tests/demo/some source.c let files = parse_diff_from_buf( diff_buf, &FileFilter::new(&["target".to_string()], vec!["c".to_string()]), + &LinesChangedOnly::Off, ); assert!(!files.is_empty()); assert!(files @@ -292,6 +324,7 @@ rename to /tests/demo/some source.c let files = parse_diff_from_buf( diff_buf, &FileFilter::new(&["target".to_string()], vec!["c".to_string()]), + &LinesChangedOnly::Off, ); assert!(!files.is_empty()); } @@ -304,8 +337,13 @@ rename to /tests/demo/some source.c parse_diff_from_buf( buf.as_bytes(), &FileFilter::new(&ignore, extensions.to_owned()), + &LinesChangedOnly::Off, + ), + parse_diff( + buf, + &FileFilter::new(&ignore, extensions.to_owned()), + &LinesChangedOnly::Off, ), - parse_diff(buf, &FileFilter::new(&ignore, extensions.to_owned())), ) } @@ -380,6 +418,7 @@ mod test { use tempfile::{tempdir, TempDir}; use crate::{ + cli::LinesChangedOnly, common_fs::FileFilter, rest_api::{github::GithubApiClient, RestApiClient}, }; @@ -409,7 +448,7 @@ mod test { env::set_var("CI", "false"); // avoid use of REST API when testing in CI rest_api_client .unwrap() - .get_list_of_changed_files(&file_filter) + .get_list_of_changed_files(&file_filter, &LinesChangedOnly::Off) .await .unwrap() } diff --git a/cpp-linter/src/logger.rs b/cpp-linter/src/logger.rs index ca2f834..1c1290e 100644 --- a/cpp-linter/src/logger.rs +++ b/cpp-linter/src/logger.rs @@ -2,7 +2,6 @@ use std::env; -use anyhow::{Error, Result}; use colored::{control::set_override, Colorize}; use log::{Level, LevelFilter, Metadata, Record}; @@ -46,8 +45,9 @@ impl log::Log for SimpleLogger { /// A function to initialize the private `LOGGER`. /// /// The logging level defaults to [`LevelFilter::Info`]. -/// Returns a [`SetLoggerError`](struct@log::SetLoggerError) if the `LOGGER` is already initialized. -pub fn init() -> Result<()> { +/// This logs a debug message about [`SetLoggerError`](struct@log::SetLoggerError) +/// if the `LOGGER` is already initialized. +pub fn try_init() { let logger: SimpleLogger = SimpleLogger; if matches!( env::var("CPP_LINTER_COLOR").as_deref(), @@ -55,21 +55,25 @@ pub fn init() -> Result<()> { ) { set_override(true); } - log::set_boxed_logger(Box::new(logger)) - .map(|()| log::set_max_level(LevelFilter::Info)) - .map_err(Error::from) + if let Err(e) = + log::set_boxed_logger(Box::new(logger)).map(|()| log::set_max_level(LevelFilter::Info)) + { + // logger singleton already instantiated. + // we'll just use whatever the current config is. + log::debug!("{e:?}"); + } } #[cfg(test)] mod test { use std::env; - use super::{init, SimpleLogger}; + use super::{try_init, SimpleLogger}; #[test] fn trace_log() { env::set_var("CPP_LINTER_COLOR", "true"); - init().unwrap_or(()); + try_init(); assert!(SimpleLogger::level_color(&log::Level::Trace).contains("TRACE")); log::set_max_level(log::LevelFilter::Trace); log::trace!("A dummy log statement for code coverage"); diff --git a/cpp-linter/src/rest_api/github/mod.rs b/cpp-linter/src/rest_api/github/mod.rs index 500d7f6..05b5a61 100644 --- a/cpp-linter/src/rest_api/github/mod.rs +++ b/cpp-linter/src/rest_api/github/mod.rs @@ -14,21 +14,19 @@ use reqwest::{ header::{HeaderMap, HeaderValue, AUTHORIZATION}, Client, Method, Url, }; -use serde_json; // project specific modules/crates use super::{RestApiClient, RestApiRateLimitHeaders}; use crate::clang_tools::clang_format::tally_format_advice; use crate::clang_tools::clang_tidy::tally_tidy_advice; use crate::clang_tools::ClangVersions; -use crate::cli::{FeedbackInput, ThreadComments}; +use crate::cli::{FeedbackInput, LinesChangedOnly, ThreadComments}; use crate::common_fs::{FileFilter, FileObj}; use crate::git::{get_diff, open_repo, parse_diff, parse_diff_from_buf}; // private submodules. mod serde_structs; mod specific_api; -use serde_structs::{GithubChangedFile, PushEventFiles}; /// A structure to work with Github REST API. pub struct GithubApiClient { @@ -121,7 +119,11 @@ impl RestApiClient for GithubApiClient { Ok(headers) } - async fn get_list_of_changed_files(&self, file_filter: &FileFilter) -> Result> { + async fn get_list_of_changed_files( + &self, + file_filter: &FileFilter, + lines_changed_only: &LinesChangedOnly, + ) -> Result> { if env::var("CI").is_ok_and(|val| val.as_str() == "true") && self.repo.is_some() && self.sha.is_some() @@ -153,9 +155,13 @@ impl RestApiClient for GithubApiClient { 0, ) .await - .with_context(|| "Failed to get list of changed files from GitHub server.")?; + .with_context(|| "Failed to get list of changed files.")?; if response.status().is_success() { - Ok(parse_diff_from_buf(&response.bytes().await?, file_filter)) + Ok(parse_diff_from_buf( + &response.bytes().await?, + file_filter, + lines_changed_only, + )) } else { let endpoint = if is_pr { Url::parse(format!("{}/files", url.as_str()).as_str())? @@ -164,7 +170,7 @@ impl RestApiClient for GithubApiClient { }; Self::log_response(response, "Failed to get full diff for event").await; log::debug!("Trying paginated request to {}", endpoint.as_str()); - self.get_changed_files_paginated(endpoint, file_filter) + self.get_changed_files_paginated(endpoint, file_filter, lines_changed_only) .await } } else { @@ -172,61 +178,11 @@ impl RestApiClient for GithubApiClient { let repo = open_repo(".").with_context(|| { "Please ensure the repository is checked out before running cpp-linter." })?; - let list = parse_diff(&get_diff(&repo)?, file_filter); + let list = parse_diff(&get_diff(&repo)?, file_filter, lines_changed_only); Ok(list) } } - async fn get_changed_files_paginated( - &self, - url: Url, - file_filter: &FileFilter, - ) -> Result> { - let mut url = Some(Url::parse_with_params(url.as_str(), &[("page", "1")])?); - let mut files = vec![]; - while let Some(ref endpoint) = url { - let request = - Self::make_api_request(&self.client, endpoint.as_str(), Method::GET, None, None)?; - let response = Self::send_api_request( - self.client.clone(), - request, - self.rate_limit_headers.clone(), - 0, - ) - .await; - if let Ok(response) = response { - url = Self::try_next_page(response.headers()); - let files_list = if self.event_name != "pull_request" { - let json_value: PushEventFiles = serde_json::from_str(&response.text().await?) - .with_context(|| { - "Failed to deserialize list of changed files from json response" - })?; - json_value.files - } else { - serde_json::from_str::>(&response.text().await?) - .with_context(|| { - "Failed to deserialize list of file changes from Pull Request event." - })? - }; - for file in files_list { - if let Some(patch) = file.patch { - let diff = format!( - "diff --git a/{old} b/{new}\n--- a/{old}\n+++ b/{new}\n{patch}", - old = file.previous_filename.unwrap_or(file.filename.clone()), - new = file.filename, - ); - if let Some(file_obj) = - parse_diff_from_buf(diff.as_bytes(), file_filter).first() - { - files.push(file_obj.to_owned()); - } - } - } - } - } - Ok(files) - } - async fn post_feedback( &self, files: &[Arc>], @@ -241,7 +197,7 @@ impl RestApiClient for GithubApiClient { self.post_annotations(files, feedback_inputs.style.as_str()); } if feedback_inputs.step_summary { - comment = Some(self.make_comment( + comment = Some(Self::make_comment( files, format_checks_failed, tidy_checks_failed, @@ -259,7 +215,7 @@ impl RestApiClient for GithubApiClient { if feedback_inputs.thread_comments != ThreadComments::Off { // post thread comment for PR or push event if comment.as_ref().is_some_and(|c| c.len() > 65535) || comment.is_none() { - comment = Some(self.make_comment( + comment = Some(Self::make_comment( files, format_checks_failed, tidy_checks_failed, @@ -319,7 +275,7 @@ mod test { clang_tidy::{TidyAdvice, TidyNotification}, ClangVersions, }, - cli::FeedbackInput, + cli::{FeedbackInput, LinesChangedOnly}, common_fs::{FileFilter, FileObj}, logger, rest_api::{RestApiClient, USER_OUTREACH}, @@ -334,7 +290,7 @@ mod test { ) -> (String, String) { let tmp_dir = tempdir().unwrap(); let rest_api_client = GithubApiClient::new().unwrap(); - logger::init().unwrap(); + logger::try_init(); if env::var("ACTIONS_STEP_DEBUG").is_ok_and(|var| var == "true") { assert!(rest_api_client.debug_enabled); log::set_max_level(log::LevelFilter::Debug); @@ -478,7 +434,7 @@ mod test { env::set_current_dir(tmp_dir.path()).unwrap(); let rest_client = GithubApiClient::new().unwrap(); let files = rest_client - .get_list_of_changed_files(&FileFilter::new(&[], vec![])) + .get_list_of_changed_files(&FileFilter::new(&[], vec![]), &LinesChangedOnly::Off) .await; assert!(files.is_err()) } diff --git a/cpp-linter/src/rest_api/github/serde_structs.rs b/cpp-linter/src/rest_api/github/serde_structs.rs index 9e0a56e..e34e3a0 100644 --- a/cpp-linter/src/rest_api/github/serde_structs.rs +++ b/cpp-linter/src/rest_api/github/serde_structs.rs @@ -49,6 +49,8 @@ pub struct GithubChangedFile { pub previous_filename: Option, /// The individual patch that describes the file's changes. pub patch: Option, + /// The number of changes to the file contents. + pub changes: i64, } /// A structure for deserializing a Push event's changed files. diff --git a/cpp-linter/src/rest_api/github/specific_api.rs b/cpp-linter/src/rest_api/github/specific_api.rs index be367f3..aa86d5f 100644 --- a/cpp-linter/src/rest_api/github/specific_api.rs +++ b/cpp-linter/src/rest_api/github/specific_api.rs @@ -5,6 +5,7 @@ use std::{ env, fs::OpenOptions, io::{Read, Write}, + path::{Path, PathBuf}, sync::{Arc, Mutex}, }; @@ -13,15 +14,16 @@ use reqwest::{Client, Method, Url}; use crate::{ clang_tools::{clang_format::summarize_style, ClangVersions, ReviewComments}, - cli::FeedbackInput, - common_fs::FileObj, + cli::{FeedbackInput, LinesChangedOnly}, + common_fs::{FileFilter, FileObj}, + git::parse_diff_from_buf, rest_api::{RestApiRateLimitHeaders, COMMENT_MARKER, USER_AGENT}, }; use super::{ serde_structs::{ - FullReview, PullRequestInfo, ReviewComment, ReviewDiffComment, ThreadComment, - REVIEW_DISMISSAL, + FullReview, GithubChangedFile, PullRequestInfo, PushEventFiles, ReviewComment, + ReviewDiffComment, ThreadComment, REVIEW_DISMISSAL, }, GithubApiClient, RestApiClient, }; @@ -77,6 +79,73 @@ impl GithubApiClient { }) } + /// A way to get the list of changed files using REST API calls that employ a paginated response. + /// + /// This is a helper to [`Self::get_list_of_changed_files()`] but takes a formulated `url` + /// endpoint based on the context of the triggering CI event. + pub(super) async fn get_changed_files_paginated( + &self, + url: Url, + file_filter: &FileFilter, + lines_changed_only: &LinesChangedOnly, + ) -> Result> { + let mut url = Some(Url::parse_with_params(url.as_str(), &[("page", "1")])?); + let mut files = vec![]; + while let Some(ref endpoint) = url { + let request = + Self::make_api_request(&self.client, endpoint.as_str(), Method::GET, None, None)?; + let response = Self::send_api_request( + self.client.clone(), + request, + self.rate_limit_headers.clone(), + 0, + ) + .await + .with_context(|| "Failed to get paginated list of changed files")?; + url = Self::try_next_page(response.headers()); + let files_list = if self.event_name != "pull_request" { + let json_value: PushEventFiles = serde_json::from_str(&response.text().await?) + .with_context(|| { + "Failed to deserialize list of changed files from json response" + })?; + json_value.files + } else { + serde_json::from_str::>(&response.text().await?) + .with_context(|| { + "Failed to deserialize list of file changes from Pull Request event." + })? + }; + for file in files_list { + let ext = Path::new(&file.filename).extension().unwrap_or_default(); + if !file_filter + .extensions + .contains(&ext.to_string_lossy().to_string()) + { + continue; + } + if let Some(patch) = file.patch { + let diff = format!( + "diff --git a/{old} b/{new}\n--- a/{old}\n+++ b/{new}\n{patch}\n", + old = file.previous_filename.unwrap_or(file.filename.clone()), + new = file.filename, + ); + if let Some(file_obj) = + parse_diff_from_buf(diff.as_bytes(), file_filter, lines_changed_only) + .first() + { + files.push(file_obj.to_owned()); + } + } else if file.changes == 0 { + // file may have been only renamed. + // include it in case files-changed-only is enabled. + files.push(FileObj::new(PathBuf::from(file.filename))); + } + // else changes are too big or we don't care + } + } + Ok(files) + } + /// Append step summary to CI workflow's summary page. pub fn post_step_summary(&self, comment: &String) { if let Ok(gh_out) = env::var("GITHUB_STEP_SUMMARY") { diff --git a/cpp-linter/src/rest_api/mod.rs b/cpp-linter/src/rest_api/mod.rs index 198c5c6..059ea9b 100644 --- a/cpp-linter/src/rest_api/mod.rs +++ b/cpp-linter/src/rest_api/mod.rs @@ -18,7 +18,7 @@ use reqwest::{Client, IntoUrl, Method, Request, Response, Url}; // project specific modules pub mod github; use crate::clang_tools::ClangVersions; -use crate::cli::FeedbackInput; +use crate::cli::{FeedbackInput, LinesChangedOnly}; use crate::common_fs::{FileFilter, FileObj}; pub static COMMENT_MARKER: &str = "\n"; @@ -209,16 +209,7 @@ pub trait RestApiClient { fn get_list_of_changed_files( &self, file_filter: &FileFilter, - ) -> impl Future>>; - - /// A way to get the list of changed files using REST API calls that employ a paginated response. - /// - /// This is a helper to [`RestApiClient::get_list_of_changed_files()`] but takes a formulated URL - /// endpoint based on the context of the triggering CI event. - fn get_changed_files_paginated( - &self, - url: Url, - file_filter: &FileFilter, + lines_changed_only: &LinesChangedOnly, ) -> impl Future>>; /// Makes a comment in MarkDown syntax based on the concerns in `format_advice` and @@ -230,7 +221,6 @@ pub trait RestApiClient { /// Returns the markdown comment as a string as well as the total count of /// `format_checks_failed` and `tidy_checks_failed` (in respective order). fn make_comment( - &self, files: &[Arc>], format_checks_failed: u64, tidy_checks_failed: u64, @@ -415,12 +405,13 @@ mod test { use anyhow::{anyhow, Result}; use chrono::Utc; use mockito::{Matcher, Server}; + use reqwest::Method; use reqwest::{ header::{HeaderMap, HeaderValue}, Client, }; - use reqwest::{Method, Url}; + use crate::cli::LinesChangedOnly; use crate::{ clang_tools::ClangVersions, cli::FeedbackInput, @@ -451,14 +442,7 @@ mod test { async fn get_list_of_changed_files( &self, _file_filter: &FileFilter, - ) -> Result> { - Err(anyhow!("Not implemented")) - } - - async fn get_changed_files_paginated( - &self, - _url: reqwest::Url, - _file_filter: &FileFilter, + _lines_changed_only: &LinesChangedOnly, ) -> Result> { Err(anyhow!("Not implemented")) } @@ -488,14 +472,7 @@ mod test { dummy.start_log_group("Dummy test".to_string()); assert_eq!(dummy.set_exit_code(1, None, None), 0); assert!(dummy - .get_list_of_changed_files(&FileFilter::new(&[], vec![])) - .await - .is_err()); - assert!(dummy - .get_changed_files_paginated( - Url::parse("https://example.net").unwrap(), - &FileFilter::new(&[], vec![]) - ) + .get_list_of_changed_files(&FileFilter::new(&[], vec![]), &LinesChangedOnly::Off) .await .is_err()); assert!(dummy @@ -513,7 +490,7 @@ mod test { assert!(headers .insert("link", HeaderValue::from_str("; rel=\"next\"").unwrap()) .is_none()); - logger::init().unwrap(); + logger::try_init(); log::set_max_level(log::LevelFilter::Debug); let result = TestClient::try_next_page(&headers); assert!(result.is_none()); @@ -528,7 +505,7 @@ mod test { HeaderValue::from_str("; rel=\"next\"").unwrap() ) .is_none()); - logger::init().unwrap(); + logger::try_init(); log::set_max_level(log::LevelFilter::Debug); let result = TestClient::try_next_page(&headers); assert!(result.is_none()); @@ -553,7 +530,7 @@ mod test { remaining: "remaining".to_string(), retry: "retry".to_string(), }; - logger::init().unwrap(); + logger::try_init(); log::set_max_level(log::LevelFilter::Debug); let mut server = Server::new_async().await; diff --git a/cpp-linter/src/run.rs b/cpp-linter/src/run.rs index d7fe857..7c29155 100644 --- a/cpp-linter/src/run.rs +++ b/cpp-linter/src/run.rs @@ -58,7 +58,7 @@ pub async fn run_main(args: Vec) -> Result<()> { return Ok(()); } - logger::init().unwrap(); + logger::try_init(); if cli.version == "NO-VERSION" { log::error!("The `--version` arg is used to specify which version of clang to use."); @@ -78,6 +78,7 @@ pub async fn run_main(args: Vec) -> Result<()> { LevelFilter::Info }); log::info!("Processing event {}", rest_api_client.event_name); + let is_pr = rest_api_client.event_name == "pull_request"; let mut file_filter = FileFilter::new(&cli.ignore, cli.extensions.clone()); file_filter.parse_submodules(); @@ -99,30 +100,31 @@ pub async fn run_main(args: Vec) -> Result<()> { } rest_api_client.start_log_group(String::from("Get list of specified source files")); - let files = if cli.lines_changed_only != LinesChangedOnly::Off || cli.files_changed_only { - // parse_diff(github_rest_api_payload) - rest_api_client - .get_list_of_changed_files(&file_filter) - .await? - } else { - // walk the folder and look for files with specified extensions according to ignore values. - let mut all_files = file_filter.list_source_files(".")?; - if rest_api_client.event_name == "pull_request" && (cli.tidy_review || cli.format_review) { - let changed_files = rest_api_client - .get_list_of_changed_files(&file_filter) - .await?; - for changed_file in changed_files { - for file in &mut all_files { - if changed_file.name == file.name { - file.diff_chunks = changed_file.diff_chunks.clone(); - file.added_lines = changed_file.added_lines.clone(); - file.added_ranges = changed_file.added_ranges.clone(); + let files = + if !matches!(cli.lines_changed_only, LinesChangedOnly::Off) || cli.files_changed_only { + // parse_diff(github_rest_api_payload) + rest_api_client + .get_list_of_changed_files(&file_filter, &cli.lines_changed_only) + .await? + } else { + // walk the folder and look for files with specified extensions according to ignore values. + let mut all_files = file_filter.list_source_files(".")?; + if is_pr && (cli.tidy_review || cli.format_review) { + let changed_files = rest_api_client + .get_list_of_changed_files(&file_filter, &LinesChangedOnly::Off) + .await?; + for changed_file in changed_files { + for file in &mut all_files { + if changed_file.name == file.name { + file.diff_chunks = changed_file.diff_chunks.clone(); + file.added_lines = changed_file.added_lines.clone(); + file.added_ranges = changed_file.added_ranges.clone(); + } } } } - } - all_files - }; + all_files + }; let mut arc_files = vec![]; log::info!("Giving attention to the following files:"); for file in files { @@ -132,6 +134,8 @@ pub async fn run_main(args: Vec) -> Result<()> { rest_api_client.end_log_group(); let mut clang_params = ClangParams::from(&cli); + clang_params.format_review &= is_pr; + clang_params.tidy_review &= is_pr; let user_inputs = FeedbackInput::from(&cli); let clang_versions = capture_clang_tools_output( &mut arc_files, @@ -145,12 +149,8 @@ pub async fn run_main(args: Vec) -> Result<()> { .post_feedback(&arc_files, user_inputs, clang_versions) .await?; rest_api_client.end_log_group(); - if env::var("PRE_COMMIT").is_ok_and(|v| v == "1") { - if checks_failed > 1 { - return Err(anyhow!("Some checks did not pass")); - } else { - return Ok(()); - } + if env::var("PRE_COMMIT").is_ok_and(|v| v == "1") && checks_failed > 1 { + return Err(anyhow!("Some checks did not pass")); } Ok(()) } @@ -191,6 +191,7 @@ mod test { "false".to_string(), "-v".to_string(), "debug".to_string(), + "-i=target|benches/libgit2".to_string(), ]) .await; assert!(result.is_ok()); @@ -217,6 +218,7 @@ mod test { "cpp-linter".to_string(), "-l".to_string(), "false".to_string(), + "-i=target|benches/libgit2".to_string(), ]) .await; assert!(result.is_err()); diff --git a/cpp-linter/tests/paginated_changed_files.rs b/cpp-linter/tests/paginated_changed_files.rs index 682e170..c2eefec 100644 --- a/cpp-linter/tests/paginated_changed_files.rs +++ b/cpp-linter/tests/paginated_changed_files.rs @@ -5,6 +5,7 @@ use mockito::Matcher; use tempfile::{NamedTempFile, TempDir}; use cpp_linter::{ + cli::LinesChangedOnly, common_fs::FileFilter, logger, rest_api::{github::GithubApiClient, RestApiClient}, @@ -74,7 +75,7 @@ async fn get_paginated_changes(lib_root: &Path, test_params: &TestParams) { let mut server = mock_server().await; env::set_var("GITHUB_API_URL", server.url()); env::set_current_dir(tmp.path()).unwrap(); - logger::init().unwrap(); + logger::try_init(); log::set_max_level(log::LevelFilter::Debug); let gh_client = GithubApiClient::new(); if test_params.fail_serde_event_payload || test_params.no_event_payload { @@ -138,7 +139,9 @@ async fn get_paginated_changes(lib_root: &Path, test_params: &TestParams) { } let file_filter = FileFilter::new(&[], vec!["cpp".to_string(), "hpp".to_string()]); - let files = client.get_list_of_changed_files(&file_filter).await; + let files = client + .get_list_of_changed_files(&file_filter, &LinesChangedOnly::Off) + .await; match files { Err(e) => { if !test_params.fail_serde_diff { diff --git a/cpp-linter/tests/paginated_changes/push_files_pg1.json b/cpp-linter/tests/paginated_changes/push_files_pg1.json index 8022a1e..b10268e 100644 --- a/cpp-linter/tests/paginated_changes/push_files_pg1.json +++ b/cpp-linter/tests/paginated_changes/push_files_pg1.json @@ -19,11 +19,10 @@ "status": "modified", "additions": 11, "deletions": 10, - "changes": 21, + "changes": 0, "blob_url": "https://github.com/cpp-linter/test-cpp-linter-action/blob/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.cpp", "raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.cpp", - "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/src%2Fdemo.cpp?ref=635a9c57bdcca07b99ddef52c2640337c50280b1", - "patch": "@@ -1,17 +1,18 @@\n /** This is a very ugly test code (doomed to fail linting) */\n #include \"demo.hpp\"\n-#include \n-#include \n+#include \n \n-// using size_t from cstddef\n-size_t dummyFunc(size_t i) { return i; }\n \n-int main()\n-{\n- for (;;)\n- break;\n+\n+\n+int main(){\n+\n+ for (;;) break;\n+\n \n printf(\"Hello world!\\n\");\n \n- return 0;\n-}\n+\n+\n+\n+ return 0;}" + "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/src%2Fdemo.cpp?ref=635a9c57bdcca07b99ddef52c2640337c50280b1" } ] } diff --git a/cspell.config.yml b/cspell.config.yml index 72c0cd0..465ff02 100644 --- a/cspell.config.yml +++ b/cspell.config.yml @@ -8,6 +8,7 @@ words: - bugprone - chrono - codecov + - codspeed - consts - cppcoreguidelines - cstdio @@ -31,6 +32,7 @@ words: - mkdocs - msvc - napi + - nextest - nonminimal - peekable - pkgs diff --git a/docs/Cargo.toml b/docs/Cargo.toml index 986d4af..e829d14 100644 --- a/docs/Cargo.toml +++ b/docs/Cargo.toml @@ -16,3 +16,4 @@ pyo3 = "0.23.3" [lib] name = "cli_gen" crate-type = ["cdylib"] +bench = false