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

Submitted by Christophe de Dinechin on Nov. 16, 2017, 10:10 a.m.

Details

Message ID 20171116101020.72028-2-christophe@dinechin.org
State New
Headers show
Series "Rework option handling" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Nov. 16, 2017, 10:10 a.m.
From: Christophe de Dinechin <dinechin@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@redhat.com>
---
 include/spice-streaming-agent/plugin.hpp | 2 +-
 src/concrete-agent.cpp                   | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

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);
 }

Comments

> 
> From: Christophe de Dinechin <dinechin@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@redhat.com>

Still nack, I already explained why.

> ---
>  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);
>  }
> On 16 Nov 2017, at 11:43, Frediano Ziglio <fziglio@redhat.com> wrote:
> 
>> 
>> From: Christophe de Dinechin <dinechin@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@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.

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.


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?

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


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);
>> }
> 
> > On 16 Nov 2017, at 11:43, Frediano Ziglio <fziglio@redhat.com> wrote:
> > 
> >> 
> >> From: Christophe de Dinechin <dinechin@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@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