[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-frameworks-devel
Subject: Re: Review Request 129760: Fixup handling of KFontUtils::adaptFontSize's flags' default value.
From: Shaheed Haque <srhaque () theiet ! org>
Date: 2017-01-14 13:07:54
Message-ID: CAHAc2jfO9Wa_Cuwr5K45g08DjowS8iAyQNt8Dp9QqM_V01gF4A () mail ! gmail ! com
[Download RAW message or body]
Thanks for the update, I'll take a look!
On 13 January 2017 at 19:15, Stephen Kelly <steveire@gmail.com> wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129760/
>
> On January 5th, 2017, 10:56 p.m. UTC, *Stephen Kelly* wrote:
>
> 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.
> On January 5th, 2017, 11:06 p.m. UTC, *Shaheed Haque* wrote:
>
> What version of SIP compiler and C++ compiler are you using? I'm on:
>
> $ 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.
>
> In any case, unless the change breaks thing for you, I would prefer to merge the \
> complete change as it is definitely needed here.
> On January 5th, 2017, 11:11 p.m. UTC, *Stephen Kelly* wrote:
>
> I have
>
> $ 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.
>
> $ cat /etc/issue
> Ubuntu 16.04.1 LTS \n \l
>
> 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
> https://git.reviewboard.kde.org/r/129759/
>
> applied or any other patches, that could affect what you get when you run the test.
>
> On January 6th, 2017, 8:54 a.m. UTC, *Shaheed Haque* wrote:
>
> 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.
> Please see https://bugs.kde.org/show_bug.cgi?id=374801 and the commits associated \
> with it.
> 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 8aa6843404f9c6faef66cb9c76358158eafc1af1 fixed the parse \
> failure, and ed1b9ce2bb2a2e51410e0a1754a72c110010a6a0 fixed the logging bug.
> Please verify that you can build kguiaddons master with ECM master.
>
>
> - Stephen
>
> On January 3rd, 2017, 12:47 p.m. UTC, Shaheed Haque wrote:
> Review request for KDE Frameworks.
> By Shaheed Haque.
>
> *Updated Jan. 3, 2017, 12:47 p.m.*
> *Repository: * kguiaddons
> Description
>
> This depends on a fix in extra-cmake-module to actually emit the
> default value for the flags parameter.
>
> Testing
>
> Without this change, once the code the depends-on review is committed, kguiaddons \
> will fail to compile as follows:
> In file included from /home/.../PyKF5/KGuiAddons/unifiedKGuiAddons.cpp:1:0:
> /home/.../PyKF5/KGuiAddons/sipKGuiAddonsKFontUtils.cpp: In function ‘PyObject* \
> meth_KFontUtils_adaptFontSize(PyObject*, PyObject*)':
> /home/...d/PyKF5/KGuiAddons/sipKGuiAddonsKFontUtils.cpp:30:50: error: ‘NoFlags' \
> was not declared in this scope KFontUtils::AdaptFontSizeOptions a6def = NoFlags;
>
> With the change, it compiles and the tests run:
>
> $ ctest
> Test project /home/srhaque/kdebuild/kguiaddons
> 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
>
> 100% tests passed, 0 tests failed out of 6
>
> Total Test time (real) = 1.03 sec
>
> Diffs
>
> - autotests/pythontest.py (c93e75397f87ba4a84f834dd248d671614ac89dd)
> - cmake/rules_PyKF5.py (PRE-CREATION)
> - src/CMakeLists.txt (63e7598d13fa1c4d9d67e2f99edcc13e0269920e)
>
> View Diff <https://git.reviewboard.kde.org/r/129760/diff/>
>
[Attachment #3 (text/html)]
<div dir="ltr">Thanks for the update, I'll take a look!<br></div><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 class="">
<table style="border:1px #c9c399 solid;border-radius:6px" cellpadding="12" \
bgcolor="#f9f3c9" width="100%"> <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.<wbr>org/r/129760/</a> </td>
</tr>
</tbody></table>
<br>
</span><blockquote style="margin-left:1em;border-left:2px solid \
#d0d0d0;padding-left:10px"><span class=""> <p style="margin-top:0">On January 5th, \
2017, 10:56 p.m. UTC, <b>Stephen Kelly</b> wrote:</p> </span><span \
class=""><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 \
class=""> <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.<wbr>org/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 class=""><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="HOEnZb"><font \
color="#888888"> <br>
<p>- Stephen</p></font></span><div><div class="h5">
<br>
<p>On January 3rd, 2017, 12:47 p.m. UTC, Shaheed Haque wrote:</p>
<table style="border:1px #888a85 solid;border-radius:6px" cellspacing="0" \
cellpadding="12" bgcolor="#fefadf" width="100%"> <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" cellspacing="0" cellpadding="10" \
bgcolor="#ffffff" width="100%"> <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" cellspacing="0" cellpadding="10" \
bgcolor="#ffffff" width="100%"> <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/<wbr>unifiedKGuiAddons.cpp:1:0:
/home/.../PyKF5/KGuiAddons/<wbr>sipKGuiAddonsKFontUtils.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/<wbr>sipKGuiAddonsKFontUtils.cpp:<wbr>30:50: error: \
‘NoFlags' was not declared in this scope KFontUtils::<wbr>AdaptFontSizeOptions \
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/<wbr>kguiaddons 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">(<wbr>c93e75397f87ba4a84f834dd248d67<wbr>1614ac89dd)</span></li>
<li>cmake/rules_PyKF5.py <span style="color:grey">(PRE-CREATION)</span></li>
<li>src/CMakeLists.txt <span \
style="color:grey">(<wbr>63e7598d13fa1c4d9d67e2f99edcc1<wbr>3e0269920e)</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>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic