CMake cleanup #7658
CMake cleanup #7658
Conversation
|
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. |
|
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:
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). |
|
The purpose of the shared libraries would only be for local dev. I wouldn't envision us installing them. |
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.
It would be possible but it would not work - so it should not be allowed. |
|
So with this change there is new |
Faster linking time. |
externals/tinyxml2/CMakeLists.txt
Outdated
| endif() | ||
|
|
||
| target_include_directories(tinyxml2 SYSTEM PUBLIC .) | ||
| target_include_directories(tinyxml2 PUBLIC .) |
There was a problem hiding this comment.
That flag needs to depend on the EXTERNALS_AS_SYSTEM option.
lib/CMakeLists.txt
Outdated
| # if (NOT CMAKE_DISABLE_PRECOMPILE_HEADERS) | ||
| # target_precompile_headers(cppcheck-core-private PRIVATE precompiled.h) | ||
| # endif() |
There was a problem hiding this comment.
Needs to be enabled again before merging.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see, you are explicitly building it in selfcheck, but its using the wrong target name.
There was a problem hiding this comment.
Switching to the OBJECT library directly fixed this issue.
| if (BUILD_CORE_DLL) | ||
| target_compile_definitions(tinyxml2_objs PRIVATE TINYXML2_EXPORT) | ||
| endif() | ||
| add_library(tinyxml2 ${srcs} ${hdrs}) |
There was a problem hiding this comment.
This should be able to become a shared library.
There was a problem hiding this comment.
That should have said "should not be able".
| if (BUILD_CORE_DLL) | ||
| target_compile_definitions(simplecpp_objs PRIVATE SIMPLECPP_EXPORT) | ||
| endif() | ||
| add_library(simplecpp ${srcs} ${hdrs}) |
There was a problem hiding this comment.
This should be able to become a shared library.
There was a problem hiding this comment.
It will if we set BUILD_SHARED_LIBS.
There was a problem hiding this comment.
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.
frontend/CMakeLists.txt
Outdated
|
|
||
| add_library(frontend_objs OBJECT ${hdrs} ${srcs}) | ||
| target_include_directories(frontend_objs PRIVATE ${PROJECT_SOURCE_DIR}/lib) | ||
| add_library(frontend OBJECT ${hdrs} ${srcs}) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Unrelated to this comment, this library shouldn't be an object library. I am not sure why its working.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Doesn't that mean that we do not actually need real libraries in CMake?
Yes, that probably will save a few microseconds... Unlike the precompiled headers you just disabled which will actually save a lot of compilation time...
How do these occur? I did not see them in the CI failures. |
| const int res = _pclose(p); | ||
| #else | ||
| const int res = pclose(p); | ||
| int res = pclose(p); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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? |
|
I am not good at cmake. I really hope others can review this more properly. |
As I said earlier, this pull request is an improvement to Cppcheck and could be improved itself. It is difficult to grasp the changes and their effect. It could be restructured and split into multiple pull requests. Several commits could be squashed together. If you are interested, I create these pull requests from this one as an alternative proposal. |
that will not help me. :-( |
I have other serious things looming and need to get more important stuff and all my local stuff cleaned up as it might result in me not being able to contribute anything at all for an undetermined time... Well, as I stated. I am the one which mainly has to work with CMake and I am not fine with some of these changes. I tried several times to get into it because the scope of these changes was too big. If you want the more modern approach I probably cannot really argue against it even if I don't think it actually makes things better. But it should not be real libraries. As I argued multiple times before that makes sense as they are not usable as such and especially there should be no support for shared libraries (the cross-dependencies actually would make that not usable/feasible) at all (and we should remove that from our Visual Studio project as well). All that requite an additional matrix in the CI to make sure it actually works but it is not something we should support. This is a dangerous precedent. |
|
You also did not properly remove the Also the additional review was more like "i like this" and not an actual review. We have blocked/revered changes in the past because a single objections I wonder why that is suddenly different. |
| if(BUILD_SHARED_LIBS) | ||
| add_library(cppcheck-core ${srcs_lib} ${hdrs}) | ||
| else() | ||
| # A OBJECT library is used because the auto-registration doesn't work with static libraries | ||
| add_library(cppcheck-core OBJECT ${srcs_lib} ${hdrs}) | ||
| endif() |
There was a problem hiding this comment.
So this is not even a real library when not being a shared one. So this is inconsistent and it won't even work as expected.
There was a problem hiding this comment.
What do you mean work as expected?
There was a problem hiding this comment.
It is not a real library unlike the other libraries. So either make them all real ones or keep the object ones. Not some mish-mash otherwise these changes feel pointless.
|
Please revert this - it is a bad and incomplete change and too sudden change and severely lacks tests and CI jobs to make sure that (totally unnecessary ) functionality it adds actually works. This needs to be properly and incremental. But this only causes necessary work now and in the future and should not be persuaded at all. |
|
And the packaging might have also needed additions. |
|
Even if this should stay in we still need to revert it and make it complete in a PR or we will have bunch of spreaded out commits patching this up. This has been done in the past and but that's no longer how this project is doing things nowadays. This also lacks documentation informing about the behavior changes. And iz should have been a ticket as well given how serious of a change this is. |
I dont see the issue here. To say they are not usable is just completely false, as they are being used right now in the current CI testing. So they are usable.
The shared library support is only a side effect of doing things properly in cmake. This PR isnt trying to add support for shared libraries.
What are you talking about? CMake already manages the dependencies. It does work on linux. Some small tweaks such as adding
Its not something we support. If developers in the future find this useful we could consider supporting it and adding CI testing for it. Its not necessary now since its not supported and no one is using it.
You should take some time to think calmly about these changes as this seems quite exaggerated.
You have had plenty of time since july to review this PR. This helps clean up the cmake to make it easier for other contributors to make changes to the cmake as well. It will also help prevent bugs in the future like in #7655, since all usage requirements are encapsulated nicely.
You said you weren't able to review it so we found others to review it. |
👍 it's never wrong with a ticket. all bigger changes should have tickets. |
That's why I kept my time looking into it but there is just too much stuff in here which requires more looking into and actual trying it out - and there is just more important things to spend that time on (simplecpp is still broken in several ways and Daniel plans a new release - the time I spent on reviewing/commenting I could have spent on that instead). It should have been several smaller changes. But nonetheless it is still bad change since it lacks a ticket, tests, and documentation (and IMO a deprecation intermediate step). Also was this tested with the oldest supported CMake version? That is also something which needs to be added to the CI (regardless of any changes here). https://trac.cppcheck.net/ticket/14259 I got more annoyed as I discovered more things so my language got a bit out of hand. And I had to deal with the fallout of an extremely similar change and circumstances in a past project which only ended up causing me unnecessary headaches and work - along with exposing some serious toxicity (which gladly is not the case here). So this gives me some serious whiplash.
Let's not start the blame game...
No, it won't if we do not have tests for it. That had been broken for years but nobody ever realized it because it wasn't widely used (i.e. it was implicitly supported but not tested). And as pointed out multiple times it was a very simple fix which did not require such a major change. The shared library stuff was never ever tested. I just added it so it is in line with the provided Visual Studio project in the hope that we could drop it. Since that will probably never happen but the DLL was never actually utilized that should have never been added to CMake and dropped from the Visual Studio project (sorry for repeating myself).
Then we need to actively disallow it and not have some implicitly enabled but probably not usable/working. I do not want any tickets about this or any time at all spent on this in the future. That includes any external contribution which built on top of this because that will leave the support up to us and only cause work down the road - if people need such a feature they have to maintain a fork.
There we are at "dangerous precedent". One of the reviewers even mentioned that this could be broken down into a smaller changes. I also gave feedback (albeit still incomplete as I am still not fully grasping all the changes). In case of any ValueFlow-related changes I feel uncomfortable to commit any changes without any feedback from you (another area where we are lacking test coverage). So we should strive for better than simply "others to review" as in some of those cases that could have ended badly. Please publish a follow-up which fixes the issues I reported and adds the missing documentation. If we keep these changes they should at least be complete. |
|
The linking of the executable appears to be slower and the added library generation causes annoying stalls (several seconds) on my slower system when using GCC (which is quite slow to begin with compared to Clang) which did not exist before. I will look into it later. |
Since this is an open source project, we should be careful about explicitly shutting the door on things that the community can reasonably support or maintain, even if the core contributors don’t personally need that path right now. It’s not a one-man show, if someone wants to keep a config/feature alive and is willing to do the CI/tests/maintenance for it, we should leave that possibility open rather than hard-disabling it up front. They shouldn't need to create a fork for this. Furthermore, there is no pressure for you to fix tickets either. The invested parties that are interested can handle those issues. |
Sure, but @chrchr-github and @danmar can handle the reviews as well if I am under the weather so its not a one-man show. I dont even have merge permission to merge those PRs. |
What documentation are you referring to? I didnt make any changes to what we currently document on our readme. |
But the point is that if we do things properly like in this PR, we dont need all the scenarios tested in CI since it should work automatically. |
|
|
||
| target_dll_compile_definitions(cppcheck-core EXPORT CPPCHECKLIB_EXPORT IMPORT CPPCHECKLIB_IMPORT) | ||
|
|
||
| target_include_directories(cppcheck-core PUBLIC .) |
There was a problem hiding this comment.
Why do you need to specify the include folder? That is unnecessary since the compiler will always search next to the file.
The problem is that stuff is often just dumped on the project and then you will never see that contributor again. Recently such contribution have not made it in because there had not been any responses on our feedback. Also the CI is more comprehensive so faulty/bad quality contributions will be immediately visible. Also things which are known not to work should tell you otherwise you think you might have done something wrong or might lead to unexpected results.
Sometimes I feel like I am the only invested party in some areas. I do the work because I want this to be usable (maintainable is something different) and I feel it still isn't in some areas. At my past job it was a really hard sell to keep it in the CI because of the friction it generated on a regular basis compared to other more integrated (but much inferior or even useless) checkers. Performance was also an issue back then and that was when we were still much fast than now. That's what got me back into the project after years away from it. That unfortunately snowballed badly and got me in the position where I still have too many WIP things.
I was not aware of that.
You broke (i.e. incompletely removed)
What? Of course you need to test it. Just because you use a certain pattern/framework stuff won't magically work. If we made a mistake in a target/configuration we never build we will never discover the issue (my point about |
|
I agree with @firewave, this pull request should be reverted. The parts we can agree on should be polished and properly described. Currently it is a mix of different topics, with fixes and revert that should be squashed. The purpose of singe commits is poorly described ("CMake cleanup", "Fix cmake").
|
|
In |
I assumed this is because it builds dmake as well, which requires cli and core(as discussed that here: #7658 (comment)), however, I dont see I am not sure how this |
I dont see how this could be split up into smaller PRs. What you described below doesnt seem to help split it up. I think reverting is overkill here.
It was squashed.
That wasnt the only description. It said: "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.". Unfortunately, I dont have merge access so cant control the commit message there.
Removing the double empty lines was probably just my editor. I dont think it needs to be a seperate commit.
The
There was no C++ changes. Those were all fixed in seperate PRs.
That is the meet of the change. It uses cmake targets to handle dependencies instead of using the hacks and copy and pasting we were using before. Its very difficult to maintain and leads(and has led) to bugs. This makes the dependencies much simpler to reason about, but it leads to more dependencies to be built, unfortunately due to the hacky nature some of the dependencies are done. So we now can say that A depends on B(ie In some cases, we depend on the headers of a library and not the sources, but it becomes problematic when you happen to include another header in the same library that will cause a linker error because the library wasnt linked in. If we want to reduce the dependencies getting built we need to reorganize our dependencies to make them smaller and clearer. Boost had similar but much more severe issues(like circular dependencies) that they worked to resolved when they switched to modular components, but it helped make dependencies much easier to reason about. We can definitely do the same here as we have a lot smaller number of components than boost. |
It makes the CMake code nicer but the CMake usage worse and slows down builds and the CI. So this is not really improving things. It also seems to reduce the parallelism of the build with more cores as it need to finish building the target because it can build the targets which depend on it. Even with Clang it is annoying but GCC is it just horrible as it is so damn slow.
"perhaps", "assume", "not really documented" - this obfuscation by the modernization makes it even harder to understand what is going on. You made the changes and you don't even understand what it is doing now. Welcome to my world - this is exactly what I had to deal with in the past after such a modernization. This is such a waste of time ... and I have not even dug into everything. |
|
Personally I feel it's unfortunate that cli/cmdlineparser.cpp is built even though I explicitly ask to not build the cli and only the gui. It's something I can live with but it's unfortunate.
ok spontanously I don't know if this is actually used but it shouldn't be removed unless we think it through. It reduces the size of the windows installer significantly right?
It does sound unfortunate that BUILD_CORE_DLL was never tested in the CI.
As a cmake noob this sounds good to me. And in your last section you talked about smaller and clearer dependencies that sounds really good..
maybe that is the best way forward? I assume there is a plan on how to split it up and polish it.. |
5 participants
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.