D29573: ECMGenerateExportHeader: add generation of *_DEPRECATED_VERSION_BELATED()

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  So many things to take care of :-)

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  addbelated

REVISION DETAIL
  https://phabricator.kde.org/D29573

To: kossebau, #frameworks, #build_system, dfaure
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, bencreasy, 
michaelh, ngraham, bruns

D29524: ECMGeneratePriFile: fix for ECM_MKSPECS_INSTALL_DIR being absolute

[David Faure]

dfaure closed this revision.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D29524

To: dfaure, cgiboudeaux, vatra, kfunk, apol, vkrause
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, bencreasy, 
michaelh, ngraham, bruns

D29524: ECMGeneratePriFile: fix for ECM_MKSPECS_INSTALL_DIR being absolute

[David Faure]

dfaure created this revision.
dfaure added reviewers: cgiboudeaux, vatra, kfunk, apol, vkrause.
Herald added projects: Frameworks, Build System.
Herald added subscribers: kde-buildsystem, kde-frameworks-devel.
dfaure requested review of this revision.

TEST PLAN
  works as before for the case where it's relative.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29524

AFFECTED FILES
  modules/ECMGeneratePriFile.cmake

To: dfaure, cgiboudeaux, vatra, kfunk, apol, vkrause
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, bencreasy, 
michaelh, ngraham, bruns

D29274: ECMGeneratePriFile: make the pri files relocatable

[David Faure]

dfaure closed this revision.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D29274

To: dfaure, vatra, kfunk, apol, vkrause
Cc: ablu, kossebau, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, 
bencreasy, michaelh, ngraham, bruns

D29096: Prefix includes and libs dir with QT_SYSROOT

[David Faure]

dfaure added a comment.


  I'm using conan, not doing cross compilation.
  
  But with relative paths it's no problem, both use cases work.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D29096

To: ablu, #build_system, apol, vkrause, kfunk
Cc: dfaure, kossebau, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, 
bencreasy, michaelh, ngraham, bruns

D29096: Prefix includes and libs dir with QT_SYSROOT

[David Faure]

dfaure added a comment.


  Thanks for the quick test!
  
  I'll indeed proceed with the other one, which is more generic (our use case 
isn't related to QT_SYSROOT).
  
  Actually: can you add your comment (that it works for you) in the other 
review request, so people know it's been tested by more than me, and discard 
this one?

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D29096

To: ablu, #build_system, apol, vkrause, kfunk
Cc: dfaure, kossebau, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, 
bencreasy, michaelh, ngraham, bruns

D29274: ECMGeneratePriFile: make the pri files relocatable

[David Faure]

dfaure updated this revision to Diff 81519.
dfaure added a comment.


  Fix error: regex "[^/]*" matched an empty string.

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29274?vs=81506=81519

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29274

AFFECTED FILES
  modules/ECMGeneratePriFile.cmake

To: dfaure, vatra, kfunk, apol
Cc: ablu, kossebau, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, 
bencreasy, michaelh, ngraham, bruns

D29096: Prefix includes and libs dir with QT_SYSROOT

[David Faure]

dfaure added a comment.


  Hi @ablu,
  
  Could you test if my patch in D29274  
solves your problem? By making these lines relative to the location of the .pri 
file, it should work very well in a sysroot context as well.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D29096

To: ablu, #build_system, apol, vkrause, kfunk
Cc: dfaure, kossebau, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, 
bencreasy, michaelh, ngraham, bruns

D29274: ECMGeneratePriFile: make the pri files relocatable

[David Faure]

dfaure created this revision.
dfaure added reviewers: vatra, kfunk, apol.
Herald added projects: Frameworks, Build System.
Herald added subscribers: kde-buildsystem, kde-frameworks-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  Instead of generating
  QT.KArchive.includes = /full/path/include/KF5/KArchive
  make it
  QT.KArchive.includes = $$PWD/../../include/KF5/KArchive
  
  This makes the whole install prefix relocatable after the fact,
  the includes will be found based on where the .pri file ended up.
  
  This is especially useful for Conan support, says Bogdan.

TEST PLAN
  After make install in ECM, cd karchive/examples/helloworld && qmake && make

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29274

AFFECTED FILES
  modules/ECMGeneratePriFile.cmake

To: dfaure, vatra, kfunk, apol
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, bencreasy, 
michaelh, ngraham, bruns

D28409: ECM: attempt to fix KDEInstallDirsTest.relative_or_absolute on Windows

[David Faure]

dfaure closed this revision.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D28409

To: dfaure, kossebau, apol, cgiboudeaux
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, bencreasy, 
michaelh, ngraham, bruns

D28409: ECM: attempt to fix KDEInstallDirsTest.relative_or_absolute on Windows

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> CMakeLists.txt:146
>  set(KDEInstallDirsTest.relative_or_absolute_usr_EXTRA_OPTIONS
>  --build-options -DCMAKE_INSTALL_PREFIX=/usr
>  -DKDE_INSTALL_USE_QT_SYS_PATHS=FALSE

This test uses /usr and passes on Windows.

It's not like those paths have to exist.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D28409

To: dfaure, kossebau, apol, cgiboudeaux
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, bencreasy, 
michaelh, ngraham, bruns

D28409: ECM: attempt to fix KDEInstallDirsTest.relative_or_absolute on Windows

[David Faure]

dfaure created this revision.
dfaure added reviewers: kossebau, apol, cgiboudeaux.
Herald added projects: Frameworks, Build System.
Herald added subscribers: kde-buildsystem, kde-frameworks-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  Not passing CMAKE_INSTALL_PREFIX is a weird thing to do. The test shows
  "Installing in ." and some values like KDE_INSTALL_FULL_EXECROOTDIR
  become "/" which is considered relative on Windows.
  
  The test that passes /usr to CMAKE_INSTALL_PREFIX actually passes on
  Windows. Pass /tmp to the other test, remove the test without prefix.

TEST PLAN
  Passes on Linux, not tested on Windows, CI will do that

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28409

AFFECTED FILES
  tests/CMakeLists.txt

To: dfaure, kossebau, apol, cgiboudeaux
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, bencreasy, 
michaelh, ngraham, bruns

D28253: ECMPoQmToolsTest: have separate moc files for tr_thread_test 1 & 2

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  Thanks!

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  fixtr_thread_tests

REVISION DETAIL
  https://phabricator.kde.org/D28253

To: kossebau, #frameworks, #build_system, dfaure
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, bencreasy, 
michaelh, ngraham, bruns

Re: ECM unittest fails, why?

[David Faure]

Thanks for your reply.

On dimanche 22 mars 2020 19:59:38 CET Johan Ouwerkerk wrote:
> On Sun, Mar 22, 2020 at 10:50 AM David Faure  wrote:
> > But this works fine here. One of the very first things that happen is [ 
> > 2%] Generating tr_thread_test.moc
> > 
> > Any idea?
> > Can anyone reproduce the problem?
> 
> One of the other things that is happening is `/usr/bin/gmake clean`.
> Could that have something to do with it?

Can you reproduce the failure?
I tried inserting a make clean in the instructions from my previous email, 
still passes.

> Another thing that strikes me is that the order is compile -> test ->
> install for the framework, but for comparison for Keysmith it is
> compile -> install -> test.

We used to do that, but it's of course much better if a framework can be 
tested without having to be installed first.
It models what happens to a developer in an IDE: make, and run test(s),
no install necessary.

> This might well be completely unrelated,
> but for Keysmith you do not get any 'noise' of CMake/gmake attempting
> to build stuff during the test run:
> https://build.kde.org/job/Extragear/job/keysmith/job/kf5-qt5%20SUSEQt5.12/22
> /execution/node/48/log/

The "noise" only shows up because of the test failure.
That's because of export CTEST_OUTPUT_ON_FAILURE=1, quite convenient to debug 
failures.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

ECM unittest fails, why?

[David Faure]

The CI for ECM has been failing ever since the tests were re-enabled.

https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.12/job/extra-cmake-modules/job/kf5-qt5%20SUSEQt5.12/103/console
shows that ECMPoQmToolsTest fails with "tr_thread_test.moc: No such file or 
directory".

I have been trying to reproduce this locally, but no such luck.
* Compiled cmake from git tag v3.16.2, to match the exact version used by CI, 
and prepended its install prefix to my $PATH
* In a clean clone of ECM :

$ mkdir build; cd build
$ cmake -DCMAKE_BUILD_TYPE=Debug -DBUILD_TESTING=ON 
-DCMAKE_INSTALL_PREFIX="/home/jenkins//install-prefix/" -DBUILD_COVERAGE=ON 
-DECM_ENABLE_SANITIZERS='address' -DBUILD_QCH=TRUE ..
  # ^ this is to match the options given by CI, but the last three have no 
effect in ECM
$ make
$ ctest --verbose -R ECMPoQmToolsTest

But this works fine here. One of the very first things that happen is [  2%] 
Generating tr_thread_test.moc

Any idea?
Can anyone reproduce the problem?

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

D25589: ECMGenerateExportHeader: add NO_BUILD_SET_DEPRECATED_WARNINGS_SINCE flag

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  My head hurts a bit but I think I understand this now ;)
  
  I'm not sure what's the use case for disabling this though? (I'm wondering 
the same about the existing NO_DEFINITION_EXPORT_TO_BUILD_INTERFACE)

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  NO_BUILD_SET_DEPRECATED_WARNINGS_SINCE

