<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/113805/">https://git.reviewboard.kde.org/r/113805/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 12th, 2013, 7:24 p.m. UTC, <b>Alexander Neundorf</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">IMO the patch as it is is not good.
Several things:
1) This file, is not mandatory at all with KDE frameworks.
You can build applications using KDE frameworks libraries without it. You (the developer of the application) simply has to not-load the file KDECompilerSettings.
If the developer has decided to load this file, it is not surprising that he gets modified compiler flags, because this is what he decided to do.
2) You removed e.g. the flags for the build types COVERAGE and PROFILE. CMake doesn't provide these build types or flags itself, so the patch removes support for these buildtypes. I don't see the need to remove support for profile or coverage builds. CMake itself comes with "debug", "minsizerel", "release" and "relwithdebinfo".
3) You remove the flags for Linux and/or gcc. Why didn't you remove them for other compilers/operating systems ?
I know that what we are doing in this file is not the cmake-recommended way.
>From cmake, the variables for the flags are cache variables which are set to some default. The idea is that the person who compiles some package can adjust them to his liking. This is from my experience not what we expect from the average KDE contributor. He should get a working set up, with known compiler flags so it is easy to fix buid bugs (or avoid them in the first place).
By simply setting the normal (non-cache) variables, the person building the package can set the cache variables to whatever he wants, it will be overridden to what is set in KDECompilerSettings.cmake.
Maybe the way this is done could be improved by doing something like
if(NOT DEFINED SOME_CXX_FLAGS_ALREADY_PRESET)
set(SOME_CXX_FLAGS_ALREADY_PRESET TRUE CACHE BOOL "docs..." )
set(SOME_CXX_FLAGS "--some-flag --another-flag" CACHE STRING "docs..." FORCE)
endif()
This way it would be only on the initial cmake run forced into the cache, and afterwards the user should be able to change them as he wants.
</pre>
</blockquote>
<p>On November 12th, 2013, 7:38 p.m. UTC, <b>Sune Vuorela</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">1) THis file is used by all kde frameworks, so it should not put in surprises for the developers there. ONe shouldn't be 100% familiar with all the internals to hack on stuff.
2) IF we want to add such things, it should be in a "ECMAddProfileBuildType" or similar.
3) For the other compilers, the build types aren't touched.
ANd note that this doesn't modify the flags that covers everything. That I'm saving for another patch.
And let us do thhings the cmake way, one step at a time.</pre>
</blockquote>
<p>On November 12th, 2013, 8:07 p.m. UTC, <b>Alexander Neundorf</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">1) I don't really understand. You say "no surprises" and at the same time you say that developers shouldn't have to be familiar with all internals to hack on stuff. If you want "no surprises", remove the line include(KDECompilerSettings) from the project. That's why it has been separated out, to make it optional for users who don't want it. Then you get plain cmake, and can/have to take care of the compiler settings yourself. Add that line, and you get "no need to care about internals".
Is your plan also to remove the added defintions, linker flags, etc. ?
I'm fine if you post a patch which removes the file completely. I just doubt that the KDE community will be happy with this.
This is the state as it was May 2006:
http://quickgit.kde.org/?p=kdelibs.git&a=blob&hb=5713bc5ddd7f11ef73e63cf67c4a0ba69180ef3a&f=cmake%2Fmodules%2FFindKDE4Internal.cmake
You'll see that it was quite different from what we have today. It is much less than today. Back then I also started with a minimal set of flags to get KDE built at all. But between then and now, all those changes have gone in for a reason, each single one of them.
P.S. I am not objecting, just giving comments.
</pre>
</blockquote>
<p>On November 12th, 2013, 8:41 p.m. UTC, <b>Sune Vuorela</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">1) By no surprises I mean by 3rd party users, skilled in Qt and cmake, of a KDE framework - like if I end up using one at work and some of my colleagues need to fix a bug or add a feature, then this would be a unpleasant surprise when dealing with a standalone framework.
My plan isn't - yet - to remove the file completely, but first to slice it down to a size where one can see what happens and what side effects it has. I have at least concrete plans for two or three more chunks to remove, but I prefer slice it down chunks at a time.</pre>
</blockquote>
<p>On November 12th, 2013, 8:55 p.m. UTC, <b>Alexander Neundorf</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">1) Let's assume karchive. You have currently at least the following options
find_package(KArchive REQUIRED NO_MODULE)
This finds the library, KDECompilerSettings.cmake is not involved at all.
OR
find_package(KF5 REQUIRED NO_MODULE COMPONENTS KArchive)
This finds the library, via tier1/kf5umbrella/, KDECompilerSettings.cmake is not involved at all.
OR
(when ECM still had FindKF5.cmake)
find_package(KF5 REQUIRED NO_MODULE COMPONENTS Compiler KArchive)
This will find the library via FindKF5.cmake, and load the compiler flags, because this was requested.
You were aware of that, right ?
P.S. the last option has SAFAIK been changed to the following:
find_package(ECM REQUIRED NO_MODULE)
include(KDECompilerSettings)
find_package(KArchive REQUIRED NO_MODULE)
As above, this finds the library and loads the compiler settings, since this was requested.
OR
AFAIK today:
find_package(ECM REQUIRED NO_MODULE)
include(KDECompilerSettings)
find_package(KF5 REQUIRED NO_MODULE COMPONENTS KArchive)
As above, this finds the library via tier1/kf5umbrealla/ and loads the compiler settings, since this was requested. Using kf5umbrella brings some conveniences when using multiple KF5 libs.
</pre>
</blockquote>
<p>On November 12th, 2013, 8:58 p.m. UTC, <b>Sune Vuorela</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'm not talking about the simple using, but we all know that there will be bugs in our libraries and we all hope that we can get library users to help fix them. For that, we need to make it possible to see what's going on.</pre>
</blockquote>
<p>On November 12th, 2013, 9:31 p.m. UTC, <b>Alexander Neundorf</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I don't see clearly in which way this is related to this patch. Can you elaborate ?
</pre>
</blockquote>
<p>On November 16th, 2013, 2:26 a.m. UTC, <b>Nicolás Alvarez</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">"Outside" people would want to contribute to KDE Frameworks or other parts of KDE, and having 'Debug' mean something different than in any other CMake-based build system is yet another 'surprise' for a new contributor.</pre>
</blockquote>
<p>On November 16th, 2013, 4:16 p.m. UTC, <b>Alexander Neundorf</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think you are making up problems.
I don't think that it is an obstacle in any way to a contributor that the Debug buildtype uses somewhat different flags than the default ones coming from cmake.
I'm not even sure how many projects are using the default flags as they are coming with cmake. E.g. at work we defined our own very specific set of compiler flags for the build types.
As I said before, what I personally would like, would be an extension of the file so that the value end up in the cache, and so the user can modify them, and the same switch might by used to skip the file at all.
With that, the compiler flags could be set as in any other cmake buildsystem by the user via the cache.
But as I said, these are just comments, I'm not maintaining this.
</pre>
</blockquote>
<p>On November 16th, 2013, 8:24 p.m. UTC, <b>Alexander Neundorf</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I just checked what the default flags are with gcc:
default, no build type set:
/usr/bin/c++ -o CMakeFiles/hello.dir/hello.cpp.o -c hello.cpp
debug:
/usr/bin/c++ -g -o CMakeFiles/hello.dir/hello.cpp.o -c hello.cpp
release:
/usr/bin/c++ -O3 -DNDEBUG -o CMakeFiles/hello.dir/hello.cpp.o -c hello.cpp
relwithdebinfo:
/usr/bin/c++ -O2 -g -DNDEBUG -o CMakeFiles/hello.dir/hello.cpp.o -c hello.cpp
minsizerel:
/usr/bin/c++ -Os -DNDEBUG -o CMakeFiles/hello.dir/hello.cpp.o -c hello.cpp
I regularly miss at least -Wall from this.
So IMO "it is different from default cmake" is not a strong argument here.
</pre>
</blockquote>
<p>On November 24th, 2013, 12:22 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ah, this is a good point.
Here's what I would like to see:
* ECM provides a ECMStricterFlags.cmake
* It does not *undo* things done on purpose by cmake, e.g. Debug means "no optimization" (and therefore debugfull can be removed).
* But it improves things by adding the useful set of warnings that prevent writing buggy code (e.g. -Wall, -Werror=return-type, etc.). I definitely want this set of flags to be on by default for all KDE developers, rather than just "making it possible for people to add their own flags".
* It also improves Debug by adding -g3.
This combines "no surprises by undoing what cmake does", with "having useful warnings".
Stricter compilation flags is not KDE specific, we want developers to use them outside KDE too, for better code quality everywhere. These would be "the compiler flags as deemed useful by the ECM community" :-)</pre>
</blockquote>
<p>On December 16th, 2013, 3:53 p.m. UTC, <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'd like to resume discussion on this request.
Regarding build types: I think it is a bad idea to alter CMake behavior at such a generic level: things named the same should behave the same. For example phonon, libnm-qt or attica all uses CMake but do not use KDECompilerSettings. This means when building them with CMAKE_BUILD_TYPE=Debug, you end up with different flags than when build kio.
It makes sense to me to provide Profile and Coverage build types: these are specific tasks which are not supported out of the box by CMake for now, so they fill a hole. DebugFull is similar: CMake provides Debug, but since it is not enough for certain tasks, we can provide an alternative.
Regarding -W flags: why are we discussing them in this request? the patch does not remove them, it only affects build-related flags. -Wall and friends are still defined in CMAKE_C_FLAGS and CMAKE_CXX_FLAGS. Moving them to a KDE-independent file could be a nice idea, but that's another topic IMO.</pre>
</blockquote>
<p>On December 18th, 2013, 5:03 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I fully agree with your second paragraph.
However, the Profile and Coverage build types were never useful to me. In fact, I officially claim that Profile has always been broken: it doesn't include -O2. Profiling must be done on optimized code, otherwise one ends up doing the work that the compiler will do anyway. I always used RelWithDebInfo for profiling (-g -O2). Coverage isn't used every day, and can be set up using CXXFLAGS instead.
Arguably, the same is true with debugfull (one can set CXXFLAGS instead). So I'm fine either way (debugfull stays, or debugfull is out).</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Given what David said, I'm inclined to say we should ditch all the build profile stuff, and just have the CMake defaults. If we decide later that actually, it would be useful to have a Profile or Coverage build, we can add it back (and try to do it "properly" and in its own ECM module, which we may want to include in KDECompilerSettings).
However, Alex's other points about the documentation, other compilers and other variable bits in the file still need to be dealt with.</pre>
<br />
<p>- Alex</p>
<br />
<p>On November 11th, 2013, 10:16 p.m. UTC, Sune Vuorela wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Build System and KDE Frameworks.</div>
<div>By Sune Vuorela.</div>
<p style="color: grey;"><i>Updated Nov. 11, 2013, 10:16 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
extra-cmake-modules
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Do not change the build types available with cmake.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Built kdelibs on linux with gcc.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kde-modules/KDECompilerSettings.cmake <span style="color: grey">(b034751a5be8073f9628971b552faa079c64e8b6)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/113805/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>