[dpdk-dev,PATCHv4,3/5] Makefiles: Add experimental tag check and warnings to trigger on use
diff mbox

Message ID 20171213151728.16747-4-nhorman@tuxdriver.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show

Checks

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

Commit Message

Neil Horman Dec. 13, 2017, 3:17 p.m. UTC
Add checks during build to ensure that all symbols in the EXPERIMENTAL
version map section have __experimental tags on their definitions, and
enable the warnings needed to announce their use.  Also add an
ALLOW_EXPERIMENTAL_APIS define to allow individual libraries and files
to declare the acceptability of experimental api usage

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Thomas Monjalon <thomas@monjalon.net>
CC: "Mcnamara, John" <john.mcnamara@intel.com>
CC: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test-eventdev/Makefile                 | 1 +
 app/test-pmd/Makefile                      | 1 +
 drivers/event/sw/Makefile                  | 1 +
 drivers/net/failsafe/Makefile              | 1 +
 drivers/net/ixgbe/Makefile                 | 1 +
 examples/eventdev_pipeline_sw_pmd/Makefile | 1 +
 examples/flow_classify/Makefile            | 1 +
 examples/ipsec-secgw/Makefile              | 1 +
 examples/service_cores/Makefile            | 1 +
 lib/librte_eal/bsdapp/eal/Makefile         | 1 +
 lib/librte_eal/linuxapp/Makefile           | 2 ++
 lib/librte_eal/linuxapp/eal/Makefile       | 2 ++
 lib/librte_eventdev/Makefile               | 1 +
 lib/librte_security/Makefile               | 1 +
 mk/internal/rte.compile-pre.mk             | 4 ++++
 mk/toolchain/clang/rte.vars.mk             | 2 +-
 mk/toolchain/gcc/rte.vars.mk               | 2 +-
 mk/toolchain/icc/rte.vars.mk               | 2 +-
 18 files changed, 23 insertions(+), 3 deletions(-)

Comments

Ferruh Yigit Jan. 11, 2018, 8:06 p.m. UTC | #1
On 12/13/2017 3:17 PM, Neil Horman wrote:
> Add checks during build to ensure that all symbols in the EXPERIMENTAL
> version map section have __experimental tags on their definitions, and
> enable the warnings needed to announce their use.  Also add an
> ALLOW_EXPERIMENTAL_APIS define to allow individual libraries and files
> to declare the acceptability of experimental api usage
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Thomas Monjalon <thomas@monjalon.net>
> CC: "Mcnamara, John" <john.mcnamara@intel.com>
> CC: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  app/test-eventdev/Makefile                 | 1 +
>  app/test-pmd/Makefile                      | 1 +
>  drivers/event/sw/Makefile                  | 1 +
>  drivers/net/failsafe/Makefile              | 1 +
>  drivers/net/ixgbe/Makefile                 | 1 +
>  examples/eventdev_pipeline_sw_pmd/Makefile | 1 +
>  examples/flow_classify/Makefile            | 1 +
>  examples/ipsec-secgw/Makefile              | 1 +
>  examples/service_cores/Makefile            | 1 +
>  lib/librte_eal/bsdapp/eal/Makefile         | 1 +
>  lib/librte_eal/linuxapp/Makefile           | 2 ++
>  lib/librte_eal/linuxapp/eal/Makefile       | 2 ++
>  lib/librte_eventdev/Makefile               | 1 +
>  lib/librte_security/Makefile               | 1 +
>  mk/internal/rte.compile-pre.mk             | 4 ++++
>  mk/toolchain/clang/rte.vars.mk             | 2 +-
>  mk/toolchain/gcc/rte.vars.mk               | 2 +-
>  mk/toolchain/icc/rte.vars.mk               | 2 +-
>  18 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/app/test-eventdev/Makefile b/app/test-eventdev/Makefile
> index dcb2ac476..78bae7633 100644
> --- a/app/test-eventdev/Makefile
> +++ b/app/test-eventdev/Makefile
> @@ -32,6 +32,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
>  
>  APP = dpdk-test-eventdev
>  
> +CFLAGS += -DALLOW_EXPERIMENTAL_APIS

Do we need this internally in DPDK? For application developers this is great,
they will get warning unless explicitly stated that they are OK with it.

Do we have any option than allowing them in DPDK library? And when experimental
API modified the users in the DPDK library internally guaranteed to be updated.
Why not globally allow this for all DPDK internally?

>  CFLAGS += -O3
>  CFLAGS += $(WERROR_FLAGS)
>  