REVISION DETAIL
  https://phabricator.kde.org/D25589

To: kossebau, #build_system, #frameworks, dfaure
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D24990: KDEFrameworkCompilerSettings: enable all Qt % KF deprecation warnings

[David Faure]

dfaure added a comment.


  So we need to set FOO_DISABLE_DEPRECATED_BEFORE_AND_AT to N-1 while building 
FOO itself, right? Either magically here, or manually in every module...

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D24990

To: kossebau, #frameworks, #build_system, apol, dfaure
Cc: dfaure, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D24990: KDEFrameworkCompilerSettings: enable all Qt % KF deprecation warnings

[David Faure]

dfaure accepted this revision.
dfaure added a comment.


  Done (in the process of being pushed), you can push this.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  enableallqtkfdeprecationwarningsforframeworks

REVISION DETAIL
  https://phabricator.kde.org/D24990

To: kossebau, #frameworks, #build_system, apol, dfaure
Cc: dfaure, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D24882: Don't set C/C++ standards if already set

[David Faure]

dfaure closed this revision.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D24882

To: dfaure, cgiboudeaux
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standard

[David Faure]

dfaure added a comment.


  Does https://phabricator.kde.org/D24882 help? (Not tested)

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D24841

To: vonreth, dfaure, cullmann
Cc: cgiboudeaux, aacid, chehrlic, kde-frameworks-devel, kde-buildsystem, 
LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D24882: Don't set C/C++ standards if already set

[David Faure]

dfaure created this revision.
dfaure added a reviewer: cgiboudeaux.
Herald added projects: Frameworks, Build System.
Herald added subscribers: kde-buildsystem, kde-frameworks-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  Fixes regression from https://phabricator.kde.org/D24841

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D24882

AFFECTED FILES
  kde-modules/KDECompilerSettings.cmake

To: dfaure, cgiboudeaux
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standard

[David Faure]

dfaure added a comment.


  Where did -std=gnu++14 come from? The old code above certainly didn't set it.
  
  Maybe some projects were doing `set(CMAKE_CXX_STANDARD 14)` before including 
KDECompilerSettings? We could test the var here to avoid overwriting it...

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D24841

To: vonreth, dfaure, cullmann
Cc: cgiboudeaux, aacid, chehrlic, kde-frameworks-devel, kde-buildsystem, 
LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standad

[David Faure]

dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  cmake_c_standard

REVISION DETAIL
  https://phabricator.kde.org/D24841

To: vonreth, dfaure, cullmann
Cc: aacid, chehrlic, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standad

[David Faure]

dfaure added a comment.


  Dunno, it's harder to justify in a standalone review :)
  "Increase dependency, just because" ;)

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D24841

To: vonreth, dfaure, cullmann
Cc: aacid, chehrlic, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24841: Use modern way to set the C/CXX standad

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> KDECompilerSettings.cmake:208
> +set(CMAKE_C_STANDARD 90)
> +set(CMAKE_CXX_STANDARD_REQUIRED 11)
>  

That's not how it works.
You want

  set(CMAKE_CXX_STANDARD 11)
  set(CMAKE_CXX_STANDARD_REQUIRED TRUE)

Note however that these require CMake >= 3.1, while ECM says 
`cmake_minimum_required(VERSION 2.8.12 FATAL_ERROR)`. I do however thing that 
ECM *should* require 3.1.
I doubt it's been tested on 2.8.x in ages, and 3.1 is really a common 
requirement these days.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D24841

To: vonreth, dfaure, cullmann
Cc: chehrlic, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24568: Provide clang-format target with a KDE Frameworks style file

[David Faure]

dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D24568

To: cullmann, #frameworks, dfaure
Cc: zzag, sitter, mwolff, ochurlaud, nalvarez, kossebau, aacid, davidedmundson, 
dhaumann, apol, ognarb, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24568: Provide clang-format target with a KDE Frameworks style file

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> cullmann wrote in clang-format.cmake:75
> I have no strong opinion on that, we can remove that or keep it.

(b) looks good to me, and more importantly, I like the goal of being as close 
as possible to the Qt coding style.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D24568

To: cullmann, #frameworks, dfaure
Cc: zzag, sitter, mwolff, ochurlaud, nalvarez, kossebau, aacid, davidedmundson, 
dhaumann, apol, ognarb, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24568: Provide clang-format target with a KDE Frameworks style file

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  I like the idea very much.

INLINE COMMENTS

> KDEClangFormat.cmake:76
> +else()
> +message(STATUS "Could not set up the clang-format target as the 
> clang-format executable is missing.")
> +endif()

I wonder if people compiling KF5 modules (and not necessarily planning on 
contributing) need to be annoyed with such a warning. Maybe we could still 
define the clang-format target and make it print an error?

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D24568

To: cullmann, #frameworks, dfaure
Cc: mwolff, ochurlaud, nalvarez, kossebau, aacid, davidedmundson, dhaumann, 
apol, ognarb, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D24568: Provide clang-format target with a KDE Frameworks style file

[David Faure]

dfaure added a comment.


  Do we want these, found in 
https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format?
  
# We use template< without space.
SpaceAfterTemplateKeyword: false

# macros for which the opening brace stays attached.
ForEachMacros:   [ foreach, Q_FOREACH, BOOST_FOREACH, forever, Q_FOREVER, 
QBENCHMARK, QBENCHMARK_ONCE ]

# Break constructor initializers before the colon and after the commas.
BreakConstructorInitializers: BeforeColon
  
  In general, I'm curious why we're not using qt5's clang-format file, with our 
only difference (braces for single-line statements) on top?

INLINE COMMENTS

> clang-format.cmake:42
> +
> +# sematics shall be grouped via empty lines
> +SortIncludes: true

Typo: semantics

> clang-format.cmake:43
> +# sematics shall be grouped via empty lines
> +SortIncludes: true
> +

I'm confused because here it says "true" will group via empty lines, while 
https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format says `the SortInclude 
feature of clang-format does not re-order includes separated by empty lines`. 
Maybe different versions of clang-format? Or I misunderstand something?

> clang-format.cmake:49
> +# CrlInstruction *a;
> +PointerAlignment: Right
> +

https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format says `PointerBindsToType: 
false`, what's the difference?

> clang-format.cmake:75
> +
> +# don't break tenary ops
> +BreakBeforeTernaryOperators: false

typo: ternary

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D24568

To: cullmann, #frameworks, dfaure
Cc: mwolff, ochurlaud, nalvarez, kossebau, aacid, davidedmundson, dhaumann, 
apol, ognarb, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D23789: Add ECMGenerateExportHeaders, for improved handling of deprecated API

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> ECMGenerateExportHeader.cmake:269
> +# cannot be used to reactivate the declaration, and ``Foo::doWhat`` will not
> +# have been built into the library binary.
> +#

Really? Isn't doWhat excluded only if EXCLUDE_DEPRECATED_BEFORE_AND_AT is set 
to 5.12.0?
With 5.0.0 it's still available, no?

I'm also surprised this paragraph talks about doWhat but not doBar, they both 
get disabled together, right?

> ECMGenerateExportHeader.cmake:276
> +# to the version before and at which all deprecated API has been excluded 
> from
> +# the build,
> +# Even more when building against other libraries from the same group "Bar" 
> and

comma followed by uppercase letter is unusual grammar

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D23789

To: kossebau, #frameworks, #build_system
Cc: chehrlic, dfaure, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, 
LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D23789: Add ECMGenerateExportHeaders, for improved handling of deprecated API

[David Faure]

dfaure added a comment.


  In D23789#541134 , @kossebau wrote:
  
  > In D23789#540033 , @dfaure wrote:
  >
  > > I found confirmation in cmake's 
Tests/RunCMake/GenerateExportHeader/reference/
  >
  >
  > Not sure what you exactly mean, can you please specify confirmation for 
what? And what this recommends us to do? :)
  
  
  It confirms that only MSVC uses a different way to express this, which is why 
