<p dir="ltr">Hi Stephen,</p>
<p dir="ltr">Am 11.06.2016 15:37 schrieb "Stephen Kelly" <<a href="mailto:steveire@gmail.com">steveire@gmail.com</a>>:<br>
><br>
> On 06/10/2016 11:35 PM, Tobias Hunger wrote:<br>
> ><br>
> > > Part of the design of the daemon is that messages that it sends can be<br>
> > > 'spontaneous' - it watches for filesystem changes and can tell<br>
> > clients to<br>
> > > re-read the buildsystem information.<br>
> ><br>
> > That is currently not done or needed, but can be added.<br>
> ><br>
><br>
> I think this is essential to the first version of the protocol.</p>
<p dir="ltr">Let's Start small and build from there. At this time this is not needed and it is trivial to add when it becomes necessary.</p>
<p dir="ltr">Can we get something that is useful and working before going overboard?</p>
<p dir="ltr">> The central goal of the daemon is to make it easier to implement good<br>
> support in existing tools and in new tools. In your branch you have a<br>
> reset command in the protocol. I think that should be dropped because<br>
> the daemon protocol should be designed such that it is never needed.</p>
<p dir="ltr">The Reset command is extremely handy as it allows to go back to a clean slate to e.g. switch to a different project.</p>
<p dir="ltr">Without it you have to restart the daemon. I do not want to run one CMake daemon per CMake project that is open in Creator. That can be a lot, and once you store snapshots memory usage will increase quite a bit I assume:)</p>
<p dir="ltr">> > > Additionally, it appears to be<br>
> > > redundant if you have a 'cookie'?<br>
> ><br>
> > No, cookies are to attach user data to requests and that is entirely<br>
> > orthogonal to what inReplyTo does.<br>
><br>
> Ok. Then I don't understand why clients need inReplyTo. I don't<br>
> understand why it is not redundant. Clients know what cookie they<br>
> specified in a request and they get the same cookie back, so they can<br>
> check their cookie jar and get the same information as inReplyTo, right?</p>
<p dir="ltr">A cookie is used to attach user data to a request.</p>
<p dir="ltr">Discovering how messages relate to each other has to be possible without a cookie.</p>
<p dir="ltr">> > > It is important that the daemon<br>
> > > does not create a new claim of truth in this regard - the structure<br>
> > from the<br>
> > > daemon should be the same as the structure generated for those ide<br>
> > projects.<br>
> ><br>
> > Why?<br>
> ><br>
> > I want the structure that *CMake* sees. What some unrelated program<br>
> > generates out of that structure is irrelevant.<br>
> ><br>
><br>
> What CMake sees is what it generates for IDE buildsystems.<br>
><br>
> Try taking a buildsystem of multiple directories and targets and putting<br>
> project() commands at various positions - before/after add_subdirectory<br>
> calls, in between targets, top of the file etc, and see what CMake does<br>
> there.<br>
><br>
> You might have an argument for *changing* what CMake does there, but you<br>
> would need to state such a change explicitly. (and to do that you need<br>
> to be aware of it in the first place)</p>
<p dir="ltr">I do not want to change anything here!</p>
<p dir="ltr">This mechanism works well, it is what the Codeblocks generator uses and it also happens to be very close to how CMake operates. At least the concepts map down to CMake object very naturally.</p>
<p dir="ltr">> This is part of the design work for the daemon because the behavior of<br>
> the daemon should not be accidental in any way. While protocol<br>
> versioning will make it possible to introduce new versions of the<br>
> protocol, old versions of the protocol will need to remain, and that can<br>
> affect the C++ code of CMake in bad ways, particularly if what is<br>
> exposed in the protocol is 'wrong' in some way. Just look at CMake<br>
> policies - the first ones were introduced a decade ago and there is no<br>
> plan for removing them.</p>
<p dir="ltr">I see no problem there: You can always cut out old protocols if their implementation hurts too much. That sucks for users that need the old version, but it can be cleanly detected.</p>
<p dir="ltr">This is an fact better than working with the command line client right now, which regularly introduces niggling small changes all over the place which are much harder to detect.</p>
<p dir="ltr">> I emphasise the above just to make the points that:<br>
><br>
> * Designing a protocol like this is hard (and not fast)</p>
<p dir="ltr">We have been discussing this for about two years now.</p>
<p dir="ltr">Due to the protocol versioning the implementation can be changed again later. If the old version is not too different we can do that even without breaking clients!</p>
<p dir="ltr">> * We should introduce something minimal, avoiding redundancy, because<br>
> removing things is hard</p>
<p dir="ltr">I agree. That is why the current daemon is very much limited to dumping the project structure. It is a strong foundation to go further than that.</p>
<p dir="ltr">> * The protocol should possibly expose a way to set policies so that they<br>
> can be used to deprecate behaviors in a compatible way, as is done<br>
> elsewhere in CMake</p>
<p dir="ltr">The policies applied elsewhere in CMake should already be expressed in the project structure managed by CMake. If that internal project structure changes in a significant way inside CMake, then we will need to adapt the daemon mode anyway.</p>
<p dir="ltr">I see no need to to apply policies on the daemon output at this time. For now protocol versions should suffice I think.</p>
<p dir="ltr">> * The version of the protocol should correspond to the version of CMake<br>
> it appears in (Just like how QDataStream works). Currently you're<br>
> versioning the protocol as '0.1' instead of the CMake version. Look at<br>
> the mess of QML versions, and let's not reproduce that here :)<br>
><br>
>  <a href="http://thread.gmane.org/gmane.comp.lib.qt.devel/23246">http://thread.gmane.org/gmane.comp.lib.qt.devel/23246</a></p>
<p dir="ltr">The protocol version is meant to get minor version bumps on feature additions and major version increases for incompatible changes anywhere in the protocol.</p>
<p dir="ltr">This makes it really simple to work with for clients. If I develop something for protocol version 1.3, then I can probably work with protocol 1.5 and 1.6. Protocol version 2.3 will be more risky:)</p>
<p dir="ltr">Using the CMake version as protocol version is really painful on the other hand: With each CMake release I have to test compatibility and hardcode a support matrix into Creator.</p>
<p dir="ltr">This is assuming that CMake will not change its versioning scheme to follow the protocol version -- which would be silly IMHO.</p>
<p dir="ltr">The protocol version is also entirely invisible to users. That is very different from QML version numbers:-)</p>
<p dir="ltr">> > <snip><br>
> ><br>
> > > > Is this the information you need for IDE integration?<br>
> > ><br>
> > > You might be packing too much into one protocol verb. Try this on<br>
> > LLVM and<br>
> > > VTK for example to see how much data it is. Perhaps then compare<br>
> > with the<br>
> > > gradual approach in my branch.<br>
> ><br>
> > CMake itself is way below 100KiB.<br>
> ><br>
> > A typical developer machine should be able to handle several 10MiB of<br>
> > JSON data just fine. So I do not expect a problem there.<br>
> ><br>
><br>
> Then we can easily verify that and the impact (latency difference etc)<br>
> with larger projects like the ones I mentioned.</p>
<p dir="ltr">I can provide numbers for my implementation. No problem there.</p>
<p dir="ltr">In fact I should probably add debug features to the daemon to make it easy to collect processing times and data throughput:-)</p>
<p dir="ltr">> I don't know what's better but we should be able to quantify the impact<br>
> of the choices made about what to return from particular protocol queries.<br>
><br>
> > > In my branch I have<br>
> > ><br>
> > > * `buildsystem` - Get the targets (name, type, project) and backtraces<br>
> > > * `target_info` - Get buildsystem properties for a target<br>
> > > * `file_info` - Get buildsystem properties for a file in a target<br>
> ><br>
> > I know. I did not like that, so I did it differently.<br>
> ><br>
> > Basically I do want the really important information in one go instead<br>
> > of requesting a little bit, iterate over the data, request some more, ...<br>
> ><br>
><br>
> Clients don't necessarily need all of it just to start up and show<br>
> something on start. Maybe QtCreator does, I don't know.</p>
<p dir="ltr">... which is why you can already limit what is returned in the project structure ...</p>
<p dir="ltr">I even documented that already:-)</p>
<p dir="ltr">> > In the end that is a lots of needless round trips and you send more<br>
> > data over the wire, too, with all the protocol overhead.<br>
> ><br>
><br>
> That's something that should be quantifyable.<br>
><br>
> If it makes no difference to compute and send it all, even on large<br>
> projects then maybe that's fine.<br>
><br>
> I think figuring this stuff out is part of the work of designing the<br>
> protocol.<br>
><br>
> > > Getting the backtraces (instead of the location) for targets is<br>
> > important -<br>
> ><br>
> > I am aware of the importance of backtraces, but I do not see any use<br>
> > case where this information needs to be available at the point the<br>
> > project structure is requested.<br>
> ><br>
><br>
> It should also be possible to quantify the impact of that. How much<br>
> extra data is it? How much latency is imposed if every time I click on a<br>
> target in a treeview there is a round-trip to the server before the<br>
> application even knows what should be shown in the editor next as a<br>
> result of the click? How much simpler is the client code if they already<br>
> have that information associated with the treeview node?</p>
<p dir="ltr">I would set up the tree as fast as possible and then fill in none critical information as it arrives *after the tree is displayed*. This is what Creator does anyway for nodes... add various stuff after the tree is initially displayed.</p>
<p dir="ltr">> > An IDE can for example request backtraces for a target when/if needed<br>
> > and does not need it right away to set up a project tree and code model.<br>
> ><br>
><br>
> What about a clickable project tree?</p>
<p dir="ltr">See above.</p>
<p dir="ltr">> There are several different concerns here. You proposal might be the<br>
> right answer, but my central point is that work needs to be done to<br>
> analyze the needs and be explicit about the choices and trade-offs made.<br>
><br>
> > A tool to run a static analyzer over all C++ files in a project will<br>
> > not need backtraces ever.<br>
><br>
> A tool which *only* runs a windowless static analyzer on C++ files of a<br>
> buildsystem without showing the buildsystem will probably not use the<br>
> daemon. Depending on how the static analyzer works it would be likely<br>
> that a different CMake feature would be more suitable.</p>
<p dir="ltr">Daemon-mode makes it dead easy to extract the necessary information with a script. Admittedly harder than just using the JSON dump of compile commands, but it still is a use case I have in the back of my head:)</p>
<p dir="ltr">Ideally you should be able to write a generator in Python using the information from daemon mode. I doubt the information returned right now suffices for that though:-)</p>
<p dir="ltr">Or a UI like cmake-gui... that should actually be completely doable with the code in my branch, even though I did not discuss the necessary command on the mailing list yet.</p>
<p dir="ltr">> > The biggest part of your mail is about the history of the patch set<br>
> > and how to collaborate further.<br>
> ><br>
> > I did that to make review simpler for *other* reviewers.<br>
> ><br>
><br>
> It makes sense to be able to compare your work with what came before it.<br>
> There is a large design space here and it should be understood before<br>
> merging a protocol to master.<br>
><br>
> Renaming the class and renaming member variables (m_Foo is not<br>
> conventional in CMake code) makes that harder.</p>
<p dir="ltr">Yeap, it does. Check the cmake-daemon branch, I am pretty sure the renames are a separate commit there.</p>
<p dir="ltr">I found it highly annoying that file name and class name did not match:-) I doubt that would have made it through review, too.</p>
<p dir="ltr">> > I had not expected you to chime in at all. Don't get me wrong: I<br>
> > appreciate you experience, your patience and your reviews! I just had<br>
> > not expected it. The impression I got talking to you before is that<br>
> > you do not have the resources to push this functionality forward at<br>
> > this time. In fact that is the *only* reason I stepped up for this task.<br>
> ><br>
><br>
> Yes, I will provide guidance on this topic where I can.</p>
<p dir="ltr">Great:)</p>
<p dir="ltr">> > This is why I wonder how working through your branch is supposed to<br>
> > function within the resource limits on your side?<br>
> ><br>
><br>
> I still have knowledge to share, even if I'm not putting time into CMake<br>
> development now :).</p>
<p dir="ltr">That is very much appreciated, but without you being able to commit to timely reviews of merge requests I will not use your branch as upstream.</p>
<p dir="ltr">This daemon mode has a huge potential, and having that in a branch on github somewhere does not help anybody.</p>
<p dir="ltr">> > I do not want for this work to vanish into some obscure github branch<br>
> > somewhere for weeks and month -- as it did so far. The CMake daemon is<br>
> > too cool a feature for that!<br>
> ><br>
><br>
> It didn't really vanish. It's a fairly well-known repo, blog and video.</p>
<p dir="ltr">Yes it is.</p>
<p dir="ltr">And basically nobody will play with the code till it lands in CMake itself. In my experience nobody tests experimental branches ever!</p>
<p dir="ltr">> No one worked on the topic for weeks or months. That is changing now,<br>
> but the work to be done is still the same as it was in January. It's<br>
> lots of design and analysis, and refactoring of CMake itself. The<br>
> patches extracted from your branch in the last weeks are great progress<br>
> for that, as is the wide-ranging refactoring from Daniel as he has more<br>
> experience with the core of CMake.</p>
<p dir="ltr">I agree that refactoring of cmake is crucial to land all the functionality of your cmake-daemon branch.</p>
<p dir="ltr">But let's merge the stuff that can go in as soon as possible. I am not saying that what I have now is perfect, I fully expect that to change. That is why I added the protocol version support: So that we have more freedom to change things again as feedback comes in.</p>
<p dir="ltr">> I don't think 'time to master' is an appropriate concern for a feature<br>
> which has not received enough attention yet. This stuff is complex and I<br>
> expect that doing it right would take more 'weeks and months'.</p>
<p dir="ltr">The project stuff is not overly complex.</p>
<p dir="ltr">I know what Creator needs. I have a very good idea what other IDEs need, too. Milian already agreed that what I have now is a step forward for KDevelop.</p>
<p dir="ltr">Creator already supports several build systems, so I do know what I can reasonably expect buildsystems to provide and which information is rather stable and which is not.</p>
<p dir="ltr">The code you work on *is* complex, at least looking at it with the experience I have with CMake. But that is currently not discussed at all and totally outside my focus at this time.</p>
<p dir="ltr">Please do not drag that complexity into this discussion.</p>
<p dir="ltr">Right now the questions are:</p>
<p dir="ltr">* Is the daemon infrastructure extensible and robust enough to handle more complexity as that becomes necessary.</p>
<p dir="ltr">* Is there enough benefit in the project structure to warrant adding the daemon code?</p>
<p dir="ltr">My hope is to get the changes I need in the CMake core merged first. Brad already merged some of these changes.</p>
<p dir="ltr">Once these changes are in, the daemon-mode branch is very self-contained and only touches one file that currently exists in the CMake sources. That is the file where the command line arguments are parsed and the daemon gets started.</p>
<p dir="ltr">So adding this is pretty low-risk, but of course it comes with a promise to keep the daemon working *once a CMake version that contains this code is officially released*. So we can still back out for a while longer... it will not be in the next version anyway. I did volunteer to maintain the daemon-mode code going forward.</p>
<p dir="ltr">IMHO the risk the CMake project is taking with this patch set is rather limited.</p>
<p dir="ltr">> My central point is that the design space for the protocol needs to be<br>
> explored and understood so that:<br>
><br>
> * What it exposes is correct from the CMake POV (what is 'correct' might<br>
> have to be changed *first*, as you suggest might be necessary with the<br>
> project parts).<br>
> * It can be used by QtCreator<br>
> * It can be used by other tools<br>
> * It can be extended with some of the kinds of features I showcased</p>
<p dir="ltr">Sorry, we have been discussing this for *years* now.</p>
<p dir="ltr">Kdevelop, Clion and Creator agreed that we wanted a JSON file that is unconditionally written by CMake, with pretty much what is in the proposed project structure. IIRC that was in response to a patch by kdevelop developers in 2014.</p>
<p dir="ltr">I think the requirements for a way to retrieve project structure data are pretty clear by now.</p>
<p dir="ltr">How to get much of the desired information out of cmake was not. I covered almost everything now in my patch set.</p>
<p dir="ltr">> I don't think the protocol should be merged before that kind of design<br>
> work is done, but ultimately it's indeed not up to me.</p>
<p dir="ltr">Are you referring to the daemon infrastructure code here or the command to query the project structure?</p>
<p dir="ltr">I do agree that the daemon mode infrastructure should not get merged before there is some real-world use case covered by it.</p>
<p dir="ltr">> Maybe we should talk about these issues over a beer instead :).</p>
<p dir="ltr">I am all for it! We both are located in Berlin, Germany after all. How about Tuesday night? Maybe somebody from KDevelop, kate or other IDEs can join in?</p>
<p dir="ltr">By the way: I ask for a BoF session on CMake and IDEs at QtCon (Qtcon.org, Berlin, Germany, Sept. 1-4 IIRC). Let's hope the session will make it into the conference program. That would be a great place to discuss in a wider round. I expect you will be around for QtCon?</p>
<p dir="ltr">I hope to see some parts merged by then, so that it is easy for other developers to play with the code before joining the session:-)</p>
<p dir="ltr">Best Regards,<br>
Tobias</p>