<...>
Neil Horman Jan. 11, 2018, 8:50 p.m. UTC | #2
On Thu, Jan 11, 2018 at 08:06:43PM +0000, Ferruh Yigit wrote:
> On 12/13/2017 3:17 PM, Neil Horman wrote:
> > Add checks during build to ensure that all symbols in the EXPERIMENTAL
> > version map section have __experimental tags on their definitions, and
> > enable the warnings needed to announce their use.  Also add an
> > ALLOW_EXPERIMENTAL_APIS define to allow individual libraries and files
> > to declare the acceptability of experimental api usage
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > CC: Thomas Monjalon <thomas@monjalon.net>
> > CC: "Mcnamara, John" <john.mcnamara@intel.com>
> > CC: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  app/test-eventdev/Makefile                 | 1 +
> >  app/test-pmd/Makefile                      | 1 +
> >  drivers/event/sw/Makefile                  | 1 +
> >  drivers/net/failsafe/Makefile              | 1 +
> >  drivers/net/ixgbe/Makefile                 | 1 +
> >  examples/eventdev_pipeline_sw_pmd/Makefile | 1 +
> >  examples/flow_classify/Makefile            | 1 +
> >  examples/ipsec-secgw/Makefile              | 1 +
> >  examples/service_cores/Makefile            | 1 +
> >  lib/librte_eal/bsdapp/eal/Makefile         | 1 +
> >  lib/librte_eal/linuxapp/Makefile           | 2 ++
> >  lib/librte_eal/linuxapp/eal/Makefile       | 2 ++
> >  lib/librte_eventdev/Makefile               | 1 +
> >  lib/librte_security/Makefile               | 1 +
> >  mk/internal/rte.compile-pre.mk             | 4 ++++
> >  mk/toolchain/clang/rte.vars.mk             | 2 +-
> >  mk/toolchain/gcc/rte.vars.mk               | 2 +-
> >  mk/toolchain/icc/rte.vars.mk               | 2 +-
> >  18 files changed, 23 insertions(+), 3 deletions(-)
> > 
> > diff --git a/app/test-eventdev/Makefile b/app/test-eventdev/Makefile
> > index dcb2ac476..78bae7633 100644
> > --- a/app/test-eventdev/Makefile
> > +++ b/app/test-eventdev/Makefile
> > @@ -32,6 +32,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
> >  
> >  APP = dpdk-test-eventdev
> >  
> > +CFLAGS += -DALLOW_EXPERIMENTAL_APIS
> 
> Do we need this internally in DPDK? For application developers this is great,
> they will get warning unless explicitly stated that they are OK with it.
> 
I'm not sure what you're asking here.  As I mentioned in the initial post, I
think we should give blanket permission to any in-tree dpdk library to allow the
use of experimental API's, but that doesn't really imply that all developers
would wan't it disabled all the time.  That is to say, I could envision a
library author who, early in development would want to get a warning issued if
they used an unstable API, and, only once they were happy with their design and
choice of API usage, turn the warning off.

> Do we have any option than allowing them in DPDK library? And when experimental
> API modified the users in the DPDK library internally guaranteed to be updated.
> Why not globally allow this for all DPDK internally?
> 
For the reason I gave above.  We certainly could enable this in a more top-level
makefile so that for in-library systems it was opt-in rather than opt-out, but I
chose a more granular approach because I could envision newer libraries wanting
it on.  I also felt, generally speaking, that where warning flags were
concerned, it generally desireous to have them on by default, and make people
explicitly choose to turn them off.

Regards
Neil


> >  CFLAGS += -O3
> >  CFLAGS += $(WERROR_FLAGS)
> >  
> 
> <...>
> 
>
Ferruh Yigit Jan. 12, 2018, 11:49 a.m. UTC | #3
On 1/11/2018 8:50 PM, Neil Horman wrote:
> On Thu, Jan 11, 2018 at 08:06:43PM +0000, Ferruh Yigit wrote:
>> On 12/13/2017 3:17 PM, Neil Horman wrote:
>>> Add checks during build to ensure that all symbols in the EXPERIMENTAL
>>> version map section have __experimental tags on their definitions, and
>>> enable the warnings needed to announce their use.  Also add an
>>> ALLOW_EXPERIMENTAL_APIS define to allow individual libraries and files
>>> to declare the acceptability of experimental api usage
>>>
>>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>>> CC: Thomas Monjalon <thomas@monjalon.net>
>>> CC: "Mcnamara, John" <john.mcnamara@intel.com>
>>> CC: Bruce Richardson <bruce.richardson@intel.com>
>>> ---
>>>  app/test-eventdev/Makefile                 | 1 +
>>>  app/test-pmd/Makefile                      | 1 +
>>>  drivers/event/sw/Makefile                  | 1 +
>>>  drivers/net/failsafe/Makefile              | 1 +
>>>  drivers/net/ixgbe/Makefile                 | 1 +
>>>  examples/eventdev_pipeline_sw_pmd/Makefile | 1 +
>>>  examples/flow_classify/Makefile            | 1 +
>>>  examples/ipsec-secgw/Makefile              | 1 +
>>>  examples/service_cores/Makefile            | 1 +
>>>  lib/librte_eal/bsdapp/eal/Makefile         | 1 +
>>>  lib/librte_eal/linuxapp/Makefile           | 2 ++
>>>  lib/librte_eal/linuxapp/eal/Makefile       | 2 ++
>>>  lib/librte_eventdev/Makefile               | 1 +
>>>  lib/librte_security/Makefile               | 1 +
>>>  mk/internal/rte.compile-pre.mk             | 4 ++++
>>>  mk/toolchain/clang/rte.vars.mk             | 2 +-
>>>  mk/toolchain/gcc/rte.vars.mk               | 2 +-
>>>  mk/toolchain/icc/rte.vars.mk               | 2 +-
>>>  18 files changed, 23 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/app/test-eventdev/Makefile b/app/test-eventdev/Makefile
>>> index dcb2ac476..78bae7633 100644
>>> --- a/app/test-eventdev/Makefile
>>> +++ b/app/test-eventdev/Makefile
>>> @@ -32,6 +32,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
>>>  
>>>  APP = dpdk-test-eventdev
>>>  
>>> +CFLAGS += -DALLOW_EXPERIMENTAL_APIS
>>
>> Do we need this internally in DPDK? For application developers this is great,
>> they will get warning unless explicitly stated that they are OK with it.
>>
> I'm not sure what you're asking here.  As I mentioned in the initial post, I
> think we should give blanket permission to any in-tree dpdk library to allow the
> use of experimental API's, but that doesn't really imply that all developers
> would wan't it disabled all the time.  That is to say, I could envision a
> library author who, early in development would want to get a warning issued if
> they used an unstable API, and, only once they were happy with their design and
> choice of API usage, turn the warning off.

