<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="http://git.reviewboard.kde.org/r/113805/">http://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>
</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;">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>
<br />
<p>- Sune</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('http://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="http://git.reviewboard.kde.org/r/113805/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>