Repository navigation
Conversation
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
One new problem: splitArgs says it splits "as the shell running the addon does", but it only handles double quotes. On Linux and macOS the addon command is run through popen(), so /bin/sh also strips single quotes. A file given as --rules='rules.txt' (with or without spaces in the name) is still passed to the addon, but Path::isFile gets the path with the quote characters in it and fails, so the file is not hashed. Editing that file then still shows the cached results. I reproduced this. It is a false negative inside a fix that is already partial, and nothing else turned up. The CLI and GUI load addons in the right order relative to buildDir, and the existing tests pass.
Testing
I built cppcheck and testrunner in pr/ without errors. I did not build the GUI.
./testrunner TestCppcheck: 21 tests, 0 failed.python3 -m pytest other_test.py -k builddirintest/cli: 13 passed, 1 xfailed. This includes the newtest_builddir_hash_addon.
Manual check. I wrote a small addon in /work/scratch that reports the content of the file given in --rules=FILE.
myaddon.py:
import sys, json
txt=None
for a in sys.argv[1:]:
if a.startswith('--rules='): txt=open(a[8:]).read().strip()
sys.stdout.write(json.dumps({"file":"t.c","linenr":1,"column":1,"severity":"style","message":str(txt),"addon":"myaddon","errorId":"x","extra":""})+"\n")t.c: void f(int a) { }
The command for every case was:
rm -rf bd; mkdir bd; printf 'V1\n' > rs.txt
cppcheck -q --enable=style --addon=a.json --cppcheck-build-dir=bd --template="{file}:{line}: {message} [{id}]" t.c
printf 'V2\n' > rs.txt
cppcheck ... (same command again)
Results:
"args": ["--rules=rs.txt"]:V1, thenV2. The cache is invalidated correctly."args": ["--rules=\"r s.txt\""]:V1, thenV2. Correct.- Editing
myaddon.pygave the new output. Correct. "args": ["--rules='rs.txt'"]:V1, thenV1. The result is stale, although the addon itself readsrs.txtbecause the shell removes the quotes."args": ["--rules='r s.txt'"]:V1, thenV1. Also stale.
Automated review of 8c748bb by Claude. The pull request was built and tested locally.
| std::vector<std::string> ret(1); | ||
| bool quoted = false; | ||
| for (const char c : args) { | ||
| if (c == '\"') |
There was a problem hiding this comment.
AI review
The addon command goes through popen(), which runs /bin/sh on POSIX systems. The shell also removes single quotes, but splitArgs only handles ". For "args": ["--rules='rs.txt'"] the addon reads rs.txt, while this code checks Path::isFile("'rs.txt'"). That check fails, so the file is not hashed and the build dir keeps returning stale results after rs.txt is edited. I reproduced this both with and without a space in the file name. Either handle single quotes here (at least on non-Windows), or change the comment so it does not claim to match the shell.
…iles) The build dir hash only included the name and the arguments of the addons. Also hash the content of the json file, the script or the executable, and of the files given as OPTION=FILE in the arguments, such as --rule-texts=FILE
8c748bb to
4da7647
Compare
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
The single-quote problem from the earlier review (C1/C2) is fixed. I found one new, minor gap. splitArgs says it splits "as the shell running the addon does", but it does not handle backslash escapes. The addon command is run through popen()//bin/sh on POSIX, so "args": ["--rules=r\\ s.txt"] reaches the addon as --rules=r s.txt. splitArgs instead splits it into --rules=r\ and s.txt, so the file is not hashed, and editing it still gives the cached results. This only affects a niche way of writing addon arguments, inside a fix that is already partial, so it isn't a blocker. Otherwise I found no problems: the CLI and GUI set up the hash after buildDir and the addons are known, and the new tests pass.
Earlier comments
| # | Comment | By | Where | Status |
|---|---|---|---|---|
| 1 | splitArgs ignores single quotes, so a file in --rules='rs.txt' is not hashed | AI review | general | ✅ Fixed |
| 2 | splitArgs does not strip single quotes the way /bin/sh does | AI review | lib/addoninfo.cpp:229 |
✅ Fixed |
Testing
I built cppcheck and testrunner in pr/ without errors. I did not build the GUI.
./testrunner TestCppcheck TestUtils: 0 tests failed.python3 -m pytest other_test.py -k builddirintest/cli: 13 passed, 1 xfailed. This includes the newtest_builddir_hash_addon.
Manual check. I used a small addon in /work/scratch/myaddon.py that reports the content of the file given in --rules=FILE:
import sys, json
txt=None
for a in sys.argv[1:]:
if a.startswith('--rules='): txt=open(a[8:]).read().strip()
sys.stdout.write(json.dumps({"file":"t.c","linenr":1,"column":1,"severity":"style","message":str(txt),"addon":"myaddon","errorId":"x","extra":""})+"\n")t.c: void f(int a) { }
For every case I wrote the rules file with V1, ran the command below, changed the file to V2 and ran it again:
cppcheck -q --enable=style --addon=a.json --cppcheck-build-dir=bd --template="{message}" t.c
"args": ["--rules=rs.txt"]:V1, thenV2. Correct."args": ["--rules='rs.txt'"]and"args": ["--rules='r s.txt'"]:V1, thenV2. The earlier single-quote problem is fixed."args": ["--rules=r\\ s.txt"](filer s.txt):V1, thenV1. The result is stale. The addon readsr s.txtbecause the shell removes the backslash, butsplitArgschecksr\ands.txt, so nothing is hashed."args": ["--rules=$PWD/rs.txt"]:V1, thenV1. The shell expands the variable andsplitArgsdoes not. This is expected given the stated limits; I mention it only for completeness.
Automated review of 4da7647 by Claude. The pull request was built and tested locally.
| return parseAddonInfo(*this, json, fileName, exename); | ||
| } | ||
|
|
||
| // split the args as the shell running the addon does |
There was a problem hiding this comment.
AI review
Minor: /bin/sh also removes backslash escapes, and splitArgs does not. With "args": ["--rules=r\\ s.txt"], the addon receives --rules=r s.txt, but splitArgs gives --rules=r\ and s.txt. The file is then not hashed, and editing it still shows the cached results (reproduced: V1, then V1). Either handle \ outside of single quotes on non-Windows, or make the comment less absolute. Environment variables such as $PWD/rules.txt are not covered either.
|
Test results for commit 4da7647 (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-9029/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
With
--cppcheck-build-dir, the hash only includes the name and the arguments of the addons. After editing the addon script, its json file or a file given in its arguments, cppcheck shows the cached results.This hashes the json file, the script or the executable, and the files given as
OPTION=FILEin the arguments, when the addon is loaded.Still not hashed: an executable found through the
PATH, a file given as a separate argument ("--rule-texts", "FILE"), and the files the script reads or imports itself.t.crules.txtmisra.json{"script": "misra.py", "args": ["--rule-texts=rules.txt"]}Change
V1toV2inrules.txtand run the same command again.Before PR (stale results from the cache):
After PR (cache invalidated):