I got your point, but I think whoever using an experimental API in another
component in DPDK is almost always the author of the that experimental API, so
not sure this check is ever really needed within dpdk.

But OK, I guess it won't hurt to have more granular approach.

> 
>> Do we have any option than allowing them in DPDK library? And when experimental
>> API modified the users in the DPDK library internally guaranteed to be updated.
>> Why not globally allow this for all DPDK internally?
>>
> For the reason I gave above.  We certainly could enable this in a more top-level
> makefile so that for in-library systems it was opt-in rather than opt-out, but I
> chose a more granular approach because I could envision newer libraries wanting
> it on.  I also felt, generally speaking, that where warning flags were
> concerned, it generally desireous to have them on by default, and make people
> explicitly choose to turn them off.
> 
> Regards
> Neil
> 
> 
>>>  CFLAGS += -O3
>>>  CFLAGS += $(WERROR_FLAGS)
>>>  
>>
>> <...>
>>
>>
Neil Horman Jan. 12, 2018, 12:44 p.m. UTC | #4
On Fri, Jan 12, 2018 at 11:49:57AM +0000, Ferruh Yigit wrote:
> On 1/11/2018 8:50 PM, Neil Horman wrote:
> > On Thu, Jan 11, 2018 at 08:06:43PM +0000, Ferruh Yigit wrote:
> >> On 12/13/2017 3:17 PM, Neil Horman wrote:
> >>> Add checks during build to ensure that all symbols in the EXPERIMENTAL
> >>> version map section have __experimental tags on their definitions, and
> >>> enable the warnings needed to announce their use.  Also add an
> >>> ALLOW_EXPERIMENTAL_APIS define to allow individual libraries and files
> >>> to declare the acceptability of experimental api usage
> >>>
> >>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> >>> CC: Thomas Monjalon <thomas@monjalon.net>
> >>> CC: "Mcnamara, John" <john.mcnamara@intel.com>
> >>> CC: Bruce Richardson <bruce.richardson@intel.com>
> >>> ---
> >>>  app/test-eventdev/Makefile                 | 1 +
> >>>  app/test-pmd/Makefile                      | 1 +
> >>>  drivers/event/sw/Makefile                  | 1 +
> >>>  drivers/net/failsafe/Makefile              | 1 +
> >>>  drivers/net/ixgbe/Makefile                 | 1 +
> >>>  examples/eventdev_pipeline_sw_pmd/Makefile | 1 +
> >>>  examples/flow_classify/Makefile            | 1 +
> >>>  examples/ipsec-secgw/Makefile              | 1 +
> >>>  examples/service_cores/Makefile            | 1 +
> >>>  lib/librte_eal/bsdapp/eal/Makefile         | 1 +
> >>>  lib/librte_eal/linuxapp/Makefile           | 2 ++
> >>>  lib/librte_eal/linuxapp/eal/Makefile       | 2 ++
> >>>  lib/librte_eventdev/Makefile               | 1 +
> >>>  lib/librte_security/Makefile               | 1 +
> >>>  mk/internal/rte.compile-pre.mk             | 4 ++++
> >>>  mk/toolchain/clang/rte.vars.mk             | 2 +-
> >>>  mk/toolchain/gcc/rte.vars.mk               | 2 +-
> >>>  mk/toolchain/icc/rte.vars.mk               | 2 +-
> >>>  18 files changed, 23 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/app/test-eventdev/Makefile b/app/test-eventdev/Makefile
> >>> index dcb2ac476..78bae7633 100644
> >>> --- a/app/test-eventdev/Makefile
> >>> +++ b/app/test-eventdev/Makefile
> >>> @@ -32,6 +32,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
> >>>  
> >>>  APP = dpdk-test-eventdev
> >>>  
> >>> +CFLAGS += -DALLOW_EXPERIMENTAL_APIS
> >>
> >> Do we need this internally in DPDK? For application developers this is great,
> >> they will get warning unless explicitly stated that they are OK with it.
> >>
> > I'm not sure what you're asking here.  As I mentioned in the initial post, I
> > think we should give blanket permission to any in-tree dpdk library to allow the
> > use of experimental API's, but that doesn't really imply that all developers
> > would wan't it disabled all the time.  That is to say, I could envision a
> > library author who, early in development would want to get a warning issued if
> > they used an unstable API, and, only once they were happy with their design and
> > choice of API usage, turn the warning off.
> 
> I got your point, but I think whoever using an experimental API in another
> component in DPDK is almost always the author of the that experimental API, so
> not sure this check is ever really needed within dpdk.
> 
I would have thought so too, but it doesn't really bear up.  The example I used
to convince myself of a more granular approach was commit
9c38b704d280ac128003238d7d80bf07fa556a7d where the rte_service API was
introduced as experimental by Nikhil Rao, and then later used in the eal library
as part of commit a894d4815f79b0d76527d9c42b23327de1501711 by Harry van Haaren.
Its no big deal because, as we agree, internal usage should be considered safe,
but it seemed clear that differing authors were using each others code
(potentially oblivious to the experimental nature of those APIs)

