#12254: cppcheck.cfg can't be loaded from relative paths anymore #5753
Conversation
|
Well - this is not really a hotfix but a partial revert.. But I checked the code and it is unlikely to cause any issues. 👍 I have local changes which will address this in a more proper way very soon. |
|
so the "format" complained. check the "runformat" script for instructions how to compile proper uncrustify version.. |
Could you elaborate, I don't understand how this makes it worse. It does solve the bug I've seen. My intention is that cppcheck.cfg is put in the same folder as the cppcheck binary and it's not something users should touch/maintain. |
I was referring to the relative path in the added test. Sorry for not being clear about that. Also the other issues I was referring to is that the |
Ah.. yes I agree about that. |
| product_name = 'Cppcheck Premium ' + str(time.time()) | ||
|
|
||
| test_cfg = lookup_cppcheck_exe() | ||
| if(test_cfg.endswith('.exe')): |
There was a problem hiding this comment.
This is very bad and dangerous. If the test gets aborted beforehand you have a stray file which will infer with your system. It might also override a file the user might have created for themselves.
A better way to test this would probably be to copy the folder where the binary is located in into a new folder within tmpdir.
There was a problem hiding this comment.
We already do something similar in other tests with the local cppcheck_local() - which is not something I would encourage. I will try to provide a shared helper which will generate a copy for the execution and update the existing tests.
| """.replace('NAME', product_name)) | ||
|
|
||
| if os.path.isfile('cppcheck'): | ||
| os.chdir('test/cli') |
There was a problem hiding this comment.
Just no. If you need to change the directory for running a process please pass the cwd parameter to the call. cppcheck() might already expose that. If it doesn't please pass that through as a an additional positional parameter.
| assert stdout == product_name + '\n' | ||
| assert stderr == '' | ||
|
|
||
| os.remove(test_cfg) |
There was a problem hiding this comment.
If you put the folder as suggested into tmpdir this is no longer necessary.
| f.write(""" | ||
| { | ||
| "addons": [], | ||
| "productName": "NAME", |
There was a problem hiding this comment.
Use {} placeholder and call format() on the string instead. You can also get rid of the product_name variable if you do not use it in any other way and just fold it in here.
There was a problem hiding this comment.
it's also used for an assert on line 863.
There was a problem hiding this comment.
it's also used for an assert on line 863.
I knew I overlooked it somehow.
Still you should use format() instead of replace().
|
This blocks release testing of cppcheck together with the premium addon.. I want to merge the fix now and then I will merge the test later when it's acceptable.. |
As mentioned in other comments I have local changes which refactor this which are very straight forward ... except for the GUI usage of it which is why I didn't post an PR for it yet. |
Hot fix