> 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.
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. - 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
