[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-frameworks-devel
Subject: Re: Review Request 113805: Do not change the build types available with cmake
From: "David Faure" <faure () kde ! org>
Date: 2013-12-18 17:03:57
Message-ID: 20131218170357.26912.61487 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Nov. 12, 2013, 7:24 p.m., Alexander Neundorf wrote:
> > 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.
> >
> >
>
> Sune Vuorela wrote:
> 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.
>
> Alexander Neundorf wrote:
> 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.
>
>
> Sune Vuorela wrote:
> 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.
> Alexander Neundorf wrote:
> 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.
>
> Sune Vuorela wrote:
> 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.
> Alexander Neundorf wrote:
> I don't see clearly in which way this is related to this patch. Can you elaborate ?
>
>
> Nicolás Alvarez wrote:
> "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.
> Alexander Neundorf wrote:
> 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.
>
>
> Alexander Neundorf wrote:
> 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.
>
>
> David Faure wrote:
> 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" :-)
> Aurélien Gâteau wrote:
> 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.
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).
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113805/#review43538
-----------------------------------------------------------
On Nov. 11, 2013, 10:16 p.m., Sune Vuorela wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113805/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2013, 10:16 p.m.)
>
>
> Review request for Build System and KDE Frameworks.
>
>
> Repository: extra-cmake-modules
>
>
> Description
> -------
>
> Do not change the build types available with cmake.
>
>
> Diffs
> -----
>
> kde-modules/KDECompilerSettings.cmake b034751a5be8073f9628971b552faa079c64e8b6
>
> Diff: http://git.reviewboard.kde.org/r/113805/diff/
>
>
> Testing
> -------
>
> Built kdelibs on linux with gcc.
>
>
> Thanks,
>
> Sune Vuorela
>
>
[Attachment #5 (text/html)]
<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>
<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>
</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;">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> <br />
<p>- David</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>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic