Re: [dpdk-dev] [PATCH v4] meter: provide experimental alias of API for old apps

[Ray Kinsella <mdr@ashroe.eu>]

On 18/05/2020 12:18, Dumitrescu, Cristian wrote:
> 
> 
>> -----Original Message-----
>> From: Thomas Monjalon <thomas@monjalon.net>
>> Sent: Monday, May 18, 2020 11:46 AM
>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Ray Kinsella <mdr@ashroe.eu>;
>> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
>> Cc: Neil Horman <nhorman@tuxdriver.com>; Eelco Chaudron
>> <echaudro@redhat.com>; dev@dpdk.org; David Marchand
>> <david.marchand@redhat.com>; stable@dpdk.org; Luca Boccassi
>> <bluca@debian.org>; Richardson, Bruce <bruce.richardson@intel.com>;
>> Stokes, Ian <ian.stokes@intel.com>; Andrzej Ostruszka
>> <amo@semihalf.com>
>> Subject: Re: [PATCH v4] meter: provide experimental alias of API for old apps
>>
>> 18/05/2020 11:30, Ray Kinsella:
>>> On 18/05/2020 10:22, Thomas Monjalon wrote:
>>>> 18/05/2020 08:29, Ray Kinsella:
>>>>> On 17/05/2020 20:52, Dumitrescu, Cristian wrote:
>>>>>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>>>>>>>
>>>>>>> On v20.02 some meter APIs have been matured and symbols moved
>> from
>>>>>>> EXPERIMENTAL to DPDK_20.0.1 block.
>>>>>>>
>>>>>>> This can break the applications that were using these mentioned APIs
>> on
>>>>>>> v19.11. Although there is no modification on the APIs and the action is
>>>>>>> positive and matures the APIs, the affect can be negative to
>>>>>>> applications.
>>>>>>>
>>>>>>> Since experimental APIs can change or go away without notice as part
>> of
>>>>>>> contract, to prevent this negative affect that may occur by maturing
>>>>>>> experimental API, a process update already suggested, which
>> enables
>>>>>>> aliasing without forcing it:
>>>>>>> https://patches.dpdk.org/patch/65863/
>>>>>>>
>>>>>>
>>>>>> Personally, I am not convinced this is really needed.
>>>>>>
>>>>>> Are there any users asking for this?
>>>>>
>>>>> As it happens it is all breaking our abi regression test suite.
>>>>> One of the things we do is to run the unit tests binary from v19.11
>> against the latest release.
>>>>>
>>>>>> Is there any other library where this is also applied, or is librte_meter
>> the only library?
>>>>>
>>>>> librte_meter is the only example AFAIK.
>>>>> But then we only have one example of needing symbol versioning also
>> at the moment (Cryptodev).
>>>>>
>>>>> This is going to happen with experimental symbols that have been
>> around a while,
>>>>> that have become used in applications. It is a non-mandatory tool a
>> maintainer can use
>>>>> to preserve abi compatibility.
>>>>
>>>> If you want to maintain ABI compatibility of experimental symbols,
>>>> it IS a mandatory tool.
>>>> You cannot enforce your "ABI regression test suite" and at the same time
>>>> say it is "non-mandatory".
>>>>
>>>> The real question here is to know whether we want to maintain
>> compatibility
>>>> of experimental symbols. We said no. Then we said we can.
>>>> The main concern is the message clarity in my opinion.
>>>>
>>>
>>> There is complete clarity, there is no obligation.
>>> Our lack of obligation around experimental, is upfront in the policy is
>> upfront in the policy.
>>>
>>> "Libraries or APIs marked as experimental may change without constraint,
>> as they are not considered part of an ABI version. Experimental libraries have
>> the major ABI version 0."
>>>
>>> Later we give the _option_ without obligation to add an alias to
>> experimental.pls see the v6.
>>>
>>> +   - In situations in which an ``experimental`` symbol has been stable for
>> some
>>> +     time. When promoting the symbol to become part of the next ABI
>> version, the
>>> +     maintainer may choose to provide an alias to the ``experimental`` tag,
>> so
>>> +     as not to break consuming applications.
>>>
>>> So it is something a Maintainer, _may_ choose to do.
>>> I use the word, "may" not "will" as there is no obligation's associated with
>> experimental.
>>
>>
>> OK Ray, this is my understanding as well.
>>
>> The only difficult part to understand is when claiming
>> "it is all breaking our abi regression test suite"
>> to justify the choice.
>> As the maintainer (Cristian) says he does not like this change,
>> it means the regression test suite should skip this case, right?
>>
> 
> I am yet to be convinced of the value of this, but if some people think it is useful, I am willing to compromise. This is subject to this code being temporary code to be removed for 20.11 release, which Ray already confirmed.
> 
> Ray, a few more suggestions, are you OK with them?
> 1. Move this code to a separate file in the library (suggest rte_meter_abi_compat.c as the file name)
> 2. Clearly state in the patch description this is temporary code to be removed for 20.11 release.
> 3. Agree that you or Ferruh take the AR to send a patch prior to the 20.11 release to remove this code.
> 
> Thanks,
> Cristian

Hi Cristain - I am good with all of the above.

Ray K