> 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. >
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. - Alexander ----------------------------------------------------------- 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
