[v2] config: disable RTE_NEXT_ABI by default

Message ID 20181004154829.66523-1-ferruh.yigit@intel.com (mailing list archive)
State Rejected, archived
Delegated to: Thomas Monjalon
Headers
Series [v2] config: disable RTE_NEXT_ABI by default |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Ferruh Yigit Oct. 4, 2018, 3:48 p.m. UTC
  Enabling RTE_NEXT_ABI means to enable APIs that break the ABI for
the current release and these APIs are targeted for further release.

RTE_NEXT_ABI shouldn't be enabled by default.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Thomas Monjalon <thomas@monjalon.net>
Cc: Luca Boccassi <bluca@debian.org>
Cc: Christian Ehrhardt <christian.ehrhardt@canonical.com>

v2:
* fix typo in commit log
---
 config/common_base | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Comments

Thomas Monjalon Oct. 4, 2018, 3:10 p.m. UTC | #1
04/10/2018 17:48, Ferruh Yigit:
> Enabling RTE_NEXT_ABI means to enable APIs that break the ABI for
> the current release and these APIs are targeted for further release.

It seems nobody is using it in last releases.

> RTE_NEXT_ABI shouldn't be enabled by default.

The reason for having it enabled by default is that when you build DPDK
yourself, you probably want the latest features.
If packaged properly for stability, it is easy to disable it in
the package recipe.
  
Ferruh Yigit Oct. 4, 2018, 3:28 p.m. UTC | #2
On 10/4/2018 4:10 PM, Thomas Monjalon wrote:
> 04/10/2018 17:48, Ferruh Yigit:
>> Enabling RTE_NEXT_ABI means to enable APIs that break the ABI for
>> the current release and these APIs are targeted for further release.
> 
> It seems nobody is using it in last releases.
> 
>> RTE_NEXT_ABI shouldn't be enabled by default.
> 
> The reason for having it enabled by default is that when you build DPDK
> yourself, you probably want the latest features.
> If packaged properly for stability, it is easy to disable it in
> the package recipe.

My concern was (if this has been used), user may get unstable APIs and without
explicitly being aware of it.
  
Thomas Monjalon Oct. 4, 2018, 3:55 p.m. UTC | #3
04/10/2018 17:28, Ferruh Yigit:
> On 10/4/2018 4:10 PM, Thomas Monjalon wrote:
> > 04/10/2018 17:48, Ferruh Yigit:
> >> Enabling RTE_NEXT_ABI means to enable APIs that break the ABI for
> >> the current release and these APIs are targeted for further release.
> > 
> > It seems nobody is using it in last releases.
> > 
> >> RTE_NEXT_ABI shouldn't be enabled by default.
> > 
> > The reason for having it enabled by default is that when you build DPDK
> > yourself, you probably want the latest features.
> > If packaged properly for stability, it is easy to disable it in
> > the package recipe.
> 
> My concern was (if this has been used), user may get unstable APIs and without
> explicitly being aware of it.

I am OK with both defaults (enabled or disabled).
  
Bruce Richardson Oct. 5, 2018, 9:13 a.m. UTC | #4
On Thu, Oct 04, 2018 at 05:55:34PM +0200, Thomas Monjalon wrote:
> 04/10/2018 17:28, Ferruh Yigit:
> > On 10/4/2018 4:10 PM, Thomas Monjalon wrote:
> > > 04/10/2018 17:48, Ferruh Yigit:
> > >> Enabling RTE_NEXT_ABI means to enable APIs that break the ABI for
> > >> the current release and these APIs are targeted for further release.
> > > 
> > > It seems nobody is using it in last releases.
> > > 
> > >> RTE_NEXT_ABI shouldn't be enabled by default.
> > > 
> > > The reason for having it enabled by default is that when you build DPDK
> > > yourself, you probably want the latest features.
> > > If packaged properly for stability, it is easy to disable it in
> > > the package recipe.
> > 
> > My concern was (if this has been used), user may get unstable APIs and without
> > explicitly being aware of it.
> 
> I am OK with both defaults (enabled or disabled).
> 
I'd keep it as is. As said, I'm not sure it's being used right now anyway.
  
Ferruh Yigit Oct. 5, 2018, 10:17 a.m. UTC | #5
On 10/5/2018 10:13 AM, Bruce Richardson wrote:
> On Thu, Oct 04, 2018 at 05:55:34PM +0200, Thomas Monjalon wrote:
>> 04/10/2018 17:28, Ferruh Yigit:
>>> On 10/4/2018 4:10 PM, Thomas Monjalon wrote:
>>>> 04/10/2018 17:48, Ferruh Yigit:
>>>>> Enabling RTE_NEXT_ABI means to enable APIs that break the ABI for
>>>>> the current release and these APIs are targeted for further release.
>>>>
>>>> It seems nobody is using it in last releases.
>>>>
>>>>> RTE_NEXT_ABI shouldn't be enabled by default.
>>>>
>>>> The reason for having it enabled by default is that when you build DPDK
>>>> yourself, you probably want the latest features.
>>>> If packaged properly for stability, it is easy to disable it in
>>>> the package recipe.
>>>
>>> My concern was (if this has been used), user may get unstable APIs and without
>>> explicitly being aware of it.
>>
>> I am OK with both defaults (enabled or disabled).
>>
> I'd keep it as is. As said, I'm not sure it's being used right now anyway.

No, not used right now.
But I think we can use it, did you able to find chance to check:

https://mails.dpdk.org/archives/dev/2018-October/114372.html

Option D.
  
Neil Horman Oct. 5, 2018, 11:30 a.m. UTC | #6
On Fri, Oct 05, 2018 at 11:17:30AM +0100, Ferruh Yigit wrote:
> On 10/5/2018 10:13 AM, Bruce Richardson wrote:
> > On Thu, Oct 04, 2018 at 05:55:34PM +0200, Thomas Monjalon wrote:
> >> 04/10/2018 17:28, Ferruh Yigit:
> >>> On 10/4/2018 4:10 PM, Thomas Monjalon wrote:
> >>>> 04/10/2018 17:48, Ferruh Yigit:
> >>>>> Enabling RTE_NEXT_ABI means to enable APIs that break the ABI for
> >>>>> the current release and these APIs are targeted for further release.
> >>>>
> >>>> It seems nobody is using it in last releases.
> >>>>
> >>>>> RTE_NEXT_ABI shouldn't be enabled by default.
> >>>>
> >>>> The reason for having it enabled by default is that when you build DPDK
> >>>> yourself, you probably want the latest features.
> >>>> If packaged properly for stability, it is easy to disable it in
> >>>> the package recipe.
> >>>
> >>> My concern was (if this has been used), user may get unstable APIs and without
> >>> explicitly being aware of it.
> >>
> >> I am OK with both defaults (enabled or disabled).
> >>
> > I'd keep it as is. As said, I'm not sure it's being used right now anyway.
> 
> No, not used right now.
> But I think we can use it, did you able to find chance to check:
> 
> https://mails.dpdk.org/archives/dev/2018-October/114372.html
> 
> Option D.
> 

Just to propose something else, We also have the ALLOW_EXPERIMENTAL_API flag
that we IIRC default to on.  Would it be worth consolidating these two
mechanisms into one?  Currently ALLOW_EXPERIMENTAL_API lets us flag symbols that
are not yet stable, and it seems to work well.  It does not however let us
simply define out structures/variables that might adversely affect the ABI.
Would it be worth considering adding a macro (something like
__rte_experimental_symbol()), that allows a variable/struct to be defined if
ALLOW_EXPERIMENTAL_API is set, and squashed otherwise?

Neil
  
Ferruh Yigit Oct. 5, 2018, 12:35 p.m. UTC | #7
On 10/5/2018 12:30 PM, Neil Horman wrote:
> On Fri, Oct 05, 2018 at 11:17:30AM +0100, Ferruh Yigit wrote:
>> On 10/5/2018 10:13 AM, Bruce Richardson wrote:
>>> On Thu, Oct 04, 2018 at 05:55:34PM +0200, Thomas Monjalon wrote:
>>>> 04/10/2018 17:28, Ferruh Yigit:
>>>>> On 10/4/2018 4:10 PM, Thomas Monjalon wrote:
>>>>>> 04/10/2018 17:48, Ferruh Yigit:
>>>>>>> Enabling RTE_NEXT_ABI means to enable APIs that break the ABI for
>>>>>>> the current release and these APIs are targeted for further release.
>>>>>>
>>>>>> It seems nobody is using it in last releases.
>>>>>>
>>>>>>> RTE_NEXT_ABI shouldn't be enabled by default.
>>>>>>
>>>>>> The reason for having it enabled by default is that when you build DPDK
>>>>>> yourself, you probably want the latest features.
>>>>>> If packaged properly for stability, it is easy to disable it in
>>>>>> the package recipe.
>>>>>
>>>>> My concern was (if this has been used), user may get unstable APIs and without
>>>>> explicitly being aware of it.
>>>>
>>>> I am OK with both defaults (enabled or disabled).
>>>>
>>> I'd keep it as is. As said, I'm not sure it's being used right now anyway.
>>
>> No, not used right now.
>> But I think we can use it, did you able to find chance to check:
>>
>> https://mails.dpdk.org/archives/dev/2018-October/114372.html
>>
>> Option D.
>>
> 
> Just to propose something else, We also have the ALLOW_EXPERIMENTAL_API flag
> that we IIRC default to on.  Would it be worth consolidating these two
> mechanisms into one?  Currently ALLOW_EXPERIMENTAL_API lets us flag symbols that
> are not yet stable, and it seems to work well.  It does not however let us
> simply define out structures/variables that might adversely affect the ABI.
> Would it be worth considering adding a macro (something like
> __rte_experimental_symbol()), that allows a variable/struct to be defined if
> ALLOW_EXPERIMENTAL_API is set, and squashed otherwise?

RTE_NEXT_ABI is not just for symbols.

If there a new API foo(), __rte_experimental works fine to mark it experimental.

But if there is an _existing API_
"bar(char)",

and we plan to change it to
"bar(int, int)",

to publish the change early in this release we need RTE_NEXT_ABI ifdef since
both can't exist together, so it will be used as:

Release N:

 #ifdef RTE_NEXT_ABI
 bar(int, int);
 #else
 bar(char);
 #endif


Release N + 1:

 bar(int, int);
  

Patch

diff --git a/config/common_base b/config/common_base
index 2e888d13b..dbd0c9ae9 100644
--- a/config/common_base
+++ b/config/common_base
@@ -43,7 +43,7 @@  CONFIG_RTE_BUILD_SHARED_LIB=n
 #
 # Use newest code breaking previous ABI
 #
-CONFIG_RTE_NEXT_ABI=y
+CONFIG_RTE_NEXT_ABI=n
 
 #
 # Major ABI to overwrite library specific LIBABIVER