> But OK, I guess it won't hurt to have more granular approach.
> 
> > 
> >> Do we have any option than allowing them in DPDK library? And when experimental
> >> API modified the users in the DPDK library internally guaranteed to be updated.
> >> Why not globally allow this for all DPDK internally?
> >>
> > For the reason I gave above.  We certainly could enable this in a more top-level
> > makefile so that for in-library systems it was opt-in rather than opt-out, but I
> > chose a more granular approach because I could envision newer libraries wanting
> > it on.  I also felt, generally speaking, that where warning flags were
> > concerned, it generally desireous to have them on by default, and make people
> > explicitly choose to turn them off.
> > 
> > Regards
> > Neil
> > 
> > 
> >>>  CFLAGS += -O3
> >>>  CFLAGS += $(WERROR_FLAGS)
> >>>  
> >>
> >> <...>
> >>
> >>
> 
>
Thomas Monjalon Jan. 21, 2018, 6:50 p.m. UTC | #5
13/12/2017 16:17, Neil Horman:
> --- a/lib/librte_eal/linuxapp/eal/Makefile
> +++ b/lib/librte_eal/linuxapp/eal/Makefile
> +CFLAGS += -DALLOW_EXPERIMENTAL_APIS

We can define this flag for the whole DPDK libraries,
	in mk/rte.vars.mk
	inside the block ifneq ($(BUILDING_RTE_SDK),)

> --- a/mk/internal/rte.compile-pre.mk
> +++ b/mk/internal/rte.compile-pre.mk
> +EXPERIMENTAL_CHECK = $(RTE_SDK)/buildtools/experimentalsyms.sh
> +CHECK_EXPERIMENTAL = $(EXPERIMENTAL_CHECK) $(SRCDIR)/$(EXPORT_MAP) $@

>  	echo $(C_TO_O_DISP); \
>  	$(C_TO_O) && \
>  	$(PMDINFO_TO_O) && \
> +	$(CHECK_EXPERIMENTAL) && \

Inserting this check looks good.
Thomas Monjalon Jan. 21, 2018, 6:54 p.m. UTC | #6
12/01/2018 13:44, Neil Horman:
> On Fri, Jan 12, 2018 at 11:49:57AM +0000, Ferruh Yigit wrote:
> > On 1/11/2018 8:50 PM, Neil Horman wrote:
> > > On Thu, Jan 11, 2018 at 08:06:43PM +0000, Ferruh Yigit wrote:
> > >> On 12/13/2017 3:17 PM, Neil Horman wrote:
> > >>> --- a/app/test-eventdev/Makefile
> > >>> +++ b/app/test-eventdev/Makefile
> > >>> @@ -32,6 +32,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
> > >>>  
> > >>>  APP = dpdk-test-eventdev
> > >>>  
> > >>> +CFLAGS += -DALLOW_EXPERIMENTAL_APIS
> > >>
> > >> Do we need this internally in DPDK? For application developers this is great,
> > >> they will get warning unless explicitly stated that they are OK with it.
> > >>
> > > I'm not sure what you're asking here.  As I mentioned in the initial post, I
> > > think we should give blanket permission to any in-tree dpdk library to allow the
> > > use of experimental API's, but that doesn't really imply that all developers
> > > would wan't it disabled all the time.  That is to say, I could envision a
> > > library author who, early in development would want to get a warning issued if
> > > they used an unstable API, and, only once they were happy with their design and
> > > choice of API usage, turn the warning off.
> > 
> > I got your point, but I think whoever using an experimental API in another
> > component in DPDK is almost always the author of the that experimental API, so
> > not sure this check is ever really needed within dpdk.
> > 
> I would have thought so too, but it doesn't really bear up.  The example I used
> to convince myself of a more granular approach was commit
> 9c38b704d280ac128003238d7d80bf07fa556a7d where the rte_service API was
> introduced as experimental by Nikhil Rao, and then later used in the eal library
> as part of commit a894d4815f79b0d76527d9c42b23327de1501711 by Harry van Haaren.
> Its no big deal because, as we agree, internal usage should be considered safe,
> but it seemed clear that differing authors were using each others code
> (potentially oblivious to the experimental nature of those APIs)
> 
> > But OK, I guess it won't hurt to have more granular approach.
> > 
> > > 
> > >> Do we have any option than allowing them in DPDK library? And when experimental
> > >> API modified the users in the DPDK library internally guaranteed to be updated.
> > >> Why not globally allow this for all DPDK internally?
> > >>
> > > For the reason I gave above.  We certainly could enable this in a more top-level
> > > makefile so that for in-library systems it was opt-in rather than opt-out, but I
> > > chose a more granular approach because I could envision newer libraries wanting
> > > it on.  I also felt, generally speaking, that where warning flags were
> > > concerned, it generally desireous to have them on by default, and make people
> > > explicitly choose to turn them off.

I think DPDK developpers look at the EXPERIMENTAL warning in the doxygen
above the prototype.
And when API will be switched to stable, we probably won't remove the flag
in the makefile to disable allowing experimental.
So at the end, we could just allow experimental API for all internal libs.
Neil Horman Jan. 22, 2018, 1:19 a.m. UTC | #7
On Sun, Jan 21, 2018 at 07:50:08PM +0100, Thomas Monjalon wrote:
> 13/12/2017 16:17, Neil Horman:
> > --- a/lib/librte_eal/linuxapp/eal/Makefile
> > +++ b/lib/librte_eal/linuxapp/eal/Makefile
> > +CFLAGS += -DALLOW_EXPERIMENTAL_APIS
> 
I think I addressed this in an earlier thread, but I'm opposed to doing
this.  I'm strongly in favor of granting an exception to any developer who wants
to use experimental apis for in-tree code, but I would like for them to have to
opt into that decision, rather than just have a blanket allowance.  I think
theres value in a developer having to make tha conscious decision when writing
new code.