nobody handles the mix-and-match case where you'd generate the header for one 
compiler and then consume it with another.
  (unless I'm wrong and mix-and-match between mingw and MSVC is actually 
possible - AFAIK it's not)

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D23789

To: kossebau, #frameworks, #build_system
Cc: chehrlic, dfaure, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, 
LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D23789: RFC: Add ECMGenerateExportHeaders, for improved handling of deprecated API

[David Faure]

dfaure added a comment.


  I found confirmation in cmake's Tests/RunCMake/GenerateExportHeader/reference/
  
$ grep -r LIBSHARED_DEPRECATED\  . | grep -w define
./UNIX_DeprecatedOnly/libshared_export.h:#  define LIBSHARED_DEPRECATED 
__attribute__ ((__deprecated__))
./Win32-Clang/libshared_export.h:#  define LIBSHARED_DEPRECATED 
__attribute__ ((__deprecated__))
./Win32/libshared_export.h:#  define LIBSHARED_DEPRECATED 
__declspec(deprecated)
./UNIX/libshared_export.h:#  define LIBSHARED_DEPRECATED __attribute__ 
((__deprecated__))
./MinGW/libshared_export.h:#  define LIBSHARED_DEPRECATED __attribute__ 
((__deprecated__))

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D23789

To: kossebau
Cc: chehrlic, dfaure, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, 
LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D23789: RFC: Add ECMGenerateExportHeaders, for improved handling of deprecated API

[David Faure]

dfaure added a comment.


  > Which might be an issue for people who would like to use different compiler 
on the same system, both building against the same generated export header file.
  
  I see the theoretical problem, but how could this ever be a problem in 
practice?
  On Unix all compilers support the same syntax (`__attribute__ 
((__deprecated__))`), so you'd have to be on Windows, build a library with 
mingw, and then try to use it with MSVC, or vice-versa? I wonder if this even 
works. AFAIK it doesn't (hence the compiler-specific binary installers for Qt, 
for instance).
  
  Oh and if C++14 support is enabled, we could use `[[deprecated("use foo 
instead")]]` which is standard and portable :-)
  (requires C++17 for enums and namespaces)

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D23789

To: kossebau
Cc: chehrlic, dfaure, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, 
LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D24159: new module ECMSourceVersionControl

[David Faure]

dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  vcs

REVISION DETAIL
  https://phabricator.kde.org/D24159

To: sitter, kde-buildsystem, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

D23791: ECMAddQch: add INCLUDE_DIRS argument

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  I don't know doxygen very well, but the cmake change seems sensible.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  supoortincludedirsinaddqch

REVISION DETAIL
  https://phabricator.kde.org/D23791

To: kossebau, #build_system, dfaure
Cc: dfaure, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D21695: Add FindTaglib.cmake

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> dfaure wrote in FindTaglib.cmake:84
> ( this searches for it both ways, so it's unclear what the include dir 
> will be --- with or without taglib at the end?)

BTW since then it was determined that applications should use , so you 
should NOT search for taglib/tag.h

> dfaure wrote in FindTaglib.cmake:94
> ... and this then adds another /taglib/ to the path.
> 
> I've always had that problem with taglib. It's not clear to me if the C/C++ 
> code should use #include  or #include  and therefore 
> it's not clear whether the include path should contain the /taglib subdir or 
> not.

In light of the above, this should be ${Taglib_INCLUDE_DIRS}/taglib.h

(though it's weird to use a plural form variable in such a way; there should 
probably be a singular form variable in this file, and then the plural one 
exported for users)

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D21695

To: heikobecker, kde-buildsystem, kde-frameworks-devel, dfaure
Cc: ngraham, elvisangelaccio, cgiboudeaux, dfaure, LeGast00n, GB_2, bencreasy, 
michaelh, bruns

D23789: RFC: Add ECMGenerateExportHeaders, for improved handling of deprecated API

[David Faure]

dfaure added a comment.


  I wouldn't offer both variants, but rather recommend always adding a hint. At 
worse that hint can be "See method documentation" or an empty string. I think 
we're just over-complicating things otherwise, for no actual gain, since 
providing a hint is good practice anyway.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D23789

To: kossebau
Cc: dfaure, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
GB_2, bencreasy, michaelh, ngraham, bruns

D23789: RFC: Add ECMGenerateExportHeaders, for improved handling of deprecated API

[David Faure]

dfaure added a comment.


  Great work. Not really easy to grasp at first sight (because it handles BC 
for no-compat builds of the lib itself, which we never did before) but this is 
certainly quite comprehensive.
  
  Since we don't yet have any source compat to worry about for these macros 
(unlike Qt, which needed _X variants for that), how about simply adding a third 
argument to FOO_DEPRECATED_VERSION right away, to allow specifying a hint in 
the compiler warning, provided the compiler supports C++14?
  
  kio's src/core/job_base.h:77 would become
  
KIOCORE_DEPRECATED_VERSION(5, 0, "use uiDelegate()") KJobUiDelegate *ui() 
const;
  
  If your first version just ignores the argument, at least we'll start writing 
those hints and we can work on making cmake's GenerateExportHeaders support 
that, or fork it here to support it.

INLINE COMMENTS

> ECMGenerateExportHeader.cmake:75
> +# use to conditionally set a 
> ``_DEPRECATED``
> +# macro for a class, struct or function (pther elements to be supported in
> +# future versions), depending on the visibility macro flags set (see below)

typo: pther -> other

> ECMGenerateExportHeader.cmake:85
> +# - evaluates to ``TRUE`` or ``FALSE`` depending on the value of
> +# ``EXCLUDE_DEPRECATED_BEFORE_AND_AT`. To be used mainly with ``#if`` &
> +# ``#endif`` to mark sections of implementation code for deprecated API, as

(minor) small inconsistency, here you use '&' as separator between #if and 
#endif, while on line 80 you use '/'.

> ECMGenerateExportHeader.cmake:87
> +# ``#endif`` to mark sections of implementation code for deprecated API, as
> +# well as declaration code of deprecated API only to disable at build time
> +# (e.g. virtual methods, see notes below).

I'm having trouble parsing the "only" in this sentence.

`deprecated API only`... that seems redundant, it's only API that gets 
deprecated :-)

Maybe you mean "to disable only at build time"?
Or "declaration-only code" ?

and it's always about build time... the choice is between building the library 
and building the users of the library, isn't it? It's all "build time" of 
someone...

> ECMGenerateExportHeader.cmake:107
> +# Note: The tricks applied here for hiding deprecated API to the compiler
> +# when building against a library does not work for all deprecated API:
> +#

"The tricks [] does not work" is invalid grammar: either "The trick" 
(singular) or "do not work" (plural).

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D23789

To: kossebau
Cc: dfaure, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
GB_2, bencreasy, michaelh, ngraham, bruns

D23552: ECM: remove set_package_properties from FindCanberra

[David Faure]

dfaure abandoned this revision.
dfaure added a comment.


  Ah, I didn't realize there was no warning if calling set_package_properties 
with different properties.
  
  Good idea. Done in 
https://commits.kde.org/knotifications/06f5a4a6f8622913cb9cecd89b98cf762a489823

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D23552

To: dfaure, cgiboudeaux, sitter
Cc: kossebau, kde-buildsystem, kde-frameworks-devel, apol, aacid, LeGast00n, 
GB_2, bencreasy, michaelh, ngraham, bruns

D23552: ECM: remove set_package_properties from FindCanberra

[David Faure]

dfaure created this revision.
dfaure added reviewers: cgiboudeaux, sitter.
Herald added projects: Frameworks, Build System.
dfaure requested review of this revision.

REVISION SUMMARY
  The user of this find module has to do it, so that they can specify the
  purpose of using canberra in their application. We can't do it twice,
  it leads to
  
  - Warning: Property DESCRIPTION for package Canberra already set to "Event 
sound library", overriding it with "Library for generating event sounds"
  
  The documentation for FeatureSummary always does find_package
  and then set_package_properties, i.e. it was never meant for the
  finder to do this.

TEST PLAN
  knotifications no longer warns

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23552

AFFECTED FILES
  find-modules/FindCanberra.cmake

To: dfaure, cgiboudeaux, sitter
Cc: kde-buildsystem, kde-frameworks-devel, apol, aacid, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D22667: Make the default build type "Debug" when compiling a git checkout.

[David Faure]

dfaure closed this revision.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D22667

To: dfaure, kde-buildsystem, cgiboudeaux
Cc: cgiboudeaux, apol, kde-frameworks-devel, LeGast00n, sbergeron, bencreasy, 
michaelh, ngraham, bruns

D22667: Make the default build type "Debug" when compiling a git checkout.

[David Faure]

dfaure created this revision.
dfaure added a reviewer: kde-buildsystem.
Herald added projects: Frameworks, Build System.
Herald added a subscriber: kde-frameworks-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  The idea comes from
  https://blog.kitware.com/cmake-and-the-default-build-type/
  but I adapted it to leave it empty for tarballs, to avoid messing up
  distribution packaging.
  
  The goal is to make this more sensible for [new] developers who just
  run cmake and end up with a "no debug symbols, no optimizations" build,
  i.e. the one and only completely useless combination of those two flags.
  
  Possible risk: distributions who compile from git checkouts...

TEST PLAN
  "mkdir build ; cd build ; cmake .." in kblog leads to CMAKE_BUILD_TYPE=Debug

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22667

AFFECTED FILES
  kde-modules/KDECompilerSettings.cmake

To: dfaure, kde-buildsystem
Cc: kde-frameworks-devel, LeGast00n, sbergeron, bencreasy, michaelh, ngraham, 
bruns

D21695: Add FindTaglib.cmake

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Certainly a good idea to have this in ECM, so that this mess can be sorted 
out once and for all...

INLINE COMMENTS

> FindTaglib.cmake:56
> +# The minimum version of Taglib we require
> +if(NOT Taglib_FIND_VERSION)
> +set(TAGLIB_FIND_VERSION "1.4")

Here it says Taglib camelcase...

> FindTaglib.cmake:57
> +if(NOT Taglib_FIND_VERSION)
> +set(TAGLIB_FIND_VERSION "1.4")
> +endif()

And here it says TAGLIB uppercase. This can't be right, one of those needs to 
be adjusted.

> FindTaglib.cmake:80
> +set(Taglib_CFLAGS ${TC_Taglib_CFLAGS})
> +string(REGEX REPLACE " *-I" ";" Taglib_INCLUDE_DIRS "${Taglib_CFLAGS}")
> +endif()

This will set the include dir to  FindTaglib.cmake:84
> +find_path(Taglib_INCLUDE_DIRS
> +NAMES tag.h taglib/tag.h
> +HINTS ${TC_Taglib_INCLUDE_DIRS}

( this searches for it both ways, so it's unclear what the include dir will 
be --- with or without taglib at the end?)

> FindTaglib.cmake:94
> +if (Taglib_INCLUDE_DIRS AND NOT Taglib_VERSION)
> +if(EXISTS "Taglib_INCLUDE_DIRS}/taglib/taglib.h")
> +file(READ "${Taglib_INCLUDE_DIRS}/taglib/taglib.h" TAGLIB_H)

... and this then adds another /taglib/ to the path.

I've always had that problem with taglib. It's not clear to me if the C/C++ 
code should use #include  or #include  and therefore it's 
not clear whether the include path should contain the /taglib subdir or not.

> FindTaglib.cmake:129
> +IMPORTED_LOCATION "${Taglib_LIBRARIES}"
> +INTERFACE_COMPILE_OPTIONS "${Taglib_CFLAGS}"
> +INTERFACE_INCLUDE_DIRECTORIES "${Taglib_INCLUDE_DIRS}"

Why set both the CFLAGS and the include dir? That means having the -I twice in 
there.

Line 80 assumes that the cflags will never contain other flags than -I.
If that's true, then using CFLAGS here is redundant.

If it's not true, then line 80 is wrong...

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D21695

To: heikobecker, kde-buildsystem, kde-frameworks-devel, dfaure
Cc: dfaure, LeGast00n, bencreasy, michaelh, ngraham, bruns

D21313: Create specific directory for kdebugsettings categories file

[David Faure]

dfaure added a comment.


  My suggestion is to call this qlogging-categories5.
  
  The 'q' prefix is missing to make this qt-specific rather than cross-toolkit.
  The "logging" instead of "debug" is because this affects all types of 
messages, and this is all about what's called "logging categories" in Qt.
  The "5" will allow co-installation of the qt5 and qt6 versions of the same 
library (but it will require two versions of kdebugsettings, or that 
kdebugsettings looks in both places -- but that can be done later).
  
  I wouldn't name it after the application using this, because well, it could 
change name, and there could be more apps using this (e.g. akonadiconsole, 
kdevelop...).

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D21313

To: mlaurent, dfaure, cgiboudeaux
Cc: kossebau, kde-frameworks-devel, kde-buildsystem, bencreasy, michaelh, 
ngraham, bruns

D21314: Don't enable QT_STRICT_ITERATORS on Windows.

[David Faure]

dfaure closed this revision.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D21314

To: dfaure, vkrause, dvratil
Cc: apol, kde-frameworks-devel, kde-buildsystem, bencreasy, michaelh, ngraham, 
bruns

D21314: Don't enable QT_STRICT_ITERATORS on Windows.

[David Faure]

dfaure updated this revision to Diff 58393.
dfaure added a comment.


  Remove unrelated change

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21314?vs=58392=58393

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D21314

AFFECTED FILES
  kde-modules/KDEFrameworkCompilerSettings.cmake

To: dfaure, vkrause, dvratil
Cc: kde-frameworks-devel, kde-buildsystem, bencreasy, michaelh, ngraham, bruns

D21314: Don't enable QT_STRICT_ITERATORS on Windows.

[David Faure]

dfaure created this revision.
dfaure added reviewers: vkrause, dvratil.
Herald added projects: Frameworks, Build System.
Herald added subscribers: kde-buildsystem, kde-frameworks-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  Strict iterators can't be used on Windows, they lead to a link error
  when application code iterates over a QVector for instance, unless
  Qt itself was also built with strict iterators.
  See example at https://bugreports.qt.io/browse/AUTOSUITE-946
  
  Technically this would be fine for mingw, but not for MSVC neither
  clang-cl (which also uses the MSVC ABI). I think it's fine to just
  disable it for all Windows compilers, since any iterators misuse
  will be detected on Unix anyway.

TEST PLAN
  None, I'm relying on Volker's findings.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D21314

AFFECTED FILES
  kde-modules/KDEFrameworkCompilerSettings.cmake
  modules/ECMAddTests.cmake

To: dfaure, vkrause, dvratil
Cc: kde-frameworks-devel, kde-buildsystem, bencreasy, michaelh, ngraham, bruns

D20349: Unconditionally enable -DQT_STRICT_ITERATORS, not just in debug mode

[David Faure]

dfaure closed this revision.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D20349

To: dfaure, dvratil, mlaurent, aacid
Cc: aacid, kde-buildsystem, kde-frameworks-devel, bencreasy, michaelh, ngraham, 
bruns

D20349: Unconditionally enable -DQT_STRICT_ITERATORS, not just in debug mode

[David Faure]

dfaure added a comment.


  All the modules that use this cmake file, already build fine in debug mode 
with strict iterators.
  This enables the same setting in release mode, for code that already builds 
fine in debug mode. So unless people have bad code in #ifndef NDEBUG, I don't 
expect this to break anything at all.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D20349

To: dfaure, dvratil, mlaurent
Cc: aacid, kde-buildsystem, kde-frameworks-devel, bencreasy, michaelh, ngraham, 
bruns

D20349: Unconditionally enable -DQT_STRICT_ITERATORS, not just in debug mode

[David Faure]

dfaure created this revision.
dfaure added reviewers: dvratil, mlaurent.
Herald added projects: Frameworks, Build System.
Herald added a subscriber: kde-frameworks-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  It turns out QT_STRICT_ITERATORS does not introduce any
  overhead (they are inlined so everything gets optimized), confirmed by
  Dan comparing the produced assembly in Compiler Explorer.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D20349

AFFECTED FILES
  kde-modules/KDEFrameworkCompilerSettings.cmake

To: dfaure, dvratil, mlaurent
Cc: kde-buildsystem, kde-frameworks-devel, bencreasy, michaelh, ngraham, bruns

D19538: ECMGeneratePkgConfigFile: make all vars dependent on ${prefix}

[David Faure]

dfaure closed this revision.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D19538

To: dfaure, kossebau, apol, cgiboudeaux
Cc: kde-frameworks-devel, kde-buildsystem, fazevedo, michaelh, ngraham, bruns

D19506: ECMGeneratePkgConfigFile: add variables used by pkg_check_modules

[David Faure]

dfaure added a comment.


  Makes sense --> https://phabricator.kde.org/D19538

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D19506

To: dfaure, apol, fazevedo, cgiboudeaux
Cc: kossebau, cgiboudeaux, fazevedo, kde-buildsystem, kde-frameworks-devel, 
michaelh, ngraham, bruns

D19538: ECMGeneratePkgConfigFile: make all vars dependent on ${prefix}

[David Faure]

dfaure created this revision.
dfaure added reviewers: kossebau, apol, cgiboudeaux.
Herald added projects: Frameworks, Build System.
dfaure requested review of this revision.

REVISION SUMMARY
  Note that the first test had a comment "without optional arguments"
  so I removed the INCLUDE_INSTALL_DIR variable to test what happens by
  default. The other tests still pass an absolute path there (which is
  unusual in practice, and breaks the idea of using ${prefix}).

TEST PLAN
  `ctest -R PkgConfig` passes

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D19538

AFFECTED FILES
  modules/ECMGeneratePkgConfigFile.cmake
  tests/ECMGeneratePkgConfigFile/KF5CoreAddons.pc
  tests/ECMGeneratePkgConfigFile/KF5CoreAddons2.pc
  tests/ECMGeneratePkgConfigFile/KF5CoreAddons3.pc
  tests/ECMGeneratePkgConfigFile/run_test.cmake.config

To: dfaure, kossebau, apol, cgiboudeaux
Cc: kde-frameworks-devel, kde-buildsystem, fazevedo, michaelh, ngraham, bruns

D19506: ECMGeneratePkgConfigFile: add variables used by pkg_check_modules

[David Faure]

dfaure closed this revision.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D19506

To: dfaure, apol, fazevedo, cgiboudeaux
Cc: cgiboudeaux, fazevedo, kde-buildsystem, kde-frameworks-devel, michaelh, 
ngraham, bruns

D19506: ECMGeneratePkgConfigFile: add variables used by pkg_check_modules

[David Faure]

dfaure updated this revision to Diff 53114.
dfaure added a comment.


  Update unittests, passes now

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19506?vs=53108=53114

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D19506

AFFECTED FILES
  modules/ECMGeneratePkgConfigFile.cmake
  tests/ECMGeneratePkgConfigFile/KF5CoreAddons.pc
  tests/ECMGeneratePkgConfigFile/KF5CoreAddons2.pc
  tests/ECMGeneratePkgConfigFile/KF5CoreAddons3.pc

To: dfaure, apol, fazevedo, cgiboudeaux
Cc: cgiboudeaux, fazevedo, kde-buildsystem, kde-frameworks-devel, michaelh, 
ngraham, bruns

D19506: ECMGeneratePkgConfigFile: add variables used by pkg_check_modules

[David Faure]

dfaure created this revision.
dfaure added a reviewer: apol.
Herald added projects: Frameworks, Build System.
Herald added a subscriber: kde-frameworks-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  If we ever end up in a situation where the generated .pc file is
  actually used by cmake's pkg_check_modules, then we need the
  includedir and libdir variables; pkg_check_modules ignores the
  Makefile-like Libs and Cflags lines of the .pc file.
  
  Context: craft adds a cmake buildsystem for sqlite3, which is then
  found using pkgconfig in akonadi.

TEST PLAN
  Filipe Azevedo is doing all the testing, I'm just providing patches :)

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D19506

AFFECTED FILES
  modules/ECMGeneratePkgConfigFile.cmake

To: dfaure, apol
Cc: kde-buildsystem, kde-frameworks-devel, michaelh, ngraham, bruns

D15795: Fix warnings about deprecated install dirs

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  The warning needs to be removed then. We can't really warn at the time of 
using a variable.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D15795

To: dinoh, apol, dfaure
Cc: ltoscano, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D19075: Fix FindEGL

[David Faure]

dfaure added a comment.


  If anyone hits a cmake error about "/usr/include/EGL/EGL/egl.h" after 
updating ECM (e.g. in kwayland or plasma-framework), remove the cache in the 
builddir and rerun cmake. This commit is actually correct, it's just 
incompatible with existing caches.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D19075

To: tcberner, #automotive, hausmann, #freebsd, apol
Cc: dfaure, bcooksley, kde-frameworks-devel, kde-buildsystem, michaelh, 
ngraham, bruns

Re: ECM tests not run by CI (Re: KDE Frameworks 5.55.0)

[David Faure]

On samedi 9 février 2019 13:48:59 CET David Faure wrote:
> On samedi 9 février 2019 11:25:20 CET Rik Mills wrote:
> > I note that the KDE CI does not seem to run the tests at all?
> 
> Indeed. I looked into it, and here's the reason why:
> 
> [ci-tooling repo]
> commit 45ff1bc4ecb18ad71ad0570f7e838d1ae7cab2e2
> Author: Ben Cooksley 
> Date:   Wed Sep 27 22:04:05 2017 +1300
> 
> Revoke test execution for extra-cmake-modules across all platforms.
> The tests in their current condition are incompatible with being run
> prior to installation

Ben, does that mean a simple solution would be to apply the "make install 
before running tests" principle to ECM, like we used to do (before I asked you 
to change it for all frameworks, not thinking about the ECM special case), and 
like we still do for applications (I think)?

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

ECM tests not run by CI (Re: KDE Frameworks 5.55.0)

[David Faure]

On samedi 9 février 2019 11:25:20 CET Rik Mills wrote:
> I note that the KDE CI does not seem to run the tests at all?

Indeed. I looked into it, and here's the reason why:


commit 45ff1bc4ecb18ad71ad0570f7e838d1ae7cab2e2
Author: Ben Cooksley 
Date:   Wed Sep 27 22:04:05 2017 +1300

Revoke test execution for extra-cmake-modules across all platforms.
The tests in their current condition are incompatible with being run prior 
to installation, and leave the build directory unusable following their 
execution.
This means that brokeness in ECM won't be seen anywhere in the CI system 
until it is picked up the Dependency Rebuild jobs, after which point the impact 
will be KDE wide.
To ensure that the rest of Frameworks is built using the most recent 
version of ECM - and thus ensure any brokeness in ECM is detected early - we 
skip running tests, which allows installation to succeed.

CCMAIL: kde-frameworks-de...@kde.org
CCMAIL: release-t...@kde.org



-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

D16894: [ECM] use a macro to add compiler flags conditionally

[David Faure]

dfaure added a comment.


  In D16894#402209 , @rjvbb wrote:
  
  > >   In that sentence, one can read "if supported" for the macro name, ...
  >
  > That was my idea too, and the reason the macro ends in "_if_supported".
  
  
  Right, but I was saying all this because I think IF_SUPPORTED (the keyword in 
the arguments) should be SUPPORTED_IF.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D16894

To: rjvbb, #build_system, kfunk
Cc: cgiboudeaux, dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, 
#build_system, michaelh, ngraham, bruns

D16894: [ECM] use a macro to add compiler flags conditionally

[David Faure]

dfaure added a comment.


  > SUPPORTED_IF : add the flag(s) if the expression is true?
  
  Yes.
  
  > TRY_IF: query the compiler if the expression is true?
  
  Yes.
  
  > How would that intersect with the SUPPORTED_IF test?
  
  In addition.
  
  AFAICS this is what you implemented, so I like it. I just find SUPPORTED_IF 
(as an intro to the condition) more readable than IF_SUPPORTED.
  
  The macro name seems fine to me.
  In plain English one would say "Add this flag if it's supported. It's 
supported for sure if condition A is true. Also try it if condition B is true, 
since it might work then, but we can't be sure".
  In that sentence, one can read "if supported" for the macro name, "SUPPORTED 
IF" for the first condition, and "TRY IF" for the second condition.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D16894

To: rjvbb, #build_system, kfunk
Cc: dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, 
michaelh, ngraham, bruns

D16894: [ECM] use a macro to add compiler flags conditionally

[David Faure]

dfaure added a comment.


  Ah. You meant an "OR", I thought it was an "AND".   (as in `our known 
selection of compilers OR/AND appleclang supports it`)
  But things are certainly not clearer now with the name CONDITION, which 
doesn't imply either one.
  
  Aside from the AppleClang issue, I've had cases where a compiler flag was 
added in an unreleased version of the compiler, so it makes sense to me to have 
a way to have a TRY_IF condition.
  At the same time, I guess we can have SUPPORTED_IF for the cases where we 
know it's supported and we want to save time by not even checking it.
  Then one could say "TRY_IF apple and clang" for the flags that we want to 
double-check on appleclang, instead of the function having this black magic 
hardcoded inside.
  And this way we don't need to check flags that work on all versions of clang 
like -pedantic.
  
  In summary, I suggest having both SUPPORTED_IF and TRY_IF, and moving 
AppleClang magic out of the function.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D16894

To: rjvbb, #build_system, kfunk
Cc: dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, 
michaelh, ngraham, bruns

D16894: [ECM] use a macro to add compiler flags conditionally

[David Faure]

dfaure added a comment.


  This makes sense to me. Just the name "SUPPORTED_IF" is strange, when reading 
that, one thinks "well, if we know the compiler flag is supported, why are we 
testing that it is?". I think this should be something like TRY_IF.
  Then it's clearer that no harm will occur if we set a too low compiler 
version after TRY_IF, it's just an optimization to avoid e.g. testing all gcc 
flags on MSVC and vice-versa.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D16894

To: rjvbb, #build_system, kfunk
Cc: dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, 
michaelh, ngraham, bruns

D18547: Don't enable -Wzero-as-null-pointer-constant on apple clang

[David Faure]

dfaure accepted this revision.
dfaure added a comment.


  If it fixes the issue, this can go in, but like René says, this is quite 
surprising, since the default behavior (CMP0025 off) is that the compiler is 
called "Clang" on Apple platforms as well.
  See `cmake --help-policy CMP0025`

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D18547

To: vonreth, aacid, apol, dfaure, rjvbb, bcooksley
Cc: aacid, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D17714: Clang: don't enable -Wzero-as-null-pointer-constant on 5.0.0

[David Faure]

dfaure added a comment.


  Damn, I went too fast. This doesn't fix the issue.
  
  CMake says 5.0.300080 is > 5.0.1, obviously.
  
  The reason I enabled it for 5.0.1 is that on x86_64 linux, clang 5.0.1 does 
support it (but now I realize that 5.0.0 does too, most probably).
  I don't understand what weird mess android does with the clang versioning, 
but apparently 5.0.380 is actually a kind of beta of 5.0.0 
https://stackoverflow.com/questions/47552782/llvm-version-on-ios-and-android 
seems to confirm that, in fact.
  
  I guess the bug is my own fault, I didn't see there was a NDK 15c after 15b, 
so I guess one shouldn't use 15b at all.
  I'll revert after confirming that it works with 15c, sorry for the noise.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D17714

To: dfaure, aacid, cgiboudeaux, apol
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D17714: Clang: don't enable -Wzero-as-null-pointer-constant on 5.0.0

[David Faure]

dfaure closed this revision.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D17714

To: dfaure, aacid, cgiboudeaux, apol
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D17714: Clang: don't enable -Wzero-as-null-pointer-constant on 5.0.0

[David Faure]

dfaure created this revision.
dfaure added reviewers: aacid, cgiboudeaux, apol.
Herald added projects: Frameworks, Build System.
Herald edited subscribers, added: kde-buildsystem, kde-frameworks-devel; 
removed: Build System, Frameworks.
dfaure requested review of this revision.

REVISION SUMMARY
  The clang compiler in Android NDK r15b, which has version 5.0.300080,
  warns as follows:
  
  warning: unknown warning option '-Wzero-as-null-pointer-constant'; did you 
mean '-Wint-to-void-pointer-cast'? [-Wunknown-warning-option]

TEST PLAN
  Removing the option removes the warning

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D17714

AFFECTED FILES
  kde-modules/KDEFrameworkCompilerSettings.cmake

To: dfaure, aacid, cgiboudeaux, apol
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D15566: Add QT_NO_NARROWING_CONVERSIONS_IN_CONNECT as default compile flags

[David Faure]

dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  add_QT_NO_NARROWING_CONVERSIONS_IN_CONNECT

REVISION DETAIL
  https://phabricator.kde.org/D15566

To: mlaurent, dfaure
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D15559: Python bindings: use cmake_parse_arguments, fix documentation

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  This is technically not 100% source compatible, but since the callers were 
already passing "TARGET" (and others) as documented, I guess it's fine.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  python_ecm_cleanup

REVISION DETAIL
  https://phabricator.kde.org/D15559

To: bruns, #frameworks, dfaure
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D15566: Add QT_NO_NARROWING_CONVERSIONS_IN_CONNECT as default compile flags

[David Faure]

dfaure added a comment.


  Do all frameworks build with this change?

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D15566

To: mlaurent, dfaure
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

Re: ECM Behaviour: Setting QT_PLUGIN_PATH on Windows

[David Faure]

On vendredi 31 août 2018 10:50:53 CEST Ben Cooksley wrote:
> On Tue, Aug 28, 2018 at 10:35 PM Ben Cooksley  wrote:
> > Hi all,
> 
> Hi all,
> 
> > We currently have a severe bug in ECM's AddTest macro due to it's
> > behaviour around the setting of QT_PLUGIN_PATH. On Windows it would
> > appear that the code in ECMAddTest mangles the environment variable,
> > removing crucial paths that the CI system includes within this
> > variable which are needed for applications and tests to function.
> > 
> > As a consequence of this, several projects tests are broken, and some
> > of these only fail by timeout. This has the effect of blocking Windows
> > CI builders for several hours in some instances simply due to tests
> > that are hanging and timing out as a result of being unable to load
> > plugins.
> > 
> > Can we please either short circuit this logic (and not try to set
> > QT_PLUGIN_PATH on Windows) or figure out the correct logic to be used
> > in this instance?
> > 
> > My suspicion is that CMake treats ';' specially, which means that when
> > the macro re-includes the existing content of the variable it gets
> > mangled by CMake, resulting in the entire content of that variable
> > being rendered unreadable.
> > 
> > Alternatively, CMAKE_LIBRARY_OUTPUT_DIRECTORY could have a trailing
> > backslash, which has certainly caused issues elsewhere.
> 
> Based on the lack of response to this, along with my earlier mail to
> k-f-d 7 days ago it doesn't look like anyone's going to pick this up.
> 
> As this issue is causing test breakages on the CI system including
> tying up valuable builder time for extraordinarily excessive amounts
> of time, i've taken a sledgehammer to the offending code in ECM.

Is there any indication that this code was misbehaving on Unix as well?
I'm surprised that you took out everything, not just the Windows case.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

D8256: Add _XOPEN_SOURCE to C definitions

[David Faure]

dfaure added a comment.


  Indeed,
  
  22:23:51 In file included from /usr/include/sys/mount.h:36:
  22:23:51 /usr/include/sys/ucred.h:84:2: error: unknown type name 'u_int'; did 
you mean 'uint'?
  22:23:51 u_int   cr_version; /* structure layout version 
*/
  22:23:51 ^
  
  
https://build.kde.org/view/Frameworks/job/Frameworks%20kcoreaddons%20kf5-qt5%20FreeBSDQt5.11/5/console
  
  I'll revert.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D8256

To: awilcox, alexmerry, dfaure
Cc: bcooksley, #freebsd, kde-frameworks-devel, kde-buildsystem, mpyne, dfaure, 
michaelh, ngraham, bruns

D8256: Add _XOPEN_SOURCE to C definitions

[David Faure]

This revision was automatically updated to reflect the committed changes.
Closed by commit R240:6684cb99bdf4: Add _XOPEN_SOURCE to C definitions 
(authored by awilcox, committed by dfaure).

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8256?vs=20623=40740

REVISION DETAIL
  https://phabricator.kde.org/D8256

AFFECTED FILES
  kde-modules/KDECompilerSettings.cmake

To: awilcox, alexmerry, dfaure
Cc: kde-frameworks-devel, kde-buildsystem, mpyne, dfaure, michaelh, ngraham, 
bruns

D8256: Add _XOPEN_SOURCE to C definitions

[David Faure]

dfaure added a comment.


  Ah, found it in bugzilla :-)

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D8256

To: awilcox, alexmerry, dfaure
Cc: kde-frameworks-devel, kde-buildsystem, mpyne, dfaure, michaelh, ngraham, 
bruns

D8256: Add _XOPEN_SOURCE to C definitions

[David Faure]

dfaure added a comment.


  Let's push this then ;)
  
  You don't seem to have a developer account? I can push this but I need your 
email address for the git authorship.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D8256

To: awilcox, alexmerry, dfaure
Cc: kde-frameworks-devel, kde-buildsystem, mpyne, dfaure, michaelh, ngraham, 
bruns

D14909: Bindings: Check for libclang without version suffix also

[David Faure]

dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  find_libclang

REVISION DETAIL
  https://phabricator.kde.org/D14909

To: bruns, #frameworks, dfaure
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D8256: Add _XOPEN_SOURCE to C definitions

[David Faure]

dfaure added a comment.
Restricted Application edited subscribers, added: kde-buildsystem, 
kde-frameworks-devel; removed: Frameworks, Build System.


  (BTW where is rindex being used? lxr says only in ksysguard's FreeBSD and 
IRIX files, surely that's not the issue?)

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D8256

To: awilcox, alexmerry, dfaure
Cc: kde-frameworks-devel, kde-buildsystem, mpyne, dfaure, michaelh, ngraham, 
bruns, #frameworks, #build_system

D9446: WIP: Allow to autogenerate and install categories file

[David Faure]

dfaure added a comment.


  I don't understand what difference that would make. Any user of 
project_debug.h would get recompiled every time that file (or one of the files 
it includes) is modified (i.e. every time a category is added, modified or 
removed). So if every piece of code should include log1.h or log2.h directly 
instead, project_debug.h would be completely useless.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D9446

To: mlaurent, kfunk, lbeltrame, cgiboudeaux, dfaure, fvogt
Cc: ltoscano, kde-frameworks-devel, kde-buildsystem, habacker, michaelh, 
ngraham, bruns

D11176: Don't call PythonInterp.cmake in KF5I18NMacros.

[David Faure]

dfaure added a comment.


  Reference: 
https://build.kde.org/view/Frameworks/job/Frameworks%20ki18n%20kf5-qt5%20SUSEQt5.10/24/testReport/(root)/TestSuite/ki18n_install/
 but also reproducible locally.

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D11176

To: cgiboudeaux
Cc: sitter, dfaure, #build_system, #frameworks, michaelh, ngraham, bruns

D11176: Don't call PythonInterp.cmake in KF5I18NMacros.

[David Faure]

dfaure added a comment.


  This broke the unittest :
  
  c
  Internal cmake changing into directory: 
/d/kde/build/5/frameworks/ki18n/autotests/ki18n_install
  Error: cmake execution failed
  The C compiler identification is GNU 4.8.5
  The CXX compiler identification is GNU 5.3.1
  Check for working C compiler: /home/dfaure/bin/cc
  Configuring
  Check for working C compiler: /home/dfaure/bin/cc -- works
  Detecting C compiler ABI info
  Configuring
  Detecting C compiler ABI info - done
  Detecting C compile features
  Configuring
  Configuring
  Configuring
  Detecting C compile features - done
  Check for working CXX compiler: /home/dfaure/bin/c++
  Configuring
  Check for working CXX compiler: /home/dfaure/bin/c++ -- works
  Detecting CXX compiler ABI info
  Configuring
  Detecting CXX compiler ABI info - done
  Detecting CXX compile features
  Configuring
  Configuring
  Configuring
  Configuring
  Detecting CXX compile features - done
  CMake Error at CMakeLists.txt:4 (include):
  
include could not find load file:

  KF5I18NMacros
  
  CMake Error at CMakeLists.txt:6 (ki18n_install):
  
Unknown CMake command "ki18n_install".
  
  Configuring
  
  Thanks for having a look.

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D11176

To: cgiboudeaux
Cc: sitter, dfaure, #build_system, #frameworks, michaelh, ngraham, bruns

Re: How to prepend CMAKE_LIBRARY_OUTPUT_DIRECTORY to QCoreApplication::libraryPaths() ?

[David Faure]

On dimanche 11 mars 2018 12:08:27 CET Elvis Angelaccio wrote:
> Hi,
> I have a weird crash in ark if I enable the target generation in the bin
> subfolder [1].
> The problem is that QCoreApplication::libraryPaths() returns the following
> list:
> 
> ark.kerfuffle: ("/usr/lib/qt/plugins",
> "/home/elvis/dev/kde/ark/build/bin")
> 
> which means that ark loads first its system-wide plugins, while it should
> load first the plugins form its build dir.
> 
> ECM has code that fixes this problem for test targets [2].
> I tried to do the same to all the ark targets (main executable, kerfuffle
> library, plugins), but it doesn't work.
> Commit message in [2] by David says:
> 
> "This of course requires running the unittests via ctest rather than
> launching the executable directly."
> 
> Does that mean that ctest has some magic that adjusts the library paths?

The magic is the one that is set by that very commit [2] you're referencing:
it sets $QT_PLUGIN_PATH in the env used by ctest to run the test.

> Is it possible to do the same with normal executables?

I don't see how cmake could help, since you're launching the executable 
directly, not via cmake/ctest.

Solution 1: a change in Qt to add executablePath() to the plugin path, but 
that got rejected when I tried.
https://codereview.qt-project.org/203646

Solution 2 : a wrapper script to launch ark after setting the plugin path.
This seems to be the qmake solution. See the uic_wrapper.sh files anywhere in 
the builddir of any qmake project...

I guess we could make ECM generate such wrapper scripts for all non-test 
executables...

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

D10599: Fix include path for x86 targets

[David Faure]

dfaure added a comment.


  (btw what I did earlier this week was to introduce ANDROID_COMPILER_PREFIX in 
the first place, also for an x86 build...)

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D10599

To: vkrause, #build_system, apol
Cc: apol, dfaure, #frameworks, michaelh

D10599: Fix include path for x86 targets

[David Faure]

dfaure added a comment.


  True I've been editing the toolchain file too, probably for the same reason...

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D10599

To: vkrause, #build_system, apol
Cc: apol, dfaure, #frameworks, michaelh

D10462: Android toolchain: add ANDROID_COMPILER_PREFIX variable.

[David Faure]

dfaure closed this revision.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D10462

To: dfaure, apol, mart
Cc: #build_system, #frameworks, michaelh

D10462: Android toolchain: add ANDROID_COMPILER_PREFIX variable.

[David Faure]

dfaure created this revision.
dfaure added reviewers: apol, mart.
Restricted Application added projects: Frameworks, Build System.
Restricted Application added a subscriber: Build System.
dfaure requested review of this revision.

REVISION SUMMARY
  For ARM it's not necessary, but when building for a x86 tablet I had to
  set ANDROID_TOOLCHAIN=x86 while gcc/g++ are prefixed with
  i686-linux-android.
  
  i.e. the path is 
android-ndk-r10e/toolchains/x86-4.9/prebuilt/linux-x86_64/bin/i686-linux-android-g++

TEST PLAN
  cmake picks up the right compiler for me now

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10462

AFFECTED FILES
  toolchain/Android.cmake

To: dfaure, apol, mart
Cc: #build_system, #frameworks, michaelh

D9550: Don't set the LD_LIBRARY_PATH in prefix.sh

[David Faure]

dfaure added a comment.


  Why leave it here, commented out, without explanations?
  I'd either remove it completely, or explain the '#' ...

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9550

To: apol, #frameworks, dfaure, ngraham
Cc: ngraham, #build_system

D9299: Introduce INSTALL_PREFIX_SCRIPT to easily set up prefixes

[David Faure]

dfaure added a comment.


  Please don't set LD_LIBRARY_PATH, it prevents running stuff from the builddir 
when hacking (I just added a note about that to 
https://community.kde.org/Guidelines_and_HOWTOs/Making_apps_run_uninstalled)

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D9299

To: apol, #frameworks, sitter
Cc: dfaure, kfunk, bcooksley, ngraham, sitter, cgiboudeaux, #build_system

D9118: ki18n cmake macros: Mark UIC-generated .h files to skip AUTOMOC by default

[David Faure]

dfaure added a reviewer: kfunk.

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D9118

To: mpyne, #frameworks, #build_system, #localization, kfunk
Cc: aacid

D9116: Make sure to search for Qt5-based qmlplugindump

[David Faure]

dfaure added a comment.


  Well, it's just a hint. If it's not found there, no harm done. But yeah, at 
least the paths for most distros could be there, to be fair ;)
  
  For OpenSuSE no change needed, it's in /usr/bin.
  
  Which distro was this commit for? Looks like a debian/ubuntu path? If so, how 
did this go unnoticed until now?

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D9116

To: asturmlechner, apol
Cc: dfaure, cgiboudeaux, #frameworks, #build_system

D8256: Add _XOPEN_SOURCE to C definitions

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  I did a test compile with this locally and it seems fine indeed.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D8256

To: awilcox, alexmerry, dfaure
Cc: dfaure, #frameworks, #build_system

D9056: Add the description tag to the generated pkgconfig files

[David Faure]

dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  pkgconfig-part1

REVISION DETAIL
  https://phabricator.kde.org/D9056

To: cgiboudeaux, dfaure, apol
Cc: #frameworks, #build_system

D8979: ecm_add_test: Use proper path sep on Windows

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  Ah, oops, indeed.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D8979

To: kfunk, dfaure, cgiboudeaux, bcooksley
Cc: #windows, #frameworks, #build_system

D8660: ECMAddTests: set QT_PLUGIN_PATH so locally built plugins can be found

[David Faure]

This revision was automatically updated to reflect the committed changes.
Closed by commit R240:f7707bb546bc: ECMAddTests: set QT_PLUGIN_PATH so locally 
built plugins can be found (authored by dfaure).

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8660?vs=21894=21928

REVISION DETAIL
  https://phabricator.kde.org/D8660

AFFECTED FILES
  modules/ECMAddTests.cmake

To: dfaure, cgiboudeaux, kfunk
Cc: #frameworks, #build_system

D8660: ECMAddTests: set QT_PLUGIN_PATH so locally built plugins can be found

[David Faure]

dfaure created this revision.
dfaure added reviewers: cgiboudeaux, kfunk.
Restricted Application added projects: Frameworks, Build System.

REVISION SUMMARY
  This of course requires running the unittests via ctest rather than
  launching the executable directly.

TEST PLAN
  `ctest -V -R kurifiltertest` in kio, which prints 
QCoreApplication::libraryPaths() at the beginning of the test.
  With this commit, builddir/kio/bin is added at the front of the list, and the 
kurifilter plugins from kio's build are loaded.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D8660

AFFECTED FILES
  CMakeLists.txt

To: dfaure, cgiboudeaux, kfunk
Cc: #frameworks, #build_system

D7612: KDECMakeSettings: more docu about the layout of the build dir

[David Faure]

This revision was automatically updated to reflect the committed changes.
Closed by commit R240:d5ac6b2455d0: KDECMakeSettings: more docu about the 
layout of the build dir (authored by dfaure).

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7612?vs=18945=21892

REVISION DETAIL
  https://phabricator.kde.org/D7612

AFFECTED FILES
  kde-modules/KDECMakeSettings.cmake

To: dfaure, kossebau
Cc: #build_system, #frameworks

D7612: KDECMakeSettings: more docu about the layout of the build dir

[David Faure]

dfaure added a comment.


  No feedback, but this is just docu, I'll push it.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D7612

To: dfaure, kossebau
Cc: #build_system, #frameworks

D8660: ECMAddTests: set QT_PLUGIN_PATH so locally built plugins can be found

[David Faure]

dfaure updated this revision to Diff 21894.
dfaure added a comment.


  now with the right patch...

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8660?vs=21893=21894

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D8660

AFFECTED FILES
  modules/ECMAddTests.cmake

To: dfaure, cgiboudeaux, kfunk
Cc: #frameworks, #build_system

ECM test failure on FreeBSD

[David Faure]

On samedi 9 septembre 2017 14:15:53 CEST no-re...@kde.org wrote:
> URL   https://build.kde.org/job/Frameworks%20extra-cmake-modules%20kf5-qt5%20
> FreeBSDQt5.7/54/ Project: Frameworks extra-cmake-modules kf5-qt5
> FreeBSDQt5.7
> Name: (root) Failed: 1 test(s), Passed: 49 test(s), Skipped: 0 test(s),
> Total: 50 test(s) Failed: TestSuite.KDEInstallDirsTest.relative_or_absolute

Any idea why 
ctest -R KDEInstallDirsTest.relative_or_absolute
passes on Linux and fails on FreeBSD with this error?

"Installing in the same prefix as Qt, adopting their path scheme.
CMake Error at /usr/home/jenkins/workspace/Frameworks extra-cmake-modules 
kf5-qt5 FreeBSDQt5.7/tests/test_helpers.cmake:52 (message):
  KDE_INSTALL_PLUGINDIR (/usr/local/lib/qt5/plugins) should be a relative
  path, but is absolute.
Call Stack (most recent call first):
  CMakeLists.txt:21 (assert_var_relative_path)
"

On Linux the attached patch shows (on my system)
KDE_INSTALL_QTPLUGINDIR=lib64/plugins
when doing
$ cd /tests/KDEInstallDirsTest/relative_or_absolute
$ cmake .

Can anyone debug this on FreeBSD ?

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5
diff --git i/tests/KDEInstallDirsTest/relative_or_absolute/CMakeLists.txt w/tests/KDEInstallDirsTest/relative_or_absolute/CMakeLists.txt
index 1a7a7e7..5348f23 100644
--- i/tests/KDEInstallDirsTest/relative_or_absolute/CMakeLists.txt
+++ w/tests/KDEInstallDirsTest/relative_or_absolute/CMakeLists.txt
@@ -15,6 +15,7 @@ if(APPLE)
 endif()
 
 foreach(suffix ${var_suffixes})
+message("KDE_INSTALL_${suffix}=" ${KDE_INSTALL_${suffix}})
 if(${suffix}_should_be_absolute)
 assert_var_absolute_path(KDE_INSTALL_${suffix})
 else()

D7677: Fix python bindings compilation after 7af93dd23873d0b9cdbac192949e7e5114940aa6

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  Thanks for the fix!

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D7677

To: arojas, dfaure, skelly
Cc: #frameworks, #build_system