[Spice-devel] [PATCH spice-streaming-agent v2 1/2] Don't tag the plugin interface as 1.0 just yet

Frediano Ziglio fziglio at redhat.com
Fri Nov 17 13:56:08 UTC 2017


> 
> > On 16 Nov 2017, at 11:43, Frediano Ziglio <fziglio at redhat.com> wrote:
> > 
> >> 
> >> From: Christophe de Dinechin <dinechin at redhat.com>
> >> 
> >> This patch series introduces changes in the ABI and API, and I expect
> >> a few more to arrive shortly. So I tagged the ABI version as 0.01,
> >> and all 0.x versions are considered as incompatible with one another
> >> by default. When we reach ABI and API stability, we can bump to a
> >> non-zero version number and then we will need to preserve ABI and API
> >> compatibility within a major version moving forward.
> >> 
> >> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> > 
> > Still nack, I already explained why.
> 
> The first “explanation" was this:
> 
> > Nack... ABI not compatible
> 
> I addressed that by adding a comment explaining why (in addition to my mail
> replies).
> 
> Then you added:
> 
> > Going backward? Would not be easier to get to 1.1 instead?
> 
> But by your compatibility rules, 1.1 should be forward compatible with 1.0.
> Which
> is not the case here.
> 

Did you try to make it compatible? Looks like you are assuming by axiom
is not possible.

> Then you wrote
> > I would try to extend maintaining API/ABI if possible. As far as I can see
> > your mainly concerns are lack of some features.
> > 
> 
> I replied to this.
> 
> You then replied to Victor but not to me, so I took that as meaning that my
> last
> explanation as to why I wanted to clean up the ABI right now had convinced
> you.
> 

I clicked Reply, edit the message and clicked Send.

> 
> So I’m confused. Are you:
> 
> - Convinced we can only move the ABI number forward, i.e. the next one has to
> be 2.0. If so, why?
> 

Going forward is a basic rule. Many people agree with the "Semantic Versioning"
document. Why that is wrong? Going against the rule usually carry the issue
having to solve new problems.
I would honestly try to have a 1.1, if fails a 2.0.
I would use that an exercise and test if the initial design was at least
extensible and what are the issue extending it or the next small change
will be 3.0 or 4.0.

> - Convinced that what I want to do should be done in an ABI or API comptaible
> way. If so, how?
> 

Not convinced, but you are not trying. For instance method should be added
at the end. And should not be removed. The only big problem I found is that
the Agent cannot check the Plugin version. But I actually have a workaround
for this (not that is really nice to see...).

> 
> Thanks
> Christophe
> 
> 
> > 
> >> ---
> >> include/spice-streaming-agent/plugin.hpp | 2 +-
> >> src/concrete-agent.cpp                   | 3 +++
> >> 2 files changed, 4 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/include/spice-streaming-agent/plugin.hpp
> >> b/include/spice-streaming-agent/plugin.hpp
> >> index 727cb3b..607fabf 100644
> >> --- a/include/spice-streaming-agent/plugin.hpp
> >> +++ b/include/spice-streaming-agent/plugin.hpp
> >> @@ -26,7 +26,7 @@ class FrameCapture;
> >>  * where MM is major and mm is the minor, can be easily expanded
> >>  * using more bits in the future
> >>  */
> >> -enum Constants : unsigned { PluginVersion = 0x100u };
> >> +enum Constants : unsigned { PluginVersion = 0x001u };
> >> 
> >> enum Ranks : unsigned {
> >>     /// this plugin should not be used
> >> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> >> index 192054a..51b4504 100644
> >> --- a/src/concrete-agent.cpp
> >> +++ b/src/concrete-agent.cpp
> >> @@ -34,6 +34,9 @@ ConcreteAgent::ConcreteAgent()
> >> bool ConcreteAgent::PluginVersionIsCompatible(unsigned pluginVersion)
> >> const
> >> {
> >>     unsigned version = Version();
> >> +    // Accept API/ABI changes until we reached a stable version
> >> +    if (MajorVersion(version) == 0)
> >> +        return version == pluginVersion;
> >>     return MajorVersion(version) == MajorVersion(pluginVersion) &&
> >>         MinorVersion(version) >= MinorVersion(pluginVersion);
> >> }
> 
> 

Frediano


More information about the Spice-devel mailing list