[Spice-devel] [PATCH 0/2] Make plugin version checking more robust

[Christophe de Dinechin]

> On 27 Mar 2018, at 10:56, Christophe Fergeau <cfergeau at redhat.com> wrote:
> 
> On Tue, Mar 27, 2018 at 10:28:24AM +0200, Christophe de Dinechin wrote:
>> 
>> 
>>> On 27 Mar 2018, at 10:12, Christophe Fergeau <cfergeau at redhat.com> wrote:
>>> 
>>> With the right patch attached this time.. ;) I've only compile-tested
>>> this as this really is just a proof of concept at this point.
>> 
>> If I understand correctly, you break the ABI twice,
> 
> Ah, could be, minimizing ABI breaks was not really a goal in that split.

Avoiding unnecessary complexity should be a goal.


> What matters in my opinion is that we decide to break it, once we've
> made that decision, the number of commits which are going to make ABI
> breaking changes is less important.

Introducing “subtle” changes such as having to rename a variable in the middle makes things harder to understand, not simpler.

> 
>> and you rely on a “side effect” of changing the variable name to avoid
>> conflicts that would otherwise arise with the version number alone?
>> 
>> It makes the history fo the code harder to track, and the changes more
>> “subtle". At the very least, I would add a commit log explaining that
>> since you can’t rely on version numbers alone since the version
>> numbering scheme changes, you renamed the variable used to track
>> version numbering.
> 
> I don't understand this part :) 

You had to rename “spice_streaming_agent_plugin_version” to “spice_streaming_agent_plugin_interface_version”.
You need at least to explain why in the commit log.
In my opinion, having to rename that variable proves that the changes I had put in a single patch were not “unrelated”.

> 
>> Also, that means you need to follow the same patterns in locksteps for
>> the plugins, so you are not just making the history of the agent
>> complicated, but also the history of the plugins.
> 
> Hmm how do we cope with ABI breakage with respect to plugins is an
> interesting question. When the ABI is in flux in git, it's not going to
> be easy to link an arbitrary plugin commit to the agent commit(s) which
> have the ABI the plugin expect anyway, so I would say that we want git
> master of the agent to work with git master of the plugins. I would not
> require plugins to have at least one commit working with each commit of
> the agent.

That is not what I was talking about.
With my change, you have one ABI change in the agent, which is easy to map to one ABI change in the plugin. That leads to 4 possible combinations, 2 of which are supposed to work.
With your suggestion, you have two ABI changes in the agent, and two ABI changes in the plugin. That leads to 9 possible combinations, 3 of which are supposed to work.
So it’s more complicated to do right, and more complicated to test.


> On that note, we should strive to never break plugin ABI, only extend
> it. The plugins are in their infancy at the moment, so we can do some
> breakage now while to put everything in place, but ideally we'd settle
> on something stable "soon"
> 
>> So overall, a lot of additional complication. What is the benefit?
> 
> The benefit is that we don't have unrelated changes grouped together in
> a big commit.

Why do you call the changes unrelated?

My definition of “unrelated” or “independent” changes are changes that commute with one another. I can apply one or the other, in any order, and it does not matter. (Funny thing that it’s the same definition as in quantum mechanics). When I said I did not know how to split my patch, it’s precisely because I don’t know how to decorrelate the two “halves” of the patch, and apparently you don’t know either since you had to rename the version check variable for correctness (if you don’t rename it, then it’s bogus).

There can be other reasons to split a patch, e.g. to clarify the evolution of the code, but they don’t seem to apply here either.


Christophe
> 
> Christophe
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel