Re: End of 2016 update on PyKF5 bindings

On 3 January 2017 at 10:18, Shaheed Haque <[email protected]> wrote:

> Hi Steve,
>
> On 2 January 2017 at 23:15, Stephen Kelly <[email protected]> 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