Am Montag, 9. Februar 2015, 04:09:59 schrieb Aleix Pol: > Hi, > I just went through the cmake code.
Thanks, Aleix. > Let's see: > - I see this, what does need to be fixed? > # TODO: fix > ecm_generate_headers to support camelcase .h files See https://git.reviewboard.kde.org/r/122317/ , and for related discussion also https://git.reviewboard.kde.org/r/122193 By current feddback in the other RR 122317 seems okay principally, just noone has yet ship-it-ed. Looking forward to someone doing so, so KDiagram can soon make use of the macro also for KChart (yes, could have also worked-around by lowercasing all filenames in the sources, but did not want to change even more things than just the namespace). > - Library targets are usually called KF5*, see KF5Parts or KF5Gantt Even if not part of KF5 itself? So should any libraries in KDE repos prefix their targets like that, because the related ECM macros are used? I would rather think KF5 prefix should be reserved to libs from KF5, but if what you propose is already standard, I will follow. > - Is this really needed? add_definitions(-DKDAB_NO_UNIT_TESTS). It's > not very good to compile differently things if there's unit tests and > not. Not perfect, agreed. Okay if I add a TODO for now? (given this is also in existing released code, and fixing might need some discussions) > - some of the definitions in the root CMakeLists.txt files are already > being defined by KDEFrameworkCompilerSettings. You might want to clean > them up. Fixed, > - misses a .reviewboardrc file. Fixed. > I created a small review request you also want to look into: > https://git.reviewboard.kde.org/r/122495/ Thanks for that, going to comment on tonight. Cheers Friedrich
