<div dir="ltr"><div>Hi Steve,<br><br>I'm all rebased onto KDE master. There are 4 pending reviews as follows:<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><a href="https://git.reviewboard.kde.org/r/129763/">https://git.reviewboard.kde.org/r/129763/<br></a><a href="https://git.reviewboard.kde.org/r/129765/">https://git.reviewboard.kde.org/r/129765/</a><br><br></div><div>The first three are fixes to the processing of C++ syntax. The 4th one reverts an API change which lays the ground for reintroducing the tools I need to go further. Those changes are not yet posted for review pending getting the currently queued changes in, but can be seen in clean logical chunks at <a href="https://github.com/ShaheedHaque/extra-cmake-modules/commits/KDE_restore_rules_engine">https://github.com/ShaheedHaque/extra-cmake-modules/commits/KDE_restore_rules_engine</a> if you are interested. Roughly:<br><ol><li>The first of the changes there adds back the databases to allow full control over the generated code (and are used by the last commits in the branch) and my development/regression testing tools.<br></li><li>The second adds extensive diagnostics in the form of source comments to make it easier to diagnose what changes were made by rules.</li><li>Changes the way rules databases are initialised (and, new, appended to) to make it easier to support the workflow you developed by greatly reducing the boilerplate needed, and better supporting #2. There is backwards compatibility in there for the way you are presently working too, so I can probably help migrate your code after it lands to take advantage of this.</li></ol><p>Changes #4-#7 take things to the point where KAuth, KCoreAddons, KCodecs, KCompletion and KConfigCore run cleanly through the SIP compilation process. I stole freely from your efforts to get there, but these are subsets of what you did i.e. just enough to act as regression tests upto the SIP compilation stage. The code for the tools part of #2 and #4-#7 all live in a subdirectory.<br></p></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On 3 January 2017 at 23:08, 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">FWIW, I was able produce a test case for the <span class="m_-5136412544735603686gmail-im"><span class="m_-5136412544735603686gmail-m_9193528389832539425gmail-">CursorKind.ENUM_CONSTANT_DECL change. I'll submit a patch with the test in due course.<br></span></span><div><div class="h5"><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="m_-5136412544735603686h5"><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_-5136412544735603686m_-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_-5136412544735603686m_-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_-5136412544735603686m_-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_-5136412544735603686m_-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_-5136412544735603686m_-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_-5136412544735603686m_-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_-5136412544735603686m_-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_-5136412544735603686m_-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_-5136412544735603686m_-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_-5136412544735603686m_-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_-5136412544735603686m_-8946810265629697254gmail-m_-3827625604018637927gmail-"><br></span></blockquote><div> <br></div></span><div>Absolutely, that is a given.<br></div><span class="m_-5136412544735603686m_-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_-5136412544735603686m_-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.or<wbr>g/r/129759/</a><br><a href="https://git.reviewboard.kde.org/r/129760/" target="_blank">https://git.reviewboard.kde.or<wbr>g/r/129760/</a> which depends on '759<br><br></div>Thanks<br></div></div>
</blockquote></div><br></div></div></div></div>
</blockquote></div><br></div>