> We can define this flag for the whole DPDK libraries,
> 	in mk/rte.vars.mk
> 	inside the block ifneq ($(BUILDING_RTE_SDK),)
> 
> > --- a/mk/internal/rte.compile-pre.mk
> > +++ b/mk/internal/rte.compile-pre.mk
> > +EXPERIMENTAL_CHECK = $(RTE_SDK)/buildtools/experimentalsyms.sh
> > +CHECK_EXPERIMENTAL = $(EXPERIMENTAL_CHECK) $(SRCDIR)/$(EXPORT_MAP) $@
> 
> >  	echo $(C_TO_O_DISP); \
> >  	$(C_TO_O) && \
> >  	$(PMDINFO_TO_O) && \
> > +	$(CHECK_EXPERIMENTAL) && \
> 
> Inserting this check looks good.
> 
>
Neil Horman Jan. 22, 2018, 1:34 a.m. UTC | #8
On Sun, Jan 21, 2018 at 07:54:44PM +0100, Thomas Monjalon wrote:
> 12/01/2018 13:44, Neil Horman:
> > On Fri, Jan 12, 2018 at 11:49:57AM +0000, Ferruh Yigit wrote:
> > > On 1/11/2018 8:50 PM, Neil Horman wrote:
> > > > On Thu, Jan 11, 2018 at 08:06:43PM +0000, Ferruh Yigit wrote:
> > > >> On 12/13/2017 3:17 PM, Neil Horman wrote:
> > > >>> --- a/app/test-eventdev/Makefile
> > > >>> +++ b/app/test-eventdev/Makefile
> > > >>> @@ -32,6 +32,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
> > > >>>  
> > > >>>  APP = dpdk-test-eventdev
> > > >>>  
> > > >>> +CFLAGS += -DALLOW_EXPERIMENTAL_APIS
> > > >>
> > > >> Do we need this internally in DPDK? For application developers this is great,
> > > >> they will get warning unless explicitly stated that they are OK with it.
> > > >>
> > > > I'm not sure what you're asking here.  As I mentioned in the initial post, I
> > > > think we should give blanket permission to any in-tree dpdk library to allow the
> > > > use of experimental API's, but that doesn't really imply that all developers
> > > > would wan't it disabled all the time.  That is to say, I could envision a
> > > > library author who, early in development would want to get a warning issued if
> > > > they used an unstable API, and, only once they were happy with their design and
> > > > choice of API usage, turn the warning off.
> > > 
> > > I got your point, but I think whoever using an experimental API in another
> > > component in DPDK is almost always the author of the that experimental API, so
> > > not sure this check is ever really needed within dpdk.
> > > 
> > I would have thought so too, but it doesn't really bear up.  The example I used
> > to convince myself of a more granular approach was commit
> > 9c38b704d280ac128003238d7d80bf07fa556a7d where the rte_service API was
> > introduced as experimental by Nikhil Rao, and then later used in the eal library
> > as part of commit a894d4815f79b0d76527d9c42b23327de1501711 by Harry van Haaren.
> > Its no big deal because, as we agree, internal usage should be considered safe,
> > but it seemed clear that differing authors were using each others code
> > (potentially oblivious to the experimental nature of those APIs)
> > 
> > > But OK, I guess it won't hurt to have more granular approach.
> > > 
> > > > 
> > > >> Do we have any option than allowing them in DPDK library? And when experimental
> > > >> API modified the users in the DPDK library internally guaranteed to be updated.
> > > >> Why not globally allow this for all DPDK internally?
> > > >>
> > > > For the reason I gave above.  We certainly could enable this in a more top-level
> > > > makefile so that for in-library systems it was opt-in rather than opt-out, but I
> > > > chose a more granular approach because I could envision newer libraries wanting
> > > > it on.  I also felt, generally speaking, that where warning flags were
> > > > concerned, it generally desireous to have them on by default, and make people
> > > > explicitly choose to turn them off.
> 
> I think DPDK developpers look at the EXPERIMENTAL warning in the doxygen
> above the prototype.
I'm not sure I agree with that, but regardless, my initial reasoning for writing
this tag was to call attention to experimental API's during review, rather than
their use during development, so I didn't gripe about ABI changes on expemted
code.   Additionally, weather they look at the docs or not, they can
pre-emptively turn off the warning if they choose.

> And when API will be switched to stable, we probably won't remove the flag
> in the makefile to disable allowing experimental.
Well, that remains to be seen I suppose.

> So at the end, we could just allow experimental API for all internal libs.
Thats a rather bootstrapping argument.  You are effecitvely saying that no one
developing will ever want to be warned of using experimental APIs in DPDK, so
lets just turn it off everyone.  I would really rather let individual developers
make that call at the time they author something new.

