[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-bindings
Subject: Re: End of 2016 update on PyKF5 bindings
From: Shaheed Haque <srhaque () theiet ! org>
Date: 2017-01-03 23:08:53
Message-ID: CAHAc2jdDoNKGA1OqYPAuCH3hz1OmDaUEDX1dOdxutXLyjW5ZEA () mail ! gmail ! com
[Download RAW message or body]
FWIW, I was able produce a test case for the CursorKind.ENUM_CONSTANT_DECL
change. I'll submit a patch with the test in due course.
On 3 January 2017 at 12:52, Shaheed Haque <srhaque@theiet.org> wrote:
> On 3 January 2017 at 10:18, Shaheed Haque <srhaque@theiet.org> wrote:
>
>> Hi Steve,
>>
>> On 2 January 2017 at 23:15, Stephen Kelly <steveire@gmail.com> wrote:
>>
>>> Shaheed Haque wrote:
>>>
>>> >> Can you extract self-contained commits and propose them for that repo?
>>> >
>>> > What is the review procedure for extra-cmake-modules?
>>>
>>> I think there is a reviewboard or something for it.
>>>
>>> Posting a link to commits anywhere (eg a branch on github) works for me
>>> anyway too as I find reviewboard unusable for multi-commit branches.
>>>
>>
>> I was mostly wanting to check whether reviewboard or phabricator was in
>> use, but github is even better. Thanks!
>>
>>
>>> > There is also one
>>> > change which needs to be made in a synchronised fashion between
>>> > extra-cmake-modules and kguiaddons, any suggestions on how to handle
>>> that
>>> > are welcome.
>>>
>>> Sure, if you post it, we can think about how to do it. I only pushed the
>>> kguiaddons bindings today. I am curious because the patch was very small
>>> and
>>> I wonder what could possibly need to be done in concert.
>>>
>>
>> I misspoke, it wasn't in kguiadddons, but basically the ModuleCodeDb dict
>> was documented as being keyed by the filename, but I implemented it
>> wrongly, and you used a key as per the implementation, not the docs :-). We
>> can deal with the conflict, by temporarily duplicating the dict entry,
>> ahead of the core logic changing, and then removing the original, but it
>> all just needs to be done in order, perhaps with a couple of days between
>> each change to avoid tripping folk up.
>>
>>
>>> >> You seem to have changed some things in your clone such as using
>>> >>
>>> >> enum.kind == CursorKind.ENUM_CONSTANT_DECL
>>> >>
>>> >> Please extend the unit test in tests/GenerateSipBindings for that and
>>> >> other similar items.
>>> >>
>>> >
>>> > I'm always looking for opportunities to add them but in most cases, the
>>> > fixes modify the text output for a given source in another framework
>>> > component. It is not obvious how to create a simple self-contained test
>>> > case.
>>>
>>> Well, what does that change do? You have a comment saying "Skip
>>> visibility
>>> attributes and the like.". What does that mean? You encountered an issue
>>> when parsing an enum with a visibility attribute? Then open
>>>
>>> tests/GenerateSipBindings/cpplib.h
>>>
>>> put an enum with an attribute somewhere, run the tests, watch them fail,
>>> make your fix, run the tests and watch them pass.
>>>
>>> Isn't this extremely obvious? What am I missing? Do I misunderstand your
>>> difficulty?
>>>
>>
>>> > For example, in the case you noticed, the point of the change only
>>> > becomes apparent when libclang decides to work in a particular
>>> way...and I
>>> > have no idea how in general to provoke those cases.
>>>
>>> Start with whatever provokes it with whatever header is being processed,
>>> and
>>> then simplify. Again, I must be missing something because this is really
>>> obvious, and I'm certain you know how to do this. I must be missing
>>> something.
>>>
>>
>> Please let me set some context.
>>
>> Remember my present focus is on making sure the tools run, and produce
>> output that the SIP compiler does not barf on across huge portions of the
>> KDE codebase. Specifically, as I have hinted before, I need to be
>> reasonably sure that I can handle the breadth without changes to the rule
>> format as my primary goal. I perceive that a failure to get that right will
>> result in a repeat of the failure to smoothly migrate PyKDE4 to KDE5 (which
>> is why we are even here!).
>>
>> Thus, my regression test environment generates 2280 non-duplicate .SIP
>> files across 133 components. Of this set, and building on the work you did
>> on github, I can currently run 5 of those components cleanly through the
>> SIP compiler. So, I'm not even concerned at this point in whether the
>> resulting bindings work (of course, I use/verify the unit tests you already
>> wrote for the various components on github, though I've not yet checked the
>> status of those on KDE master).
>>
>> My *main* regression tests therefore include carefully diff'ing the 2280
>> files from run to run, gradually picking off errors as I can to get to the
>> point of a clean SIP compiler run across the 133 components in turn. (BTW,
>> that is also the reason for the continued existence of
>> sip_buik_generator.py and sip_compiler.py as necessary tooling to support
>> this development and testing). It was this test process that found this bug
>> when an assert was triggered.
>>
>> Now, if I could easily see what in the source code of the enum causes
>> libclang to emit an attribute, I would have considered writing the test,
>> but based on the ROI of the effort involved in doing what you suggest
>> (which is, I agree, theoretically the right thing to do) just did not meet
>> the bar of a simple-to-develop test, given the scope of what else I need to
>> get done. That is, not firing on the assert seemed, and frankly seems, good
>> enough to me in this case.
>>
>> > My focus beyond the PRs I posted has been to make the system easier to
>>> use
>>> > and diagnose, especially for your workflow.
>>>
>>> Great, I think I saw some of that stuff with follow-up 'oops - fix
>>> something' commits. If you can present that work in commits which are
>>> already 'fixed' I'd love to see them.
>>>
>>
>> Absolutely, that is a given.
>>
>> >> > If we can get the PRs merged, and work out item 4 above,
>>> >> > what else might be needed to getting this into production?
>>> >>
>>> >> There are still packaging related issues, but I don't know how to
>>> solve
>>> >> them
>>> >> and I would look to others to provide guidance there.
>>> >>
>>> >
>>> > Likewise, assuming the rebase does not fix it, I'm sure I'll need help
>>> > with item 4.
>>>
>>> I'm sure I can help with that soon enough.
>>>
>>
>> Excellent. Having had a quick look at the code in the KDE repos, I see
>> that there are quite some changes, notably to rules_engine.py which removes
>> some of the test facilities I need to revive the sip_builk_generator.py
>> test workflow. I'll see if I can package those changes with collateral that
>> will ease any concerns with the testing that is being done.
>>
>> Thanks, Shaheed
>>
>>
>>> Thanks,
>>>
>>> Steve.
>>>
>>
>>
> OK, step 1. After a few "how does reviewboard work again?" moments, I've
> posted:
>
> https://git.reviewboard.kde.org/r/129759/
> https://git.reviewboard.kde.org/r/129760/ which depends on '759
>
> Thanks
>
[Attachment #3 (text/html)]
<div dir="ltr">FWIW, I was able produce a test case for the <span \
class="gmail-im"><span \
class="gmail-m_9193528389832539425gmail-">CursorKind.ENUM_CONSTANT_DECL change. \
I'll submit a patch with the test in due course.<br></span></span><br><div \
class="gmail_extra"><div class="gmail_quote">On 3 January 2017 at 12:52, 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"><div class="gmail_extra"><div><div \
class="h5"><div class="gmail_quote">On 3 January 2017 at 10:18, 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:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi Steve,<br><div><div \
class="gmail_extra"><br><div class="gmail_quote"><span \
class="m_-8946810265629697254gmail-">On 2 January 2017 at 23: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:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><span \
class="m_-8946810265629697254gmail-m_-3827625604018637927gmail-">Shaheed Haque \
wrote:<br> <br>
>> Can you extract self-contained commits and propose them for that repo?<br>
><br>
> What is the review procedure for extra-cmake-modules?<br>
<br>
</span>I think there is a reviewboard or something for it.<br>
<br>
Posting a link to commits anywhere (eg a branch on github) works for me<br>
anyway too as I find reviewboard unusable for multi-commit branches.<span \
class="m_-8946810265629697254gmail-m_-3827625604018637927gmail-"><br></span></blockquote><div><br></div></span><div>I \
was mostly wanting to check whether reviewboard or phabricator was in use, but github \
is even better. Thanks!<br> <br></div><span \
class="m_-8946810265629697254gmail-"><blockquote class="gmail_quote" \
style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><span \
class="m_-8946810265629697254gmail-m_-3827625604018637927gmail-"> > There is also \
one<br> > change which needs to be made in a synchronised fashion between<br>
> extra-cmake-modules and kguiaddons, any suggestions on how to handle that<br>
> are welcome.<br>
<br>
</span>Sure, if you post it, we can think about how to do it. I only pushed the<br>
kguiaddons bindings today. I am curious because the patch was very small and<br>
I wonder what could possibly need to be done in \
concert.<br></blockquote><div><br></div></span><div>I misspoke, it wasn't in \
kguiadddons, but basically the ModuleCodeDb dict was documented as being keyed by the \
filename, but I implemented it wrongly, and you used a key as per the implementation, \
not the docs :-). We can deal with the conflict, by temporarily duplicating the dict \
entry, ahead of the core logic changing, and then removing the original, but it all \
just needs to be done in order, perhaps with a couple of days between each change to \
avoid tripping folk up.<br></div><span class="m_-8946810265629697254gmail-"><div> \
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px \
solid rgb(204,204,204);padding-left:1ex"> <span \
class="m_-8946810265629697254gmail-m_-3827625604018637927gmail-"> >> You seem \
to have changed some things in your clone such as using<br> >><br>
>> enum.kind == CursorKind.ENUM_CONSTANT_DECL<br>
>><br>
>> Please extend the unit test in tests/GenerateSipBindings for that and<br>
>> other similar items.<br>
>><br>
><br>
> I'm always looking for opportunities to add them but in most cases, the<br>
> fixes modify the text output for a given source in another framework<br>
> component. It is not obvious how to create a simple self-contained test<br>
> case.<br>
<br>
</span>Well, what does that change do? You have a comment saying "Skip \
visibility<br> attributes and the like.". What does that mean? You encountered \
an issue<br> when parsing an enum with a visibility attribute? Then open<br>
<br>
tests/GenerateSipBindings/cpp<wbr>lib.h<br>
<br>
put an enum with an attribute somewhere, run the tests, watch them fail,<br>
make your fix, run the tests and watch them pass.<br>
<br>
Isn't this extremely obvious? What am I missing? Do I misunderstand your<br>
difficulty?<br></blockquote><div></div><blockquote class="gmail_quote" \
style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><span \
class="m_-8946810265629697254gmail-m_-3827625604018637927gmail-"><br> > For \
example, in the case you noticed, the point of the change only<br> > becomes \
apparent when libclang decides to work in a particular way...and I<br> > have no \
idea how in general to provoke those cases.<br> <br>
</span>Start with whatever provokes it with whatever header is being processed, \
and<br> then simplify. Again, I must be missing something because this is really<br>
obvious, and I'm certain you know how to do this. I must be missing<br>
something.<br></blockquote><div><br></div></span><div>Please let me set some \
context.<br></div><div><br></div><div>Remember my present focus is on making sure the \
tools run, and produce output that the SIP compiler does not barf on across huge \
portions of the KDE codebase. Specifically, as I have hinted before, I need to be \
reasonably sure that I can handle the breadth without changes to the rule format as \
my primary goal. I perceive that a failure to get that right will result in a repeat \
of the failure to smoothly migrate PyKDE4 to KDE5 (which is why we are even \
here!).<br><br>Thus, my regression test environment generates 2280 non-duplicate .SIP \
files across 133 components. Of this set, and building on the work you did on github, \
I can currently run 5 of those components cleanly through the SIP compiler. So, \
I'm not even concerned at this point in whether the resulting bindings work (of \
course, I use/verify the unit tests you already wrote for the various components on \
github, though I've not yet checked the status of those on KDE \
master).<br><br></div><div>My *main* regression tests therefore include carefully \
diff'ing the 2280 files from run to run, gradually picking off errors as I can to \
get to the point of a clean SIP compiler run across the 133 components in turn. (BTW, \
that is also the reason for the continued existence of sip_buik_generator.py and \
sip_compiler.py as necessary tooling to support this development and testing). It was \
this test process that found this bug when an assert was triggered.<br><br>Now, if I \
could easily see what in the source code of the enum causes libclang to emit an \
attribute, I would have considered writing the test, but based on the ROI of the \
effort involved in doing what you suggest (which is, I agree, theoretically the right \
thing to do) just did not meet the bar of a simple-to-develop test, given the scope \
of what else I need to get done. That is, not firing on the assert seemed, and \
frankly seems, good enough to me in this case.<br></div><span \
class="m_-8946810265629697254gmail-"><div><br></div><blockquote class="gmail_quote" \
style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"> <span \
class="m_-8946810265629697254gmail-m_-3827625604018637927gmail-"> > My focus \
beyond the PRs I posted has been to make the system easier to use<br> > and \
diagnose, especially for your workflow.<br> <br>
</span>Great, I think I saw some of that stuff with follow-up 'oops - fix<br>
something' commits. If you can present that work in commits which are<br>
already 'fixed' I'd love to see them.<span \
class="m_-8946810265629697254gmail-m_-3827625604018637927gmail-"><br></span></blockquote><div> \
<br></div></span><div>Absolutely, that is a given.<br></div><span \
class="m_-8946810265629697254gmail-"><div><br></div><blockquote class="gmail_quote" \
style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><span \
class="m_-8946810265629697254gmail-m_-3827625604018637927gmail-"> >> > If we \
can get the PRs merged, and work out item 4 above,<br> >> > what else might \
be needed to getting this into production?<br> >><br>
>> There are still packaging related issues, but I don't know how to \
solve<br> >> them<br>
>> and I would look to others to provide guidance there.<br>
>><br>
><br>
> Likewise, assuming the rebase does not fix it, I'm sure I'll need \
help<br> > with item 4.<br>
<br>
</span>I'm sure I can help with that soon \
enough.<br></blockquote></span><div><br>Excellent. Having had a quick look at the \
code in the KDE repos, I see that there are quite some changes, notably to \
rules_engine.py which removes some of the test facilities I need to revive the \
sip_builk_generator.py test workflow. I'll see if I can package those changes \
with collateral that will ease any concerns with the testing that is being \
done.<br><br></div><div>Thanks, Shaheed<br></div><div> </div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex">
Thanks,<br>
<br>
Steve.<br>
</blockquote></div><br></div></div></div>
</blockquote></div><br></div></div><div>OK, step 1. After a few "how does \
reviewboard work again?" moments, I've posted:<br><br><a \
href="https://git.reviewboard.kde.org/r/129759/" \
target="_blank">https://git.reviewboard.kde.<wbr>org/r/129759/</a><br><a \
href="https://git.reviewboard.kde.org/r/129760/" \
target="_blank">https://git.reviewboard.kde.<wbr>org/r/129760/</a> which depends on \
'759<br><br></div>Thanks<br></div></div> </blockquote></div><br></div></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic