Re: End of 2016 update on PyKF5 bindings

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 <[email protected]> wrote:

> 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
>