> 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. > > David Faure wrote: > 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). > > Alex Merry wrote: > 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. > > Alex Merry wrote: > Although keeping debugfull does have the advantage of not causing issues > for developers' build setups (since most developers will be using that, as > per our traditional recommendations). > > David Faure wrote: > Now is the perfect time to change habits and recommendations, so IMHO > this isn't a reason. Well, as soon as we start releasing stuff (e.g. even > TP1) then changing this becomes harder, of course. Which is why getting this > resolved ASAP would be really good :)
If Sune isn't available to do more on this, I have a local updated version of this patch that I could post as a separate review. - Alex ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://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: > https://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: https://git.reviewboard.kde.org/r/113805/diff/ > > > Testing > ------- > > Built kdelibs on linux with gcc. > > > Thanks, > > Sune Vuorela > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
