-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Cppcheck 2.2 #3424
Add Cppcheck 2.2 #3424
Conversation
All green in build 1 (
|
Some configurations of 'cppcheck/2.2' failed in build 2 (
|
The conan generated z3 package file does not provide the cmake definitions that cppcheck expects: https://github.com/Z3Prover/z3/blob/master/cmake/Z3Config.cmake.in
All green in build 4 (
|
Failure in build 5 (
|
config.yml syntax error in build 6:
|
This reverts commit 5fdcb4a.
Some configurations of 'cppcheck/2.2' failed in build 7 (
|
Some configurations of 'cppcheck/2.2' failed in build 8 (
|
Some configurations of 'cppcheck/2.2' failed in build 9 (
|
Failure in build 10 (
|
I am unsure why the build fails, any tips? |
$ conan search -r conan-center z3*
Existing package recipes:
z3/4.8.8
$ conan search -r conan-center -rev z3/4.8.8@
Revisions for 'z3/4.8.8' at remote 'conan-center':
0d84ad48876496eb900c5ad9a028367e (2020-08-25 11:38:27 UTC) Matches the original PR: #1451 😖 I can see a few minor changes, let's see if rebuilding will do the trick |
# Setup libraries to link | ||
set(Z3_LIBRARIES CONAN_PKG::z3) | ||
set(PCRE_LIBRARY CONAN_PKG::pcre) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these for? I dont see where they are being used 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are normally set in https://github.com/danmar/cppcheck/blob/488813d00fab8278a78d518cf0d3d6651c8f0caa/cmake/findDependencies.cmake and used in various places. Using the standard mechanism leaves out some other dependencies (in case of PCRE) or uses the wrong name (in case of z3).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh thank you for the insight, I did not get to that file while searching the repo! Awesome work!
recipes/cppcheck/all/conanfile.py
Outdated
os.rename(extracted_dir, self._source_subfolder) | ||
|
||
def _configure_cmake(self): | ||
cmake = CMake(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we ask to cache the CMake object, example: https://github.com/conan-io/conan-center-index/pull/3086/files#diff-ec045c054e4b94f8729715c3ec798f88b6dbda611dea1dd091597e2fa3d615bfR24
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
recipes/cppcheck/all/conanfile.py
Outdated
cmake = CMake(self) | ||
cmake.definitions["USE_Z3"] = self.options.with_z3 | ||
cmake.definitions["HAVE_RULES"] = self.options.have_rules | ||
cmake.definitions["USE_MATCHCOMPILER"] = self.settings.build_type == "Release" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at upstream, I am not 100% sure is this makes sense
https://github.com/danmar/cppcheck/blob/488813d00fab8278a78d518cf0d3d6651c8f0caa/cmake/options.cmake#L2
can you please confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the matchcompiler to "auto. The rest I left as is, I think those are correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly what I was looking at, just the github UI showing more.
$<TARGET_OBJECTS:simplecpp_objs_sanitized> | ||
$<TARGET_OBJECTS:tinyxml_objs_sanitized> | ||
$<TARGET_OBJECTS:lib_objs_sanitized>) | ||
- target_include_directories(fuzz-client PRIVATE ${CMAKE_SOURCE_DIR}/lib ${CMAKE_SOURCE_DIR}/externals/simplecpp ${CMAKE_SOURCE_DIR}/externals/tinyxml ${CMAKE_SOURCE_DIR}/externals) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like to recommend keeping patches small. It appears that some of these are not relevant to building.
Would you mind trying to cut of the ones which are not a hard requirement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reduced it to dmake, thank you for all your comments!
Co-authored-by: Chris Mc <[email protected]>
Co-authored-by: Chris Mc <[email protected]>
Co-authored-by: Chris Mc <[email protected]>
All green in build 15 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 LGTM !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disclaimer, I did not test this !
# Setup libraries to link | ||
set(Z3_LIBRARIES CONAN_PKG::z3) | ||
set(PCRE_LIBRARY CONAN_PKG::pcre) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this won't work. The z3 package is exposed as z3_LIBRARIES not Z3_LIBRARIES (ref. original Z3Config.cmake.in https://github.com/Z3Prover/z3/blob/master/cmake/Z3Config.cmake.in).
I could patch more files in cppcheck but I think it is nicer to solve it outside that project.
As for PCRE I think as cppcheck uses find_library dependencies of PCRE (such as mpir) are not linked in correct? I'm not 100% sure but I remember mpir linker errors. I will try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing PCRE leads to errors as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cppcheck uses find_library to find pcre, so if you want this to work, you need to add the cmake_path
generator to the recipe, and add include(conan_paths.cmake)
to this CMakeLists.txt , as I suggested earlier
Regarding Z3, there is indeed an incompatibility in the varibales generated by conan. this is conan-io/conan#7691 striking again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scratch that. this won't work as you pointed out, because find_library won't guess the dependencies. Too bad the upstream project uses cmake variables and find_library, instead of cmake targets :/
# Setup libraries to link | ||
set(Z3_LIBRARIES CONAN_PKG::z3) | ||
set(PCRE_LIBRARY CONAN_PKG::pcre) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this won't work. The z3 package is exposed as z3_LIBRARIES not Z3_LIBRARIES (ref. original Z3Config.cmake.in https://github.com/Z3Prover/z3/blob/master/cmake/Z3Config.cmake.in).
I could patch more files in cppcheck but I think it is nicer to solve it outside that project.
As for PCRE I think as cppcheck uses find_library dependencies of PCRE (such as mpir) are not linked in correct? I'm not 100% sure but I remember mpir linker errors. I will try.
Some configurations of 'cppcheck/2.2' failed in build 16 (
|
This reverts commit 259e3cf.
# Setup libraries to link | ||
set(Z3_LIBRARIES CONAN_PKG::z3) | ||
set(PCRE_LIBRARY CONAN_PKG::pcre) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scratch that. this won't work as you pointed out, because find_library won't guess the dependencies. Too bad the upstream project uses cmake variables and find_library, instead of cmake targets :/
All green in build 17 (
|
Yeah that's a shame, when I find some time I will try to update cppcheck :) |
Specify library name and version: cppcheck/2.2
conan-center hook activated.
Deprecates https://github.com/bincrafters/conan-cppcheck_installer
I removed the z3 option as the original z3 (https://github.com/Z3Prover/z3/blob/master/cmake/Z3Config.cmake.in) provides some definitions that the conan z3 recipe does not provide, while cppcheck uses them. The best way to solve this is to add these to the z3 conan package right?