>
Thomas Monjalon Jan. 22, 2018, 1:37 a.m. UTC | #9
22/01/2018 02:34, Neil Horman:
> On Sun, Jan 21, 2018 at 07:54:44PM +0100, Thomas Monjalon wrote:
> > 12/01/2018 13:44, Neil Horman:
> > > On Fri, Jan 12, 2018 at 11:49:57AM +0000, Ferruh Yigit wrote:
> > > > On 1/11/2018 8:50 PM, Neil Horman wrote:
> > > > > On Thu, Jan 11, 2018 at 08:06:43PM +0000, Ferruh Yigit wrote:
> > > > >> On 12/13/2017 3:17 PM, Neil Horman wrote:
> > > > >>> --- a/app/test-eventdev/Makefile
> > > > >>> +++ b/app/test-eventdev/Makefile
> > > > >>> @@ -32,6 +32,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
> > > > >>>  
> > > > >>>  APP = dpdk-test-eventdev
> > > > >>>  
> > > > >>> +CFLAGS += -DALLOW_EXPERIMENTAL_APIS
> > > > >>
> > > > >> Do we need this internally in DPDK? For application developers this is great,
> > > > >> they will get warning unless explicitly stated that they are OK with it.
> > > > >>
> > > > > I'm not sure what you're asking here.  As I mentioned in the initial post, I
> > > > > think we should give blanket permission to any in-tree dpdk library to allow the
> > > > > use of experimental API's, but that doesn't really imply that all developers
> > > > > would wan't it disabled all the time.  That is to say, I could envision a
> > > > > library author who, early in development would want to get a warning issued if
> > > > > they used an unstable API, and, only once they were happy with their design and
> > > > > choice of API usage, turn the warning off.
> > > > 
> > > > I got your point, but I think whoever using an experimental API in another
> > > > component in DPDK is almost always the author of the that experimental API, so
> > > > not sure this check is ever really needed within dpdk.
> > > > 
> > > I would have thought so too, but it doesn't really bear up.  The example I used
> > > to convince myself of a more granular approach was commit
> > > 9c38b704d280ac128003238d7d80bf07fa556a7d where the rte_service API was
> > > introduced as experimental by Nikhil Rao, and then later used in the eal library
> > > as part of commit a894d4815f79b0d76527d9c42b23327de1501711 by Harry van Haaren.
> > > Its no big deal because, as we agree, internal usage should be considered safe,
> > > but it seemed clear that differing authors were using each others code
> > > (potentially oblivious to the experimental nature of those APIs)
> > > 
> > > > But OK, I guess it won't hurt to have more granular approach.
> > > > 
> > > > > 
> > > > >> Do we have any option than allowing them in DPDK library? And when experimental
> > > > >> API modified the users in the DPDK library internally guaranteed to be updated.
> > > > >> Why not globally allow this for all DPDK internally?
> > > > >>
> > > > > For the reason I gave above.  We certainly could enable this in a more top-level
> > > > > makefile so that for in-library systems it was opt-in rather than opt-out, but I
> > > > > chose a more granular approach because I could envision newer libraries wanting
> > > > > it on.  I also felt, generally speaking, that where warning flags were
> > > > > concerned, it generally desireous to have them on by default, and make people
> > > > > explicitly choose to turn them off.
> > 
> > I think DPDK developpers look at the EXPERIMENTAL warning in the doxygen
> > above the prototype.
> I'm not sure I agree with that, but regardless, my initial reasoning for writing
> this tag was to call attention to experimental API's during review, rather than
> their use during development, so I didn't gripe about ABI changes on expemted
> code.   Additionally, weather they look at the docs or not, they can
> pre-emptively turn off the warning if they choose.
> 
> > And when API will be switched to stable, we probably won't remove the flag
> > in the makefile to disable allowing experimental.
> Well, that remains to be seen I suppose.
> 
> > So at the end, we could just allow experimental API for all internal libs.
> Thats a rather bootstrapping argument.  You are effecitvely saying that no one
> developing will ever want to be warned of using experimental APIs in DPDK, so
> lets just turn it off everyone.  I would really rather let individual developers
> make that call at the time they author something new.

I don't see the benefit,
but I am OK to keep it like this.

Patch
diff mbox

diff --git a/app/test-eventdev/Makefile b/app/test-eventdev/Makefile
index dcb2ac476..78bae7633 100644
--- a/app/test-eventdev/Makefile
+++ b/app/test-eventdev/Makefile
@@ -32,6 +32,7 @@  include $(RTE_SDK)/mk/rte.vars.mk
 
 APP = dpdk-test-eventdev
 
+CFLAGS += -DALLOW_EXPERIMENTAL_APIS
 CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
 
diff --git a/app/test-pmd/Makefile b/app/test-pmd/Makefile
index d21308fcd..23f9ddb86 100644
--- a/app/test-pmd/Makefile
+++ b/app/test-pmd/Makefile
@@ -38,6 +38,7 @@  ifeq ($(CONFIG_RTE_TEST_PMD),y)
 #
 APP = testpmd
 
+CFLAGS += -DALLOW_EXPERIMENTAL_APIS
 CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
 
diff --git a/drivers/event/sw/Makefile b/drivers/event/sw/Makefile
index 2f2b67bac..e54477ec5 100644
--- a/drivers/event/sw/Makefile
+++ b/drivers/event/sw/Makefile
@@ -34,6 +34,7 @@  include $(RTE_SDK)/mk/rte.vars.mk
 LIB = librte_pmd_sw_event.a
 
 # build flags
+CFLAGS += -DALLOW_EXPERIMENTAL_APIS
 CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
 # for older GCC versions, allow us to initialize an event using
diff --git a/drivers/net/failsafe/Makefile b/drivers/net/failsafe/Makefile
index ea2a8fe46..29f1f0ff2 100644
--- a/drivers/net/failsafe/Makefile
+++ b/drivers/net/failsafe/Makefile
@@ -50,6 +50,7 @@  SRCS-$(CONFIG_RTE_LIBRTE_PMD_FAILSAFE) += failsafe_flow.c
 # No exported include files
 
 # Basic CFLAGS:
