[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 12:52:24
Message-ID: CAHAc2jczUQ1rzCKZKXQf0ZYVtHp=Cidnmfa5ZPJjqb2uSAsshw () mail ! gmail ! com
[Download RAW message or body]
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"><div class="gmail_extra"><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="gmail-">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="gmail-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="gmail-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="gmail-"><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><span class="gmail-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="gmail-"><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="gmail-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="gmail-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="gmail-"><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="gmail-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="gmail-m_-3827625604018637927gmail-"><br></span></blockquote><div> \
<br></div></span><div>Absolutely, that is a given.<br></div><span \
class="gmail-"><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="gmail-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>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/">https://git.reviewboard.kde.org/r/129759/</a><br><a \
href="https://git.reviewboard.kde.org/r/129760/">https://git.reviewboard.kde.org/r/129760/</a> \
which depends on '759<br><br></div>Thanks<br></div></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic