CMake cleanup #7658

pfultz2 · 2025-07-09T16:58:42Z

This removes the object libraries and uses normal cmake targets. This helps simplifies a lot of the cmake since we can properly propagate usage requirements.

pfultz2 · 2025-07-09T18:30:14Z

Because of the global registration an object library will need to be used for at least cppcheck-core, but we can still wrap it in an interface library so it can propagate usage requirements though.

firewave · 2025-07-09T19:17:25Z

Using interface libraries would be an acceptable modernization (although I personally detest them - I had to many problems with them in the past). We need to test though that things work with the currently specified minimum CMake version and raise it appropriately if necessary.

But if we actually want to support shared libraries there is a lot of work to be done:

visibility handling on non-Windows platforms
needs to add annotation to all exported symbols (currently it was only set for the ones actually used in tests)
needs ABI handling/versioning for the shared objects
adjust the install targets
adjust the shipped Visual Studio project to generate the same output files
adjust the Windows installer
CI jobs to test this on all platforms
... possibly even more things I cannot think off the top of my head

And IMO externals should not be shared libraries.

Also we still have static objects in our code which possibly makes the proper usage of the shared objects impossible. Beside that I reckon there never was and never will be a single user of the shared objects. And the current implementation was only partial.

We should not be supporting shared libraries at all because it just causes more work now and in the future and I do not see any upsides at all. We should be removing features instead of adding and simplifying things instead of making them even more complex (with latter I am referring to the support matrix - not the code).

pfultz2 · 2025-07-09T20:14:39Z

The purpose of the shared libraries would only be for local dev. I wouldn't envision us installing them.

firewave · 2025-07-09T20:27:49Z

The purpose of the shared libraries would only be for local dev.

Please elaborate. I do not see how it would be of any use - especially given that it was never available and nobody complained. And if we want to support that we need to test it i.e. CI workflows. Otherwise it is just more stuff which rots away in this repo.

I wouldn't envision us installing them.

It would be possible but it would not work - so it should not be allowed.

pfultz2 · 2025-07-09T22:07:06Z

So with this change there is new -Wsuggest-attribute=noreturn warnings. I am not sure why we didnt warn before. I dont see us disabling warnings. Do you know why?

pfultz2 · 2025-07-09T22:08:59Z

Please elaborate. I do not see how it would be of any use

Faster linking time.

firewave · 2025-07-10T09:15:26Z

externals/tinyxml2/CMakeLists.txt

 endif()

-target_include_directories(tinyxml2 SYSTEM PUBLIC .)
+target_include_directories(tinyxml2 PUBLIC .)


That flag needs to depend on the EXTERNALS_AS_SYSTEM option.

firewave · 2025-07-10T09:15:43Z

lib/CMakeLists.txt

+# if (NOT CMAKE_DISABLE_PRECOMPILE_HEADERS)
+#     target_precompile_headers(cppcheck-core-private PRIVATE precompiled.h)
+# endif()


Needs to be enabled again before merging.

Yea, that was to test the CI. I was getting error:

../../../lib/CMakeFiles/cppcheck-core-private.dir/cmake_pch.hxx.gch: file not recognized: file format not recognized

Now with it disabled I get an error that there isnt a make rule for it.

I see, you are explicitly building it in selfcheck, but its using the wrong target name.

Switching to the OBJECT library directly fixed this issue.

firewave · 2025-07-10T09:20:33Z

externals/tinyxml2/CMakeLists.txt

-if (BUILD_CORE_DLL)
-    target_compile_definitions(tinyxml2_objs PRIVATE TINYXML2_EXPORT)
-endif()
+add_library(tinyxml2 ${srcs} ${hdrs})


This should be able to become a shared library.

firewave · 2025-07-10T09:20:40Z

externals/simplecpp/CMakeLists.txt

-if (BUILD_CORE_DLL)
-    target_compile_definitions(simplecpp_objs PRIVATE SIMPLECPP_EXPORT)
-endif()
+add_library(simplecpp ${srcs} ${hdrs})


