End of 2016 update on PyKF5 bindings

[Shaheed Haque]

Hi Steve,

On 2 January 2017 at 23:15, Stephen Kelly <steveire at 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-bindings/attachments/20170103/1d3baca8/attachment-0001.html>