<div dir="ltr"><div><div>Hi Steve,<br><br>I have verified this works on KDE/master, and will close the review.<br><br></div>I did see a "funny" in that I had to run CMake twice in a fresh directory before it would run clean. You might want to look into that as it was not clear to me what it CMake is trying to do. Anyway, here is the first unsuccessful run:<br><br>==========<br>$ rm -r *<br>$ cmake ~/kdedev/frameworks-bindings/kguiaddons/;<br>-- The C compiler identification is GNU 6.2.0<br>-- The CXX compiler identification is GNU 6.2.0<br>-- Check for working C compiler: /usr/bin/cc<br>-- Check for working C compiler: /usr/bin/cc -- works<br>-- Detecting C compiler ABI info<br>-- Detecting C compiler ABI info - done<br>-- Detecting C compile features<br>-- Detecting C compile features - done<br>-- Check for working CXX compiler: /usr/bin/c++<br>-- Check for working CXX compiler: /usr/bin/c++ -- works<br>-- Detecting CXX compiler ABI info<br>-- Detecting CXX compiler ABI info - done<br>-- Detecting CXX compile features<br>-- Detecting CXX compile features - done<br>-- <br><br>-- Looking for __GLIBC__<br>-- Looking for __GLIBC__ - found<br>-- Performing Test _OFFT_IS_64BIT<br>-- Performing Test _OFFT_IS_64BIT - Success<br>-- Looking for XOpenDisplay in /usr/lib/x86_64-linux-gnu/libX11.so;/usr/lib/x86_64-linux-gnu/libXext.so<br>-- Looking for XOpenDisplay in /usr/lib/x86_64-linux-gnu/libX11.so;/usr/lib/x86_64-linux-gnu/libXext.so - found<br>-- Looking for gethostbyname<br>-- Looking for gethostbyname - found<br>-- Looking for connect<br>-- Looking for connect - found<br>-- Looking for remove<br>-- Looking for remove - found<br>-- Looking for shmat<br>-- Looking for shmat - found<br>-- Found X11: /usr/lib/x86_64-linux-gnu/libX11.so<br>-- Found PkgConfig: /usr/bin/pkg-config (found version "0.29.1") <br>-- Found XCB_XCB: /usr/lib/x86_64-linux-gnu/libxcb.so (found version "1.11.1") <br>-- Found XCB: /usr/lib/x86_64-linux-gnu/libxcb.so (found version "1.11.1") found components: XCB <br>-- Performing Test COMPILER_HAS_HIDDEN_VISIBILITY<br>-- Performing Test COMPILER_HAS_HIDDEN_VISIBILITY - Success<br>-- Performing Test COMPILER_HAS_HIDDEN_INLINE_VISIBILITY<br>-- Performing Test COMPILER_HAS_HIDDEN_INLINE_VISIBILITY - Success<br>-- Performing Test COMPILER_HAS_DEPRECATED_ATTR<br>-- Performing Test COMPILER_HAS_DEPRECATED_ATTR - Success<br>CMake Error at /usr/local/share/ECM/find-modules/FindPythonModuleGeneration.cmake:256 (message):<br> Failed to find clang driver corresponding to<br> /usr/lib/x86_64-linux-gnu/<a href="http://libclang-3.9.so">libclang-3.9.so</a><br>Call Stack (most recent call first):<br> /usr/local/share/ECM/find-modules/FindPythonModuleGeneration.cmake:301 (_compute_implicit_include_dirs)<br> src/CMakeLists.txt:94 (ecm_generate_python_binding)<br><br><br>-- Configuring incomplete, errors occurred!<br>See also "/home/srhaque/kdebuild/kguiaddons/CMakeFiles/CMakeOutput.log".<br>================<br><br></div>and then the second successful run:<br><br>================<br><div>$ cmake ~/kdedev/frameworks-bindings/kguiaddons/;<br>-- <br><br>-- <br>-- The following OPTIONAL packages have been found:<br><br> * X11<br> * PkgConfig<br> * XCB , X protocol C-language Binding , <<a href="http://xcb.freedesktop.org">http://xcb.freedesktop.org</a>><br> * Qt5X11Extras (required version >= 5.5.0)<br> * PythonModuleGeneration<br><br>-- The following REQUIRED packages have been found:<br><br> * ECM (required version >= 5.30.0) , Extra CMake Modules. , <<a href="https://projects.kde.org/projects/kdesupport/extra-cmake-modules">https://projects.kde.org/projects/kdesupport/extra-cmake-modules</a>><br> * Qt5Gui (required version >= 5.5.0)<br> * Qt5Test<br> * Qt5Widgets<br> * Qt5 (required version >= 5.5.0)<br><br>-- Configuring done<br>-- Generating done<br>-- Build files have been written to: /home/srhaque/kdebuild/kguiaddons<br><br>================<br></div><div><br>After this, "make; make test" ran with no issues, generating the .sip files and so on.<br><br></div><div>Thanks, Shaheed<br></div><div><br><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On 14 January 2017 at 13:07, Shaheed Haque <span dir="ltr"><<a href="mailto:srhaque@theiet.org" target="_blank">srhaque@theiet.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Thanks for the update, I'll take a look!<br></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On 13 January 2017 at 19:15, Stephen Kelly <span dir="ltr"><<a href="mailto:steveire@gmail.com" target="_blank">steveire@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>
<div style="font-family:Verdana,Arial,Helvetica,Sans-Serif"><span>
<table style="border:1px #c9c399 solid;border-radius:6px" width="100%" cellpadding="12" bgcolor="#f9f3c9">
<tbody><tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/129760/" target="_blank">https://git.reviewboard.kde.or<wbr>g/r/129760/</a>
</td>
</tr>
</tbody></table>
<br>
</span><blockquote style="margin-left:1em;border-left:2px solid #d0d0d0;padding-left:10px"><span>
<p style="margin-top:0">On January 5th, 2017, 10:56 p.m. UTC, <b>Stephen Kelly</b> wrote:</p>
</span><span><blockquote style="margin-left:1em;border-left:2px solid #d0d0d0;padding-left:10px">
<pre style="white-space:pre-wrap;white-space:-moz-pre-wrap;white-space:-pre-wrap;white-space:-o-pre-wrap;word-wrap:break-word"><p style="padding:0;margin:0;line-height:inherit;white-space:inherit">I've added the change to the unit test. It already passes, so it's not clear to me what else is needed from this review request.</p></pre>
</blockquote>
</span><p>On January 5th, 2017, 11:06 p.m. UTC, <b>Shaheed Haque</b> wrote:</p><span>
<blockquote style="margin-left:1em;border-left:2px solid #d0d0d0;padding-left:10px">
<pre style="white-space:pre-wrap;white-space:-moz-pre-wrap;white-space:-pre-wrap;white-space:-o-pre-wrap;word-wrap:break-word"><p style="padding:0;margin:0;line-height:inherit;white-space:inherit">What version of SIP compiler and C++ compiler are you using? I'm on:</p>
<p style="padding:0;margin:0;line-height:inherit;white-space:inherit">$ sip -V
4.18.1
$ g++ --version
g++ (Ubuntu 6.2.0-5ubuntu12) 6.2.0 20161005
Copyright (C) 2016 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.</p>
<p style="padding:0;margin:0;line-height:inherit;white-space:inherit">In any case, unless the change breaks thing for you, I would prefer to merge the complete change as it is definitely needed here.</p></pre>
</blockquote>
<p>On January 5th, 2017, 11:11 p.m. UTC, <b>Stephen Kelly</b> wrote:</p>
<blockquote style="margin-left:1em;border-left:2px solid #d0d0d0;padding-left:10px">
<pre style="white-space:pre-wrap;white-space:-moz-pre-wrap;white-space:-pre-wrap;white-space:-o-pre-wrap;word-wrap:break-word"><p style="padding:0;margin:0;line-height:inherit;white-space:inherit">I have</p>
<p style="padding:0;margin:0;line-height:inherit;white-space:inherit">$ sip -V
4.17
$ g++ --version
g++ (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.</p>
<p style="padding:0;margin:0;line-height:inherit;white-space:inherit">$ cat /etc/issue
Ubuntu 16.04.1 LTS \n \l</p>
<p style="padding:0;margin:0;line-height:inherit;white-space:inherit">Can you ensure that when you test this that you are using the clean master branch of ECM and the master branch of KGuiAddons? If you have </p>
<p style="padding:0;margin:0;line-height:inherit;white-space:inherit"><a href="https://git.reviewboard.kde.org/r/129759/" target="_blank">https://git.reviewboard.kde.or<wbr>g/r/129759/</a></p>
<p style="padding:0;margin:0;line-height:inherit;white-space:inherit">applied or any other patches, that could affect what you get when you run the test.</p></pre>
</blockquote>
</span><span><p>On January 6th, 2017, 8:54 a.m. UTC, <b>Shaheed Haque</b> wrote:</p>
<blockquote style="margin-left:1em;border-left:2px solid #d0d0d0;padding-left:10px">
<pre style="white-space:pre-wrap;white-space:-moz-pre-wrap;white-space:-pre-wrap;white-space:-o-pre-wrap;word-wrap:break-word"><p style="padding:0;margin:0;line-height:inherit;white-space:inherit">As per my reviewboard markings, this change depends on '759. The point is that without '759, the SIP compiler bails out before this issue can be seen. So the real question is why/how you were able to manage without '759...I would guess that the SIP compiler version delta explains this.</p></pre>
</blockquote>
</span></blockquote>
<pre style="white-space:pre-wrap;white-space:-moz-pre-wrap;white-space:-pre-wrap;white-space:-o-pre-wrap;word-wrap:break-word"><p style="padding:0;margin:0;line-height:inherit;white-space:inherit">Please see <a href="https://bugs.kde.org/show_bug.cgi?id=374801" target="_blank">https://bugs.kde.org/show_bug.<wbr>cgi?id=374801</a> and the commits associated with it. </p>
<p style="padding:0;margin:0;line-height:inherit;white-space:inherit">In particular: your compiles were failing, but bindings were being created anyway, which mostly worked for you with the exception of this enum issue. This enum issue failed because it relies on being able to parse the QFlags properly. Your compilations were failing because you are using Qt 5.7 or later which requires -std=c++11 or later. That was not being added to the flags when parsing the headers with libclang. ECM commit 8aa6843404f9c6faef66cb9c763581<wbr>58eafc1af1 fixed the parse failure, and ed1b9ce2bb2a2e51410e0a1754a72c<wbr>110010a6a0 fixed the logging bug.</p>
<p style="padding:0;margin:0;line-height:inherit;white-space:inherit">Please verify that you can build kguiaddons master with ECM master.</p></pre><span class="m_868752649434179552HOEnZb"><font color="#888888">
<br>
<p>- Stephen</p></font></span><div><div class="m_868752649434179552h5">
<br>
<p>On January 3rd, 2017, 12:47 p.m. UTC, Shaheed Haque wrote:</p>
<table style="border:1px #888a85 solid;border-radius:6px" width="100%" cellspacing="0" cellpadding="12" bgcolor="#fefadf">
<tbody><tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By Shaheed Haque.</div>
<p style="color:grey"><i>Updated Jan. 3, 2017, 12:47 p.m.</i></p>
<div style="margin-top:1.5em">
<b style="color:#575012;font-size:10pt">Repository: </b>
kguiaddons
</div>
<h1 style="color:#575012;font-size:10pt;margin-top:1.5em">Description </h1>
<table style="border:1px solid #b8b5a0" width="100%" cellspacing="0" cellpadding="10" bgcolor="#ffffff">
<tbody><tr>
<td>
<pre style="margin:0;padding:0;white-space:pre-wrap;white-space:-moz-pre-wrap;white-space:-pre-wrap;white-space:-o-pre-wrap;word-wrap:break-word">This depends on a fix in extra-cmake-module to actually emit the
default value for the flags parameter.</pre>
</td>
</tr>
</tbody></table>
<h1 style="color:#575012;font-size:10pt;margin-top:1.5em">Testing </h1>
<table style="border:1px solid #b8b5a0" width="100%" cellspacing="0" cellpadding="10" bgcolor="#ffffff">
<tbody><tr>
<td>
<pre style="margin:0;padding:0;white-space:pre-wrap;white-space:-moz-pre-wrap;white-space:-pre-wrap;white-space:-o-pre-wrap;word-wrap:break-word"><p style="padding:0;margin:0;line-height:inherit;white-space:inherit">Without this change, once the code the depends-on review is committed, kguiaddons will fail to compile as follows:</p>
<p style="padding:0;margin:0;line-height:inherit;white-space:inherit">In file included from /home/.../PyKF5/KGuiAddons/uni<wbr>fiedKGuiAddons.cpp:1:0:
/home/.../PyKF5/KGuiAddons/sip<wbr>KGuiAddonsKFontUtils.cpp: In function ‘PyObject<em style="padding:0;margin:0;line-height:inherit;white-space:normal"> meth_KFontUtils_adaptFontSize(<wbr>PyObject</em>, PyObject*)’:
/home/...d/PyKF5/KGuiAddons/si<wbr>pKGuiAddonsKFontUtils.cpp:30:<wbr>50: error: ‘NoFlags’ was not declared in this scope
KFontUtils::AdaptFontSizeOptio<wbr>ns a6def = NoFlags;</p>
<p style="padding:0;margin:0;line-height:inherit;white-space:inherit">With the change, it compiles and the tests run:</p>
<p style="padding:0;margin:0;line-height:inherit;white-space:inherit">$ ctest
Test project /home/srhaque/kdebuild/kguiadd<wbr>ons
Start 1: appstreamtest
1/6 Test #1: appstreamtest .................... Passed 0.02 sec
Start 2: Py3Test
2/6 Test #2: Py3Test .......................... Passed 0.13 sec
Start 3: Py2Test
3/6 Test #3: Py2Test .......................... Passed 0.11 sec
Start 4: kwordwraptest
4/6 Test #4: kwordwraptest .................... Passed 0.16 sec
Start 5: kcolorutilstest
5/6 Test #5: kcolorutilstest .................. Passed 0.53 sec
Start 6: kiconutilstest
6/6 Test #6: kiconutilstest ................... Passed 0.08 sec</p>
<p style="padding:0;margin:0;line-height:inherit;white-space:inherit">100% tests passed, 0 tests failed out of 6</p>
<p style="padding:0;margin:0;line-height:inherit;white-space:inherit">Total Test time (real) = 1.03 sec</p></pre>
</td>
</tr>
</tbody></table>
<h1 style="color:#575012;font-size:10pt;margin-top:1.5em">Diffs </h1>
<ul style="margin-left:3em;padding-left:0">
<li>autotests/pythontest.py <span style="color:grey">(c93e75397f87ba4a84f834dd248d6<wbr>71614ac89dd)</span></li>
<li>cmake/rules_PyKF5.py <span style="color:grey">(PRE-CREATION)</span></li>
<li>src/CMakeLists.txt <span style="color:grey">(63e7598d13fa1c4d9d67e2f99edcc<wbr>13e0269920e)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/129760/diff/" style="margin-left:3em" target="_blank">View Diff</a></p>
</td>
</tr>
</tbody></table>
</div></div></div>
</div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>