+CFLAGS += -DALLOW_EXPERIMENTAL_APIS
 CFLAGS += -std=gnu99 -Wextra
 CFLAGS += -O3
 CFLAGS += -I.
diff --git a/drivers/net/ixgbe/Makefile b/drivers/net/ixgbe/Makefile
index 511a64eb0..c1cf226f1 100644
--- a/drivers/net/ixgbe/Makefile
+++ b/drivers/net/ixgbe/Makefile
@@ -36,6 +36,7 @@  include $(RTE_SDK)/mk/rte.vars.mk
 #
 LIB = librte_pmd_ixgbe.a
 
+CFLAGS += -DALLOW_EXPERIMENTAL_APIS
 CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
 
diff --git a/examples/eventdev_pipeline_sw_pmd/Makefile b/examples/eventdev_pipeline_sw_pmd/Makefile
index de4e22c88..dc54b0df2 100644
--- a/examples/eventdev_pipeline_sw_pmd/Makefile
+++ b/examples/eventdev_pipeline_sw_pmd/Makefile
@@ -43,6 +43,7 @@  APP = eventdev_pipeline_sw_pmd
 # all source are stored in SRCS-y
 SRCS-y := main.c
 
+CFLAGS += -DALLOW_EXPERIMENTAL_APIS
 CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
 
diff --git a/examples/flow_classify/Makefile b/examples/flow_classify/Makefile
index eecdde14c..905a11cef 100644
--- a/examples/flow_classify/Makefile
+++ b/examples/flow_classify/Makefile
@@ -45,6 +45,7 @@  APP = flow_classify
 # all source are stored in SRCS-y
 SRCS-y := flow_classify.c
 
+CFLAGS += -DALLOW_EXPERIMENTAL_APIS
 CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
 
diff --git a/examples/ipsec-secgw/Makefile b/examples/ipsec-secgw/Makefile
index 9fd33cb7f..84ba9fd29 100644
--- a/examples/ipsec-secgw/Makefile
+++ b/examples/ipsec-secgw/Makefile
@@ -46,6 +46,7 @@  endif
 
 APP = ipsec-secgw
 
+CFLAGS += -DALLOW_EXPERIMENTAL_APIS
 CFLAGS += -O3 -gdwarf-2
 CFLAGS += $(WERROR_FLAGS)
 ifeq ($(CONFIG_RTE_TOOLCHAIN_ICC),y)
diff --git a/examples/service_cores/Makefile b/examples/service_cores/Makefile
index bd4a345dc..1f0448e27 100644
--- a/examples/service_cores/Makefile
+++ b/examples/service_cores/Makefile
@@ -43,6 +43,7 @@  APP = service_cores
 # all source are stored in SRCS-y
 SRCS-y := main.c
 
+CFLAGS += -DALLOW_EXPERIMENTAL_APIS
 CFLAGS += $(WERROR_FLAGS)
 
 # workaround for a gcc bug with noreturn attribute
diff --git a/lib/librte_eal/bsdapp/eal/Makefile b/lib/librte_eal/bsdapp/eal/Makefile
index afa117de4..ed595f104 100644
--- a/lib/librte_eal/bsdapp/eal/Makefile
+++ b/lib/librte_eal/bsdapp/eal/Makefile
@@ -37,6 +37,7 @@  ARCH_DIR ?= $(RTE_ARCH)
 VPATH += $(RTE_SDK)/lib/librte_eal/common
 VPATH += $(RTE_SDK)/lib/librte_eal/common/arch/$(ARCH_DIR)
 
+CFLAGS += -DALLOW_EXPERIMENTAL_APIS
 CFLAGS += -I$(SRCDIR)/include
 CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common
 CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common/include
diff --git a/lib/librte_eal/linuxapp/Makefile b/lib/librte_eal/linuxapp/Makefile
index 2ebdf3139..37367fc88 100644
--- a/lib/librte_eal/linuxapp/Makefile
+++ b/lib/librte_eal/linuxapp/Makefile
@@ -36,4 +36,6 @@  DIRS-$(CONFIG_RTE_EAL_IGB_UIO) += igb_uio
 DIRS-$(CONFIG_RTE_KNI_KMOD) += kni
 DEPDIRS-kni := eal
 
+CFLAGS += -DALLOW_EXPERIMENTAL_APIS
+
 include $(RTE_SDK)/mk/rte.subdir.mk
diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
index 5a7b8b2ac..ec810d92b 100644
--- a/lib/librte_eal/linuxapp/eal/Makefile
+++ b/lib/librte_eal/linuxapp/eal/Makefile
@@ -34,6 +34,7 @@  include $(RTE_SDK)/mk/rte.vars.mk
 LIB = librte_eal.a
 
 ARCH_DIR ?= $(RTE_ARCH)
+
 EXPORT_MAP := ../../rte_eal_version.map
 VPATH += $(RTE_SDK)/lib/librte_eal/common/arch/$(ARCH_DIR)
 
@@ -41,6 +42,7 @@  LIBABIVER := 6
 
 VPATH += $(RTE_SDK)/lib/librte_eal/common
 
+CFLAGS += -DALLOW_EXPERIMENTAL_APIS
 CFLAGS += -I$(SRCDIR)/include
 CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common
 CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common/include