This should be able to become a shared library.

It will if we set BUILD_SHARED_LIBS.

Then everything becomes a shared library which is not what we want. IMO the shared library stuff in the Windows build should be completely dropped. And I still thing this should not be real libraries since they are not actually reusable.

firewave · 2025-07-10T09:24:04Z

frontend/CMakeLists.txt


-add_library(frontend_objs OBJECT ${hdrs} ${srcs})
-target_include_directories(frontend_objs PRIVATE ${PROJECT_SOURCE_DIR}/lib)
+add_library(frontend OBJECT ${hdrs} ${srcs})


If this can become a shared library (which it shouldn't) it also needs the import/export handling. This is getting way more complicating than the existing code.

If this can become a shared library (which it shouldn't) it also needs the import/export handling.

Only on windows is that needed. Either way the purpose of this PR is not to support shared libraries.

Unrelated to this comment, this library shouldn't be an object library. I am not sure why its working.

I see why this works. Since cmake 3.12, object libraries can be linked directly instead of adding the $<TARGET_OBJECTS> to the sources. This will make it simpler.

firewave · 2025-07-10T09:25:54Z

Faster linking time.

Yes, that probably will save a few microseconds...

Unlike the precompiled headers you just disabled which will actually save a lot of compilation time...

So with this change there is new -Wsuggest-attribute=noreturn warnings. I am not sure why we didnt warn before. I dont see us disabling warnings. Do you know why?

How do these occur? I did not see them in the CI failures.

firewave · 2025-07-10T09:55:27Z

cli/cppcheckexecutor.cpp

    const int res = _pclose(p);
 #else
-    const int res = pclose(p);
+    int res = pclose(p);


Fixes (among others) in macos-15 builds:

/Users/runner/work/cppcheck/cppcheck/cli/cppcheckexecutor.cpp:746:9: warning: cast from 'const int *' to 'int *' drops const qualifier [-Wcast-qual] 746 | if (WIFEXITED(res)) { | ^ /Applications/Xcode_16.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.0.sdk/usr/include/sys/wait.h:152:26: note: expanded from macro 'WIFEXITED' 152 | #define WIFEXITED(x) (_WSTATUS(x) == 0) | ^

Failing builds on warnings is tracked via https://trac.cppcheck.net/ticket/12021.

Its needed to fix CI, because they became errors in the CI with these changes. I am not sure why it didnt error out before in the CI.

The scope is getting way too big with a lot of questionable sidecar changes. This needs to be fixed more incrementally.

Its needed to fix CI, because they became errors in the CI with these changes. I am not sure why it didnt error out before in the CI.

That should not be happening. That indicates that something is wrong in these CMake change.

pfultz2 · 2025-08-11T22:03:20Z

@firewave Are you able to review this?

sonarqubecloud · 2025-08-11T22:03:38Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

firewave · 2025-08-12T11:48:09Z

@firewave Are you able to review this?

Thanks for the extensive description.

I will take another look at this soon - need to take care of things which piled up in the past few weeks.

The first time I looked at this I saw some different shortcomings in the existing code which need to be singled out/addressed first but I cannot remember what it was.

My aim is to have this resolved in this dev cycle.

gruenich · 2025-08-17T17:35:24Z

I think most changes are an improvement and should be merged.
I would drop the non-CMake changes (74f853f, 91afb23) from this pull request and create one for them alone.
Several of the commits should be squashed together and get a better commit message with some explanations. (If nobody is willing, I can do so and prove an alternative pull request).

pfultz2 · 2025-08-18T03:08:30Z

I would drop the non-CMake changes (74f853f, 91afb23) from this pull request and create one for them alone

The changes are needed to fix the compiler warnings in CI otherwise this PR cant be merged.

Several of the commits should be squashed together and get a better commit message with some explanations. (If nobody is willing, I can do so and prove an alternative pull request).

All of the commits are usually squashed when it gets merge.

firewave · 2025-08-18T08:57:53Z

The changes are needed to fix the compiler warnings in CI otherwise this PR cant be merged.

That's why I am working on making the CI bail out on compiler warnings (and fixing them) right now. When that is done this will be next.

pfultz2 · 2025-09-18T14:06:51Z

I removed the warning fix.

pfultz2 · 2025-09-19T14:42:23Z

@firewave I think this can be merged now.

firewave · 2025-09-23T10:01:48Z

Should be rebased after the warnings stuff is all in (I hope that is finally done by tomorrow). And I think I have looked into most of the side tracks of this already - will have another look at it later.

firewave · 2025-09-23T12:42:15Z

While looking into something completely unrelated I stumbled into some shortcomings in the GUI CMake which tied back into #7835, so that probably also needs to be merged first. 😕

pfultz2 · 2025-09-26T19:16:42Z

Should be rebased after the warnings stuff is all in (I hope that is finally done by tomorrow). And I think I have looked into most of the side tracks of this already - will have another look at it later.

I merged the latest last week, and there is no need for warnings fix. Everything is green, this can be merged in now.

While looking into something completely unrelated I stumbled into some shortcomings in the GUI CMake which tied back into #7835, so that probably also needs to be merged first.

No, that fix should build on top of this. This PR has already been open for almost 3 months, it should be merged in now.

firewave · 2025-10-07T15:36:38Z

I am currently out with a worsening cold.

pfultz2 · 2025-10-28T16:48:03Z

@firewave @danmar Can you this be merged now? Its been open long enough and it causing merge conflicts.

firewave · 2025-10-28T17:05:15Z

Still a lot of things in here I have to look at more closely which I unfortunately didn't get around to yet. And there's still way more important things for me to look into then this.

firewave · 2025-10-28T17:05:40Z

tools/dmake/CMakeLists.txt

 )
-target_include_directories(dmake PRIVATE ${CMAKE_SOURCE_DIR}/cli ${CMAKE_SOURCE_DIR}/lib)
-target_externals_include_directories(dmake PRIVATE ${CMAKE_SOURCE_DIR}/externals/simplecpp)
+target_link_libraries(dmake cppcheck-core cli simplecpp)


This will get rid of the matchcompiler hack but I do not like that it now requires the whole libraries to build this.

If you dont want the whole library then it needs to be separated into a smaller library that can be shared among the components instead. That can be done in a later PR.

firewave · 2025-10-28T17:08:28Z

cli/CMakeLists.txt

@@ -1,68 +1,37 @@
-if (BUILD_CLI)


This looks like we will do CLI stuff in CMake even if the CLI is disabled and that seems to defeat the purpose to disable it.

Its a dependency for dmake which seems required even if CLI is disabled.

In latest HEAD I am able to build the GUI without the building CLI. Will this work still?

No, because dmake requires FileLister from cli library, and dmake is always built so its always needed.

The previous hacks just cause a long chain of other problems and hacks. It also breaks the modularity of the components in cppcheck.

This can be solved by either moving FileLister into cppcheck-core or making a seperate library for FileLister. I figure this could be handled by a later PR to minimize the changes in this PR.

firewave · 2025-10-28T17:11:48Z

gui/test/resultstree/CMakeLists.txt

+if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
+    # caused by mocks
+    target_compile_options_safe(test-resultstree -Wno-suggest-attribute=noreturn)
+endif()
+
+


Duplicated - can be removed.

firewave · 2025-10-28T17:12:54Z

lib/CMakeLists.txt

 endif()
 if (Boost_FOUND)
-    target_include_directories(cppcheck-core SYSTEM PRIVATE ${Boost_INCLUDE_DIRS})
+    target_include_directories(cppcheck-core SYSTEM PUBLIC ${Boost_INCLUDE_DIRS})


Boost should not be spilled from this library.

Its used in the header files. If you dont want it to "spill out" then you will need to refactor it to stay in a .cpp file only. That can be done in a later PR.

firewave · 2025-10-28T18:22:04Z

Still a lot of things in here I have to look at more closely which I unfortunately didn't get around to yet. And there's still way more important things for me to look into then this.

Some clarification. Because of ongoing various personal issues it's often hard for me to concentrate on bigger tasks so I tend to look at some of my (not the one of others) older stuff/backlog or low-hanging fruits which usually requires less focused work rather than just some cleaning up. And I also try to avoid things which involve discussions. It also annoys me especially in case of external submissions.

sonarqubecloud · 2025-10-28T18:47:17Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

pfultz2 · 2025-10-28T23:38:05Z

Some clarification. Because of ongoing various personal issues it's often hard for me to concentrate on bigger tasks so I tend to look at some of my (not the one of others) older stuff/backlog or low-hanging fruits which usually requires less focused work rather than just some cleaning up. And I also try to avoid things which involve discussions. It also annoys me especially in case of external submissions.

Sure, hope you feel better soon. I dont think you need to be a blocker for this.

@danmar @chrchr-github @orbitcowboy Can you review and merge this PR since @firewave is not able to right now?

pfultz2 added 4 commits July 9, 2025 11:56

CMake cleanup

a72f0e8

Add missing cmake file

7536500

Fix cmake

9524828

Consistent link libraries

aae3fa0

Use object library

e5f8f8c

Use targets

599a54e

pfultz2 added 6 commits July 9, 2025 17:25

Remove const to fix cast

74f853f

Remove noreturn

91afb23

Fix pcre linking

00c462a

Add frontend

0e7f402

Remove SYSTEM

ca5c192

Disable precompile headers for now

be40581

firewave reviewed Jul 10, 2025

View reviewed changes

pfultz2 added 5 commits July 10, 2025 09:42

Remove OBJECT

e690613

Use target_external_include_directories

151d3d0

Update target name

0276fd8

Add verbose flag

25d8719

Remove interface library

f95486d

pfultz2 added 3 commits September 18, 2025 08:52

Merge

b8bd740

Add back const

f211acd

Remove line

5c47f9e

firewave removed the merge-after-next-release Wait with merging this PR until after the next Release label Sep 23, 2025

pfultz2 mentioned this pull request Sep 26, 2025

replaced deprecated qt_wrap_ui() with AUTOUIC property #7835

Draft

pfultz2 mentioned this pull request Sep 27, 2025

added CMake option USE_INT128 and support built-in __int128 / some compilation fixes #7840

Draft

Merge branch 'main' into cmake-cleanup

8193d33

Merge

56c3699

firewave reviewed Oct 28, 2025

View reviewed changes

Remove duplicate line

872f922

Uh oh!

CMake cleanup #7658

Are you sure you want to change the base?

CMake cleanup #7658

Uh oh!

Conversation

pfultz2 commented Jul 9, 2025

Uh oh!

pfultz2 commented Jul 9, 2025

Uh oh!

firewave commented Jul 9, 2025

Uh oh!

pfultz2 commented Jul 9, 2025

Uh oh!

firewave commented Jul 9, 2025

Uh oh!

pfultz2 commented Jul 9, 2025

Uh oh!

pfultz2 commented Jul 9, 2025

Uh oh!

firewave Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firewave commented Jul 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfultz2 Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfultz2 commented Aug 11, 2025

Uh oh!

sonarqubecloud bot commented Aug 11, 2025

Quality Gate passed

Uh oh!

firewave commented Aug 12, 2025

Uh oh!

gruenich commented Aug 17, 2025

Uh oh!

pfultz2 commented Aug 18, 2025

Uh oh!

firewave commented Aug 18, 2025

Uh oh!

pfultz2 commented Sep 18, 2025

Uh oh!

pfultz2 commented Sep 19, 2025

Uh oh!

firewave commented Sep 23, 2025

Uh oh!

firewave commented Sep 23, 2025

Uh oh!

pfultz2 commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

firewave Jul 10, 2025 •

edited

Loading

pfultz2 Jul 10, 2025 •

edited

Loading

pfultz2 commented Sep 26, 2025 •

edited

Loading

pfultz2 Oct 28, 2025 •

edited

Loading