This was added for feature-parity with Meson-based projects; but an
individual package is the wrong place for that. To avoid the proliferation
of auto-detected ccache support in kde.org packages please submit such
a proposal to kde-frameworks-devel instead, where it could be added for
the benefit of all the projects using ECM. And more importantly, with a
standard switch to disable it for packaging environments
This reverts commit 007c2a08523887cf9c0445a288ad82994bd02a57.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
ccache should never be enabled by default for packaging builds.
Why?
As it currently stands, I'm against the suggested patch. If you want to make it opt-in, then you can as well remove the whole block of code related to ccache, because if a user wants it, they can manually tweak CC/CXX env. variables or whatnot. The whole point of having this block of code is autodetection + opt-out.
It makes the build unpredictable and may interfere with existing ccache setups (hint: some package manager already enables ccache for all packages if asked to). It may also lead to random build failures unless the cache is cleared manually.
If anything, the better approach might be to make it an ECM option, default off, document it in the Wiki, and anyone's build script including kdesrc-build may enable it globally, to not have various methods of enabling ccache pop up in random repositories. This time after review.
It makes the build unpredictable and may interfere with existing ccache setups (hint: some package manager already enables ccache for all packages if asked to). It may also lead to random build failures unless the cache is cleared manually.
It works well for Mesa, Xorg, libinput, and other freedesktop projects, Geary, GTK4… Basically, any Meson-based project has ccache autodetection, and Mesa even use it for CI.
What makes you think Konsole is different?
And packaging is disabling it.
What makes konsole different to other kde.org projects? As said, make an effort to get it into ECM, and everyone profits.
And packaging is disabling it.
Ok, so, I think there's some confusion between us. You're talking about "packaging" however here you want to make a commit into Konsole sources, i.e. to have influence on both packaging and building.
And either way, this statement doesn't seem to be correct: see, here's a script for packaging Mesa in Archlinux, it doesn't disable ccache anywhere.
What makes konsole different to other kde.org projects?
Well, given the context of modifying the CMakeLists.txt I guess the answer is: having a distinct CMakeLists.txt file? 😄
As said, make an effort to get it into ECM, and everyone profits.
Thanks, currently I don't have plans in doing so, although I'm trying to do what I can in terms of my free time, experience, and motivation. E.g. recently I created this task.
There's no confusion here, and I also wouldn't expect Arch to set global standards in a package script. Packagers like default switches and no surprises. If it is clear that ECM uses ccache, and it provides a standard switch to enable or disable it, then this is perfectly fine.
That is perfectly analogous to your ccache change; you wouldn't enable this per-package either, but globally. Our packaging for cmake-based projects has switched to ninja for some time already, with only minimal fallout in terms of build failures.
Packagers like default switches and no surprises. If it is clear that ECM uses ccache, and it provides a standard switch to enable or disable it, then this is perfectly fine.
I agree that the more standardized the better. But there always are options that only exists for one apps and not for others. This is one of them. The question is: how important it is that it's enabled by default? What does it affect?
For this one:
some package manager already enables ccache for all packages if asked to
I can answer that running ccache ccache g++ works same way as just ccache g++; and either way, this is not a reason to disable it, but rather to add another check to the file.
Do you have an example where having ccache on by default would cause a problem? When I asked what makes Konsole different from Mesa, libinput, etc, I was expecting to see an example.
If it is clear that ECM uses ccache, and it provides a standard switch to enable or disable it, then this is perfectly fine.
Btw, I just figured that even if ECM agrees all packages should use ccache — what would happen then? I'm not sure that the task of adding ccache autodetection to all existing projects is that automatable. It would be a lot of merge requests, and who's gonna address review? And what to do if some requests were accepted, and then someone addresses review to modify the changes: what to do with already accepted code…?
I'm not saying it's impossible, but it sounds like something that may take looong, time, and sure I'd prefer to have Konsole builds to be more quick at that timespan.
ccache is nothing new, you can imagine it will be a hot topic on a source-based distribution, so we have years of experience with it. Issues are mostly sandbox violations as are typically used for packaging builds, but also build errors, so much that on bug reports it is regularly demanded to rebuild without ccache.
No, it can be as simple as getting it approved into KDECMakeSettings.cmake or KDECompilerSettings.cmake, whatever fits best, and after upgrade to an ECM version with your change every project already using ECM will automatically have the option available. And enabling or disabling that stuff will be a matter for the person or script running make, not by cluttering dozens of individual CMakeLists.txt files.
What do we do here then going further, am I supposed to apply a downstream patch for konsole 19.12.x?
What do we do here then going further, am I supposed to apply a downstream patch for konsole 19.12.x?
Sorry, after the discussion, I personally can't state that I agree or disagree. On one hand, you haven't posted links to bugreports you mentioned that turned out to be because of ccache *(I'm curious because I still don't understand why all Meson-based packages do not have these problems)*. On the other hand, you do sound like you know what you're talking about. So I'd prefer stay neutral here, and let others comment.
After this patch, you'd need to add -DENABLE_CCACHE=TRUE to enable ccache - I don't really see this as a big hurdle and 19.12 is around the corner so I'd like some consensus here.
After this patch, you'd need to add -DENABLE_CCACHE=TRUE to enable ccache - I don't really see this as a big hurdle
Indeed this isn't a hurdle, but there's not much point in having it as opt-in because, excluding long-time Konsole developers, almost nobody gonna remember to use it. Originally, when I added this as an opt-out, I wanted some feature-parity with Meson-based projects, which do autodetection.
Btw, if you do decide to make it opt-in, you don't need to surround it with the new if()…endif(). The proper way would be to replace ON word with OFF at this line:
set(CCACHE_SUPPORT ON CACHE BOOL "Enable ccache support")
Okay, +1 for the revert then.
That said, I don't think I gonna have time in near future for re-submitting a ccache proposal to kde-frameworks-devel instead, so if somebody wants this improvement on cmake, please go for it.
Oh, btw, sorry for possible confusion: I am @hiangel , this is my another account. As for why I have two of them: this is a bug in KDE account system, I can't login with "hiangel" name, because for some reason the system uses another name, something like k.kharlamov, or kkharlamov, I don't even know what is it… It's a long story, but to be short: randomly chosen "kkharlamov" gives me this another account I had to create at some point.
Is there really any reason to fully revert? Why not just make it not the default per the above comment.
set(CCACHE_SUPPORT OFF CACHE BOOL "Enable ccache support")
As noted by @kkharlamov, this will be then only known to insiders. We'll have to remove it anyway once implemented in ECM, better to make a clean cut now. I plan to test how it could work.