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

[Christophe Fergeau]

On Wed, Mar 28, 2018 at 10:47:27AM +0200, Christophe de Dinechin wrote:
> > 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.

Ah, actually the only reason I changed its name was because I wanted
some less confusing name in the intermediate change. It's probably fine
not changing it so much ;)

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

My point was, I don't really mind if for the intermediate commit, we
don't have a corresponding plugin change. So that's only 2 plugin
commits. And the intermediate agent commit will indeed not be working
with anything, as long as the series is pushed at once, I would say it's
acceptable even if suboptimal. In this specific case, we could probably
introduce the macro early, use that first thing in the plugin, and I
think we could have a plugin commit working with both agent commits (did
not try not looked again at the source).
ABI breaks are going to be messy anyway, and because of that I'm
strongly in the camp of "let's avoid breaking ABI as much as possible"

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

Then it's a bad choice of words from me, this is not what I meant. But
from reading the commit log, we really have 3 separate things, you even
numbered them ;) Moving the version check to the agent is a functional
change, the way the ODR part is phrased make it look as a bugfix, but
it's probably mostly a consequence of moving the version check than
something we can separate. And then we've got the change of how we do
the version checking, which would be another logical change.

If my task is to "move version check to the agent", do I _have_ to change
the semantics of the version check? No. If my task is to "change the
semantics of the version check", do I have to move it to the agent? No.
I could implement both separately. This is why I see them as separate.

Of course you are not going to be able to exchange them freely, this is
why they are sent as a single patch series.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180328/904c2793/attachment.sig>