Revert "CMake cleanup (#7658)" [skip ci]

[firewave]

This reverts commit 554b98a.

[sonarqubecloud]

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]

This "improvement" introduces way too many unwanted side effects.

Here's an (incomplete as I still had the time and mental stability to deal with all of these changes yet):

• we now generate actual libraries (actually we don't in all cases because it doesn't actually in all case)
  • that might stall or slow down the build during linking
    • it introduces additional linking. if you change any file in lib it needs to link all libraries which dependent on it
  • the introduced dependencies reduce the parallelisms in the project since the target have to be complete before other files can be build (most visible with using ninja)
  • having these actual libraries causes dependencies which now need to build for targets which actually don't need those. we already had a similar problem before with the dmake target which required workarounds as the problem is not really understood yet. and we now need to add more of those workaround to restore the status quo
  • there should also be no separate libraries since the version we depend on is fixed and cannot be replaced so they should never be shared libraries
• BUILD_CORE_DLL is broken
  • it was introduced to mirror the behavior of the include Visual Studio project (in hope of dropping it in favor of CMake) but has never been tested. It is also not clear if we even need this. with the changes. the DLL is now also different from the one in the Visual Studio project as it requires the additional DLLs we build (which we should not build at all). so that option should be removed (and building a DLL in the Visual Studio project) should be removed
• no tickets/releasenotes were created for the behavior/breaking changes
• it is not clear if this might affect any of the packagers now that we allow shared libraries which we do not test and essentially never supported.

This is just a brain dump for now I will add to this in the next few days so it is in one place and not the mess as in the original PR. Also there is still more things to look at and the CI builds still need to be compared.

[pfultz2]

These issues can be addressed without reverting the PR. I do have #8016 to address the BUILD_CORE_DLL issue, and I am working on some other updates to try to address the other issues.

It does make it much easier to make changes to the build, like the fix in #8015 is a one-line change whereas before it would require modifying lots of files.

[pfultz2]

the introduced dependencies reduce the parallelisms in the project since the target have to be complete before other files can be build (most visible with using ninja)

Let me check this. The ninja generator in cmake should build in parallel across dependencies before linking, so this shouldn't be an issue.

[firewave]

These issues can be addressed without reverting the PR. I do have #8016 to address the BUILD_CORE_DLL issue, and I am working on some other updates to try to address the other issues.

As I stated multiple times that option should probably should have never existed and the Visual Studio project should be adjusted. That would actually have some upsides unlike the CMake changes.

It does make it much easier to make changes to the build, like the fix in #8015 is a one-line change whereas before it would require modifying lots of files.

That argument doesn't fly because as the issue which triggered this again was "just" a code bug caused by missing CI coverage. So it has nothing to do with how CMake is structured but just with stuff in the usage matrix which is not covered. And the CMake changes blows up the matrix we need to test. And it requires a lot of follow-up changes to restore the state we had before so as I stated this is just wasting time. This hours/days/weeks/months of discussions and work is to potentially(!!!) avoiding to specify a string in more than one place.

Let me check this. The ninja generator in cmake should build in parallel across dependencies before linking, so this shouldn't be an issue.

It does not. Before it was just building everything until it arrived at the first linking (static libraries are being linked - and with GCC it is slow). Now it stops at the library and does not continue until the linking has finished. Probably because the target must be complete before it can use the properties stored in it.

[pfultz2]

So it has nothing to do with how CMake is structured

It does, since its a one line fix instead of multi-line multi-file change.

[pfultz2]

It does not. Before it was just building everything until it arrived at the first linking (static libraries are being linked - and with GCC it is slow). Now it stops at the library and does not continue until the linking has finished. Probably because the target must be complete before it can use the properties stored in it.

I dont see that. I am using cmake 3.28. You might need to set -DCMAKE_OPTIMIZE_DEPENDENCIES=On(I didnt set it though, but it could help).

It builds cppcheck-core, cli, frontend, simplecpp, and tinyxml2 in parallel. Then it links those and goes and builds the testrunner and cppcheck. That is because of dmake. If I use -DDISABLE_DMAKE=On then it builds everything in parallel and links at the very end.