diff --git a/lib/librte_eventdev/Makefile b/lib/librte_eventdev/Makefile
index 5ac22cde7..96afc3be8 100644
--- a/lib/librte_eventdev/Makefile
+++ b/lib/librte_eventdev/Makefile
@@ -37,6 +37,7 @@  LIB = librte_eventdev.a
 LIBABIVER := 3
 
 # build flags
+CFLAGS += -DALLOW_EXPERIMENTAL_APIS
 CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
 LDLIBS += -lrte_eal -lrte_ring -lrte_ethdev -lrte_hash
diff --git a/lib/librte_security/Makefile b/lib/librte_security/Makefile
index bb93ec33d..d433937a5 100644
--- a/lib/librte_security/Makefile
+++ b/lib/librte_security/Makefile
@@ -37,6 +37,7 @@  LIB = librte_security.a
 LIBABIVER := 1
 
 # build flags
+CFLAGS += -DALLOW_EXPERIMENTAL_APIS
 CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
 LDLIBS += -lrte_eal -lrte_mempool
diff --git a/mk/internal/rte.compile-pre.mk b/mk/internal/rte.compile-pre.mk
index da8dda498..83a53c202 100644
--- a/mk/internal/rte.compile-pre.mk
+++ b/mk/internal/rte.compile-pre.mk
@@ -84,6 +84,9 @@  C_TO_O = $(CC) -Wp,-MD,$(call obj2dep,$(@)).tmp $(CPPFLAGS) $(CFLAGS) \
 C_TO_O_STR = $(subst ','\'',$(C_TO_O)) #'# fix syntax highlight
 C_TO_O_DISP = $(if $(V),"$(C_TO_O_STR)","  CC $(@)")
 endif
+EXPERIMENTAL_CHECK = $(RTE_SDK)/buildtools/experimentalsyms.sh
+CHECK_EXPERIMENTAL = $(EXPERIMENTAL_CHECK) $(SRCDIR)/$(EXPORT_MAP) $@
+
 PMDINFO_GEN = $(RTE_SDK_BIN)/app/dpdk-pmdinfogen $@ $@.pmd.c
 PMDINFO_CC = $(CC) $(CPPFLAGS) $(CFLAGS) -c -o $@.pmd.o $@.pmd.c
 PMDINFO_LD = $(CROSS)ld $(LDFLAGS) -r -o $@.o $@.pmd.o $@
@@ -100,6 +103,7 @@  C_TO_O_DO = @set -e; \
 	echo $(C_TO_O_DISP); \
 	$(C_TO_O) && \
 	$(PMDINFO_TO_O) && \
+	$(CHECK_EXPERIMENTAL) && \
 	echo $(C_TO_O_CMD) > $(call obj2cmd,$(@)) && \
 	sed 's,'$@':,dep_'$@' =,' $(call obj2dep,$(@)).tmp > $(call obj2dep,$(@)) && \
 	rm -f $(call obj2dep,$(@)).tmp
diff --git a/mk/toolchain/clang/rte.vars.mk b/mk/toolchain/clang/rte.vars.mk
index dde922d22..3cfe92545 100644
--- a/mk/toolchain/clang/rte.vars.mk
+++ b/mk/toolchain/clang/rte.vars.mk
@@ -67,7 +67,7 @@  WERROR_FLAGS := -W -Wall -Wstrict-prototypes -Wmissing-prototypes
 WERROR_FLAGS += -Wmissing-declarations -Wold-style-definition -Wpointer-arith
 WERROR_FLAGS += -Wnested-externs -Wcast-qual
 WERROR_FLAGS += -Wformat-nonliteral -Wformat-security
-WERROR_FLAGS += -Wundef -Wwrite-strings
+WERROR_FLAGS += -Wundef -Wwrite-strings -Wdeprecated
 
 ifeq ($(RTE_DEVEL_BUILD),y)
 WERROR_FLAGS += -Werror
diff --git a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk
index 3b907e201..3356a37c8 100644
--- a/mk/toolchain/gcc/rte.vars.mk
+++ b/mk/toolchain/gcc/rte.vars.mk
@@ -75,7 +75,7 @@  WERROR_FLAGS := -W -Wall -Wstrict-prototypes -Wmissing-prototypes
 WERROR_FLAGS += -Wmissing-declarations -Wold-style-definition -Wpointer-arith
 WERROR_FLAGS += -Wcast-align -Wnested-externs -Wcast-qual
 WERROR_FLAGS += -Wformat-nonliteral -Wformat-security
-WERROR_FLAGS += -Wundef -Wwrite-strings
+WERROR_FLAGS += -Wundef -Wwrite-strings -Wdeprecated
 
 ifeq ($(RTE_DEVEL_BUILD),y)
 WERROR_FLAGS += -Werror
diff --git a/mk/toolchain/icc/rte.vars.mk b/mk/toolchain/icc/rte.vars.mk
index 33a8ba79e..58940602e 100644
--- a/mk/toolchain/icc/rte.vars.mk
+++ b/mk/toolchain/icc/rte.vars.mk
@@ -73,7 +73,7 @@  TOOLCHAIN_ASFLAGS =
 WERROR_FLAGS := -Wall -w2 -diag-disable 271 -diag-warning 1478
 WERROR_FLAGS += -diag-disable 13368 -diag-disable 15527
 WERROR_FLAGS += -diag-disable 188
-WERROR_FLAGS += -diag-disable 11074 -diag-disable 11076
+WERROR_FLAGS += -diag-disable 11074 -diag-disable 11076 -Wdeprecated
 
 ifeq ($(RTE_DEVEL_BUILD),y)
 WERROR_FLAGS += -Werror-all