out of tree driver builds broken with C++

out of tree driver builds broken with C++

[Tyler Retzlaff]

while the driver api is "internal" we agreed some time ago that drivers
could be built external to the dpdk tree. by enabling the meson setup
option -Denable_driver_sdk=true.

it was agreed that the driver api was internal and would attract no
binary compatibility support which was fine.  this change has now
imposed a further restriction that out of tree drivers have to be
authored in C only as non-C++ compatible code will invariably leak into
the internal structures.

you won't allow us to build C++ drivers in the dpdk tree and it seems
now you are preventing building of C++ drivers outside of the tree too.

could we please re-evaluate this.

thanks.

commit 7a335720575507f55b723b1e10bfea7daeba1386
Author: Thomas Monjalon <thomas@monjalon.net>
Date:   Wed Sep 15 18:46:35 2021 +0200

    lib: remove C++ include guard from private headers

    The private headers are compiled internally with a C compiler.
    Thus extern "C" declaration is useless in such files.

    Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Re: out of tree driver builds broken with C++

[Thomas Monjalon]

14/02/2022 10:13, Tyler Retzlaff:
> while the driver api is "internal" we agreed some time ago that drivers
> could be built external to the dpdk tree. by enabling the meson setup
> option -Denable_driver_sdk=true.
> 
> it was agreed that the driver api was internal and would attract no
> binary compatibility support which was fine.  this change has now
> imposed a further restriction that out of tree drivers have to be
> authored in C only as non-C++ compatible code will invariably leak into
> the internal structures.
> 
> you won't allow us to build C++ drivers in the dpdk tree and it seems
> now you are preventing building of C++ drivers outside of the tree too.

That's the problem of non-written assumptions, they are unknown or forgotten.
Did we agree to support out-of-tree drivers in C++?

We really need to make things clear and written in documentation.

> could we please re-evaluate this.

Yes we can re-evaluate.
What is the list of impacted files?



> commit 7a335720575507f55b723b1e10bfea7daeba1386
> Author: Thomas Monjalon <thomas@monjalon.net>
> Date:   Wed Sep 15 18:46:35 2021 +0200
> 
>     lib: remove C++ include guard from private headers
> 
>     The private headers are compiled internally with a C compiler.
>     Thus extern "C" declaration is useless in such files.
> 
>     Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Re: out of tree driver builds broken with C++

[Bruce Richardson]

On Mon, Feb 14, 2022 at 10:22:08AM +0100, Thomas Monjalon wrote:
> 14/02/2022 10:13, Tyler Retzlaff:
> > while the driver api is "internal" we agreed some time ago that drivers
> > could be built external to the dpdk tree. by enabling the meson setup
> > option -Denable_driver_sdk=true.
> > 
> > it was agreed that the driver api was internal and would attract no
> > binary compatibility support which was fine.  this change has now
> > imposed a further restriction that out of tree drivers have to be
> > authored in C only as non-C++ compatible code will invariably leak into
> > the internal structures.
> > 
> > you won't allow us to build C++ drivers in the dpdk tree and it seems
> > now you are preventing building of C++ drivers outside of the tree too.
> 
> That's the problem of non-written assumptions, they are unknown or forgotten.
> Did we agree to support out-of-tree drivers in C++?
> 
> We really need to make things clear and written in documentation.
> 
> > could we please re-evaluate this.
> 
> Yes we can re-evaluate.
> What is the list of impacted files?
> 
Hacking meson files a bit, the list of SDK header files is reported as below.

/Bruce

Message: SDK headers: 
Message: ethdev_driver.h

Message: ethdev_pci.h

Message: ethdev_vdev.h

Message: cryptodev_pmd.h

Message: eventdev_pmd.h

Message: eventdev_pmd_pci.h

Message: eventdev_pmd_vdev.h

Message: eventdev_trace.h

Message: event_timer_adapter_pmd.h

Message: rte_dmadev_pmd.h

Message: vdpa_driver.h

Re: out of tree driver builds broken with C++

[Thomas Monjalon]

14/02/2022 11:45, Bruce Richardson:
> On Mon, Feb 14, 2022 at 10:22:08AM +0100, Thomas Monjalon wrote:
> > 14/02/2022 10:13, Tyler Retzlaff:
> > > while the driver api is "internal" we agreed some time ago that drivers
> > > could be built external to the dpdk tree. by enabling the meson setup
> > > option -Denable_driver_sdk=true.
> > > 
> > > it was agreed that the driver api was internal and would attract no
> > > binary compatibility support which was fine.  this change has now
> > > imposed a further restriction that out of tree drivers have to be
> > > authored in C only as non-C++ compatible code will invariably leak into
> > > the internal structures.
> > > 
> > > you won't allow us to build C++ drivers in the dpdk tree and it seems
> > > now you are preventing building of C++ drivers outside of the tree too.
> > 
> > That's the problem of non-written assumptions, they are unknown or forgotten.
> > Did we agree to support out-of-tree drivers in C++?
> > 
> > We really need to make things clear and written in documentation.
> > 
> > > could we please re-evaluate this.
> > 
> > Yes we can re-evaluate.
> > What is the list of impacted files?
> > 
> Hacking meson files a bit, the list of SDK header files is reported as below.
> 
> /Bruce
> 
> Message: SDK headers: 
> Message: ethdev_driver.h
> Message: ethdev_pci.h
> Message: ethdev_vdev.h
> Message: cryptodev_pmd.h
> Message: eventdev_pmd.h
> Message: eventdev_pmd_pci.h
> Message: eventdev_pmd_vdev.h
> Message: eventdev_trace.h
> Message: event_timer_adapter_pmd.h
> Message: rte_dmadev_pmd.h
> Message: vdpa_driver.h

I see no harm in supporting C++ include of these headers.
Any objection?

Could we have a test in chkincs for the SDK headers?

Re: out of tree driver builds broken with C++

[Bruce Richardson]

On Mon, Feb 14, 2022 at 12:19:24PM +0100, Thomas Monjalon wrote:
> 14/02/2022 11:45, Bruce Richardson:
> > On Mon, Feb 14, 2022 at 10:22:08AM +0100, Thomas Monjalon wrote:
> > > 14/02/2022 10:13, Tyler Retzlaff:
> > > > while the driver api is "internal" we agreed some time ago that drivers
> > > > could be built external to the dpdk tree. by enabling the meson setup
> > > > option -Denable_driver_sdk=true.
> > > > 
> > > > it was agreed that the driver api was internal and would attract no
> > > > binary compatibility support which was fine.  this change has now
> > > > imposed a further restriction that out of tree drivers have to be
> > > > authored in C only as non-C++ compatible code will invariably leak into
> > > > the internal structures.
> > > > 
> > > > you won't allow us to build C++ drivers in the dpdk tree and it seems
> > > > now you are preventing building of C++ drivers outside of the tree too.
> > > 
> > > That's the problem of non-written assumptions, they are unknown or forgotten.
> > > Did we agree to support out-of-tree drivers in C++?
> > > 
> > > We really need to make things clear and written in documentation.
> > > 
> > > > could we please re-evaluate this.
> > > 
> > > Yes we can re-evaluate.
> > > What is the list of impacted files?
> > > 
> > Hacking meson files a bit, the list of SDK header files is reported as below.
> > 
> > /Bruce
> > 
> > Message: SDK headers: 
> > Message: ethdev_driver.h
> > Message: ethdev_pci.h
> > Message: ethdev_vdev.h
> > Message: cryptodev_pmd.h
> > Message: eventdev_pmd.h
> > Message: eventdev_pmd_pci.h
> > Message: eventdev_pmd_vdev.h
> > Message: eventdev_trace.h
> > Message: event_timer_adapter_pmd.h
> > Message: rte_dmadev_pmd.h
> > Message: vdpa_driver.h
> 
> I see no harm in supporting C++ include of these headers.
> Any objection?
> 
> Could we have a test in chkincs for the SDK headers?
> 
Yes. It may make things a little more complicated, though, as it seems
these headers also have a tendency to rely on some driver headers -
specifically bus driver headers.

/Bruce

Re: out of tree driver builds broken with C++

[Bruce Richardson]

On Mon, Feb 14, 2022 at 11:43:35AM +0000, Bruce Richardson wrote:
> On Mon, Feb 14, 2022 at 12:19:24PM +0100, Thomas Monjalon wrote:
> > 14/02/2022 11:45, Bruce Richardson:
> > > On Mon, Feb 14, 2022 at 10:22:08AM +0100, Thomas Monjalon wrote:
> > > > 14/02/2022 10:13, Tyler Retzlaff:
> > > > > while the driver api is "internal" we agreed some time ago that drivers
> > > > > could be built external to the dpdk tree. by enabling the meson setup
> > > > > option -Denable_driver_sdk=true.
> > > > > 
> > > > > it was agreed that the driver api was internal and would attract no
> > > > > binary compatibility support which was fine.  this change has now
> > > > > imposed a further restriction that out of tree drivers have to be
> > > > > authored in C only as non-C++ compatible code will invariably leak into
> > > > > the internal structures.
> > > > > 
> > > > > you won't allow us to build C++ drivers in the dpdk tree and it seems
> > > > > now you are preventing building of C++ drivers outside of the tree too.
> > > > 
> > > > That's the problem of non-written assumptions, they are unknown or forgotten.
> > > > Did we agree to support out-of-tree drivers in C++?
> > > > 
> > > > We really need to make things clear and written in documentation.
> > > > 
> > > > > could we please re-evaluate this.
> > > > 
> > > > Yes we can re-evaluate.
> > > > What is the list of impacted files?
> > > > 
> > > Hacking meson files a bit, the list of SDK header files is reported as below.
> > > 
> > > /Bruce
> > > 
> > > Message: SDK headers: 
> > > Message: ethdev_driver.h
> > > Message: ethdev_pci.h
> > > Message: ethdev_vdev.h
> > > Message: cryptodev_pmd.h
> > > Message: eventdev_pmd.h
> > > Message: eventdev_pmd_pci.h
> > > Message: eventdev_pmd_vdev.h
> > > Message: eventdev_trace.h
> > > Message: event_timer_adapter_pmd.h
> > > Message: rte_dmadev_pmd.h
> > > Message: vdpa_driver.h
> > 
> > I see no harm in supporting C++ include of these headers.
> > Any objection?
> > 
> > Could we have a test in chkincs for the SDK headers?
> > 
> Yes. It may make things a little more complicated, though, as it seems
> these headers also have a tendency to rely on some driver headers -
> specifically bus driver headers.
> 

Working on a patch to add these to checks. However, just running a c++
compile does not check for valid 'extern "C"' blocks. Adding the following
to chkincs/meson.build as a basic sanity check throws up a larger list of
files to be looked at.

Diff:

+# check for extern C in files, since this is not detected as an error by the compiler
+grep = find_program('grep', required: false)
+if grep.found()
+    errlist = run_command([grep, '--files-without-match', '^extern "C"', dpdk_chkinc_headers],
+            check: true, capture: true).stdout().split()
+    if errlist != []
+        error('Files missing C++ \'extern "C"\' guards:\n- ' + '\n- '.join(errlist))
+    endif
+endif
+

Output:
../buildtools/chkincs/meson.build:45:8: ERROR: Problem encountered: Files missing C++ 'extern "C"' guards:
- /home/bruce/dpdk.org/lib/telemetry/rte_telemetry.h
- /home/bruce/dpdk.org/lib/eal/include/rte_bitops.h
- /home/bruce/dpdk.org/lib/eal/include/rte_branch_prediction.h
- /home/bruce/dpdk.org/lib/eal/include/rte_compat.h
- /home/bruce/dpdk.org/lib/eal/include/rte_hypervisor.h
- /home/bruce/dpdk.org/lib/eal/include/rte_keepalive.h
- /home/bruce/dpdk.org/lib/eal/include/rte_pci_dev_feature_defs.h
- /home/bruce/dpdk.org/lib/eal/include/rte_pci_dev_features.h
- /home/bruce/dpdk.org/lib/eal/include/rte_time.h
- /home/bruce/dpdk.org/lib/eal/include/rte_trace_point_register.h
- /home/bruce/dpdk.org/lib/eal/linux/include/rte_os.h
- /home/bruce/dpdk.org/lib/ethdev/rte_dev_info.h
- /home/bruce/dpdk.org/lib/ethdev/ethdev_driver.h
- /home/bruce/dpdk.org/lib/ethdev/ethdev_pci.h
- /home/bruce/dpdk.org/lib/ethdev/ethdev_vdev.h
- /home/bruce/dpdk.org/lib/metrics/rte_metrics_telemetry.h
- /home/bruce/dpdk.org/lib/acl/rte_acl_osdep.h
- /home/bruce/dpdk.org/lib/bpf/bpf_def.h
- /home/bruce/dpdk.org/lib/compressdev/rte_compressdev_internal.h
- /home/bruce/dpdk.org/lib/cryptodev/cryptodev_pmd.h
- /home/bruce/dpdk.org/lib/eventdev/rte_event_ring.h
- /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd.h
- /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd_pci.h
- /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd_vdev.h
- /home/bruce/dpdk.org/lib/kni/rte_kni_common.h
- /home/bruce/dpdk.org/lib/vhost/rte_vdpa.h
- /home/bruce/dpdk.org/lib/vhost/rte_vhost_async.h
- /home/bruce/dpdk.org/lib/vhost/rte_vhost_crypto.h
- /home/bruce/dpdk.org/lib/vhost/vdpa_driver.h

Regards,
/Bruce

Re: out of tree driver builds broken with C++

[Stephen Hemminger]

On Mon, 14 Feb 2022 15:03:43 +0000
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Mon, Feb 14, 2022 at 11:43:35AM +0000, Bruce Richardson wrote:
> > On Mon, Feb 14, 2022 at 12:19:24PM +0100, Thomas Monjalon wrote:  
> > > 14/02/2022 11:45, Bruce Richardson:  
> > > > On Mon, Feb 14, 2022 at 10:22:08AM +0100, Thomas Monjalon wrote:  
> > > > > 14/02/2022 10:13, Tyler Retzlaff:  
> > > > > > while the driver api is "internal" we agreed some time ago that drivers
> > > > > > could be built external to the dpdk tree. by enabling the meson setup
> > > > > > option -Denable_driver_sdk=true.
> > > > > > 
> > > > > > it was agreed that the driver api was internal and would attract no
> > > > > > binary compatibility support which was fine.  this change has now
> > > > > > imposed a further restriction that out of tree drivers have to be
> > > > > > authored in C only as non-C++ compatible code will invariably leak into
> > > > > > the internal structures.
> > > > > > 
> > > > > > you won't allow us to build C++ drivers in the dpdk tree and it seems
> > > > > > now you are preventing building of C++ drivers outside of the tree too.  
> > > > > 
> > > > > That's the problem of non-written assumptions, they are unknown or forgotten.
> > > > > Did we agree to support out-of-tree drivers in C++?
> > > > > 
> > > > > We really need to make things clear and written in documentation.
> > > > >   
> > > > > > could we please re-evaluate this.  
> > > > > 
> > > > > Yes we can re-evaluate.
> > > > > What is the list of impacted files?
> > > > >   
> > > > Hacking meson files a bit, the list of SDK header files is reported as below.
> > > > 
> > > > /Bruce
> > > > 
> > > > Message: SDK headers: 
> > > > Message: ethdev_driver.h
> > > > Message: ethdev_pci.h
> > > > Message: ethdev_vdev.h
> > > > Message: cryptodev_pmd.h
> > > > Message: eventdev_pmd.h
> > > > Message: eventdev_pmd_pci.h
> > > > Message: eventdev_pmd_vdev.h
> > > > Message: eventdev_trace.h
> > > > Message: event_timer_adapter_pmd.h
> > > > Message: rte_dmadev_pmd.h
> > > > Message: vdpa_driver.h  
> > > 
> > > I see no harm in supporting C++ include of these headers.
> > > Any objection?
> > > 
> > > Could we have a test in chkincs for the SDK headers?
> > >   
> > Yes. It may make things a little more complicated, though, as it seems
> > these headers also have a tendency to rely on some driver headers -
> > specifically bus driver headers.
> >   
> 
> Working on a patch to add these to checks. However, just running a c++
> compile does not check for valid 'extern "C"' blocks. Adding the following
> to chkincs/meson.build as a basic sanity check throws up a larger list of
> files to be looked at.
> 
> Diff:
> 
> +# check for extern C in files, since this is not detected as an error by the compiler
> +grep = find_program('grep', required: false)
> +if grep.found()
> +    errlist = run_command([grep, '--files-without-match', '^extern "C"', dpdk_chkinc_headers],
> +            check: true, capture: true).stdout().split()
> +    if errlist != []
> +        error('Files missing C++ \'extern "C"\' guards:\n- ' + '\n- '.join(errlist))
> +    endif
> +endif
> +
> 
> Output:
> ../buildtools/chkincs/meson.build:45:8: ERROR: Problem encountered: Files missing C++ 'extern "C"' guards:
> - /home/bruce/dpdk.org/lib/telemetry/rte_telemetry.h
> - /home/bruce/dpdk.org/lib/eal/include/rte_bitops.h
> - /home/bruce/dpdk.org/lib/eal/include/rte_branch_prediction.h
> - /home/bruce/dpdk.org/lib/eal/include/rte_compat.h
> - /home/bruce/dpdk.org/lib/eal/include/rte_hypervisor.h
> - /home/bruce/dpdk.org/lib/eal/include/rte_keepalive.h
> - /home/bruce/dpdk.org/lib/eal/include/rte_pci_dev_feature_defs.h
> - /home/bruce/dpdk.org/lib/eal/include/rte_pci_dev_features.h
> - /home/bruce/dpdk.org/lib/eal/include/rte_time.h
> - /home/bruce/dpdk.org/lib/eal/include/rte_trace_point_register.h
> - /home/bruce/dpdk.org/lib/eal/linux/include/rte_os.h
> - /home/bruce/dpdk.org/lib/ethdev/rte_dev_info.h
> - /home/bruce/dpdk.org/lib/ethdev/ethdev_driver.h
> - /home/bruce/dpdk.org/lib/ethdev/ethdev_pci.h
> - /home/bruce/dpdk.org/lib/ethdev/ethdev_vdev.h
> - /home/bruce/dpdk.org/lib/metrics/rte_metrics_telemetry.h
> - /home/bruce/dpdk.org/lib/acl/rte_acl_osdep.h
> - /home/bruce/dpdk.org/lib/bpf/bpf_def.h
> - /home/bruce/dpdk.org/lib/compressdev/rte_compressdev_internal.h
> - /home/bruce/dpdk.org/lib/cryptodev/cryptodev_pmd.h
> - /home/bruce/dpdk.org/lib/eventdev/rte_event_ring.h
> - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd.h
> - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd_pci.h
> - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd_vdev.h
> - /home/bruce/dpdk.org/lib/kni/rte_kni_common.h
> - /home/bruce/dpdk.org/lib/vhost/rte_vdpa.h
> - /home/bruce/dpdk.org/lib/vhost/rte_vhost_async.h
> - /home/bruce/dpdk.org/lib/vhost/rte_vhost_crypto.h
> - /home/bruce/dpdk.org/lib/vhost/vdpa_driver.h
> 
> Regards,
> /Bruce

The actual C++ guards are small and have no impact therefore we
should accept patches as needed. Having more guards than needed is
just noise.

But the responsibility of telling DPDK project where they are required
should fall to any out-of-tree users who care.

Re: out of tree driver builds broken with C++

[Thomas Monjalon]

14/02/2022 18:t24, Stephen Hemminger:
> On Mon, 14 Feb 2022 15:03:43 +0000
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > On Mon, Feb 14, 2022 at 11:43:35AM +0000, Bruce Richardson wrote:
> > > On Mon, Feb 14, 2022 at 12:19:24PM +0100, Thomas Monjalon wrote:  
> > > > 14/02/2022 11:45, Bruce Richardson:  
> > > > > On Mon, Feb 14, 2022 at 10:22:08AM +0100, Thomas Monjalon wrote:  
> > > > > > 14/02/2022 10:13, Tyler Retzlaff:  
> > > > > > > while the driver api is "internal" we agreed some time ago that drivers
> > > > > > > could be built external to the dpdk tree. by enabling the meson setup
> > > > > > > option -Denable_driver_sdk=true.
> > > > > > > 
> > > > > > > it was agreed that the driver api was internal and would attract no
> > > > > > > binary compatibility support which was fine.  this change has now
> > > > > > > imposed a further restriction that out of tree drivers have to be
> > > > > > > authored in C only as non-C++ compatible code will invariably leak into
> > > > > > > the internal structures.
> > > > > > > 
> > > > > > > you won't allow us to build C++ drivers in the dpdk tree and it seems
> > > > > > > now you are preventing building of C++ drivers outside of the tree too.  
> > > > > > 
> > > > > > That's the problem of non-written assumptions, they are unknown or forgotten.
> > > > > > Did we agree to support out-of-tree drivers in C++?
> > > > > > 
> > > > > > We really need to make things clear and written in documentation.
> > > > > >   
> > > > > > > could we please re-evaluate this.  
> > > > > > 
> > > > > > Yes we can re-evaluate.
> > > > > > What is the list of impacted files?
> > > > > >   
> > > > > Hacking meson files a bit, the list of SDK header files is reported as below.
> > > > > 
> > > > > /Bruce
> > > > > 
> > > > > Message: SDK headers: 
> > > > > Message: ethdev_driver.h
> > > > > Message: ethdev_pci.h
> > > > > Message: ethdev_vdev.h
> > > > > Message: cryptodev_pmd.h
> > > > > Message: eventdev_pmd.h
> > > > > Message: eventdev_pmd_pci.h
> > > > > Message: eventdev_pmd_vdev.h
> > > > > Message: eventdev_trace.h
> > > > > Message: event_timer_adapter_pmd.h
> > > > > Message: rte_dmadev_pmd.h
> > > > > Message: vdpa_driver.h  
> > > > 
> > > > I see no harm in supporting C++ include of these headers.
> > > > Any objection?
> > > > 
> > > > Could we have a test in chkincs for the SDK headers?
> > > >   
> > > Yes. It may make things a little more complicated, though, as it seems
> > > these headers also have a tendency to rely on some driver headers -
> > > specifically bus driver headers.
> > >   
> > 
> > Working on a patch to add these to checks. However, just running a c++
> > compile does not check for valid 'extern "C"' blocks. Adding the following
> > to chkincs/meson.build as a basic sanity check throws up a larger list of
> > files to be looked at.
> > 
> > Diff:
> > 
> > +# check for extern C in files, since this is not detected as an error by the compiler
> > +grep = find_program('grep', required: false)
> > +if grep.found()
> > +    errlist = run_command([grep, '--files-without-match', '^extern "C"', dpdk_chkinc_headers],
> > +            check: true, capture: true).stdout().split()
> > +    if errlist != []
> > +        error('Files missing C++ \'extern "C"\' guards:\n- ' + '\n- '.join(errlist))
> > +    endif
> > +endif
> > +
> > 
> > Output:
> > ../buildtools/chkincs/meson.build:45:8: ERROR: Problem encountered: Files missing C++ 'extern "C"' guards:
> > - /home/bruce/dpdk.org/lib/telemetry/rte_telemetry.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_bitops.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_branch_prediction.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_compat.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_hypervisor.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_keepalive.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_pci_dev_feature_defs.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_pci_dev_features.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_time.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_trace_point_register.h
> > - /home/bruce/dpdk.org/lib/eal/linux/include/rte_os.h
> > - /home/bruce/dpdk.org/lib/ethdev/rte_dev_info.h
> > - /home/bruce/dpdk.org/lib/ethdev/ethdev_driver.h
> > - /home/bruce/dpdk.org/lib/ethdev/ethdev_pci.h
> > - /home/bruce/dpdk.org/lib/ethdev/ethdev_vdev.h
> > - /home/bruce/dpdk.org/lib/metrics/rte_metrics_telemetry.h
> > - /home/bruce/dpdk.org/lib/acl/rte_acl_osdep.h
> > - /home/bruce/dpdk.org/lib/bpf/bpf_def.h
> > - /home/bruce/dpdk.org/lib/compressdev/rte_compressdev_internal.h
> > - /home/bruce/dpdk.org/lib/cryptodev/cryptodev_pmd.h
> > - /home/bruce/dpdk.org/lib/eventdev/rte_event_ring.h
> > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd.h
> > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd_pci.h
> > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd_vdev.h
> > - /home/bruce/dpdk.org/lib/kni/rte_kni_common.h
> > - /home/bruce/dpdk.org/lib/vhost/rte_vdpa.h
> > - /home/bruce/dpdk.org/lib/vhost/rte_vhost_async.h
> > - /home/bruce/dpdk.org/lib/vhost/rte_vhost_crypto.h
> > - /home/bruce/dpdk.org/lib/vhost/vdpa_driver.h
> > 
> > Regards,
> > /Bruce
> 
> The actual C++ guards are small and have no impact therefore we
> should accept patches as needed. Having more guards than needed is
> just noise.
> 
> But the responsibility of telling DPDK project where they are required
> should fall to any out-of-tree users who care.

Sorry Stephen I don't understand.
Please could you rephrase your comment?

Re: out of tree driver builds broken with C++

[Bruce Richardson]

On Mon, Feb 14, 2022 at 09:24:35AM -0800, Stephen Hemminger wrote:
> On Mon, 14 Feb 2022 15:03:43 +0000
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > On Mon, Feb 14, 2022 at 11:43:35AM +0000, Bruce Richardson wrote:
> > > On Mon, Feb 14, 2022 at 12:19:24PM +0100, Thomas Monjalon wrote:  
> > > > 14/02/2022 11:45, Bruce Richardson:  
> > > > > On Mon, Feb 14, 2022 at 10:22:08AM +0100, Thomas Monjalon wrote:  
> > > > > > 14/02/2022 10:13, Tyler Retzlaff:  
> > > > > > > while the driver api is "internal" we agreed some time ago that drivers
> > > > > > > could be built external to the dpdk tree. by enabling the meson setup
> > > > > > > option -Denable_driver_sdk=true.
> > > > > > > 
> > > > > > > it was agreed that the driver api was internal and would attract no
> > > > > > > binary compatibility support which was fine.  this change has now
> > > > > > > imposed a further restriction that out of tree drivers have to be
> > > > > > > authored in C only as non-C++ compatible code will invariably leak into
> > > > > > > the internal structures.
> > > > > > > 
> > > > > > > you won't allow us to build C++ drivers in the dpdk tree and it seems
> > > > > > > now you are preventing building of C++ drivers outside of the tree too.  
> > > > > > 
> > > > > > That's the problem of non-written assumptions, they are unknown or forgotten.
> > > > > > Did we agree to support out-of-tree drivers in C++?
> > > > > > 
> > > > > > We really need to make things clear and written in documentation.
> > > > > >   
> > > > > > > could we please re-evaluate this.  
> > > > > > 
> > > > > > Yes we can re-evaluate.
> > > > > > What is the list of impacted files?
> > > > > >   
> > > > > Hacking meson files a bit, the list of SDK header files is reported as below.
> > > > > 
> > > > > /Bruce
> > > > > 
> > > > > Message: SDK headers: 
> > > > > Message: ethdev_driver.h
> > > > > Message: ethdev_pci.h
> > > > > Message: ethdev_vdev.h
> > > > > Message: cryptodev_pmd.h
> > > > > Message: eventdev_pmd.h
> > > > > Message: eventdev_pmd_pci.h
> > > > > Message: eventdev_pmd_vdev.h
> > > > > Message: eventdev_trace.h
> > > > > Message: event_timer_adapter_pmd.h
> > > > > Message: rte_dmadev_pmd.h
> > > > > Message: vdpa_driver.h  
> > > > 
> > > > I see no harm in supporting C++ include of these headers.
> > > > Any objection?
> > > > 
> > > > Could we have a test in chkincs for the SDK headers?
> > > >   
> > > Yes. It may make things a little more complicated, though, as it seems
> > > these headers also have a tendency to rely on some driver headers -
> > > specifically bus driver headers.
> > >   
> > 
> > Working on a patch to add these to checks. However, just running a c++
> > compile does not check for valid 'extern "C"' blocks. Adding the following
> > to chkincs/meson.build as a basic sanity check throws up a larger list of
> > files to be looked at.
> > 
> > Diff:
> > 
> > +# check for extern C in files, since this is not detected as an error by the compiler
> > +grep = find_program('grep', required: false)
> > +if grep.found()
> > +    errlist = run_command([grep, '--files-without-match', '^extern "C"', dpdk_chkinc_headers],
> > +            check: true, capture: true).stdout().split()
> > +    if errlist != []
> > +        error('Files missing C++ \'extern "C"\' guards:\n- ' + '\n- '.join(errlist))
> > +    endif
> > +endif
> > +
> > 
> > Output:
> > ../buildtools/chkincs/meson.build:45:8: ERROR: Problem encountered: Files missing C++ 'extern "C"' guards:
> > - /home/bruce/dpdk.org/lib/telemetry/rte_telemetry.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_bitops.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_branch_prediction.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_compat.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_hypervisor.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_keepalive.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_pci_dev_feature_defs.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_pci_dev_features.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_time.h
> > - /home/bruce/dpdk.org/lib/eal/include/rte_trace_point_register.h
> > - /home/bruce/dpdk.org/lib/eal/linux/include/rte_os.h
> > - /home/bruce/dpdk.org/lib/ethdev/rte_dev_info.h
> > - /home/bruce/dpdk.org/lib/ethdev/ethdev_driver.h
> > - /home/bruce/dpdk.org/lib/ethdev/ethdev_pci.h
> > - /home/bruce/dpdk.org/lib/ethdev/ethdev_vdev.h
> > - /home/bruce/dpdk.org/lib/metrics/rte_metrics_telemetry.h
> > - /home/bruce/dpdk.org/lib/acl/rte_acl_osdep.h
> > - /home/bruce/dpdk.org/lib/bpf/bpf_def.h
> > - /home/bruce/dpdk.org/lib/compressdev/rte_compressdev_internal.h
> > - /home/bruce/dpdk.org/lib/cryptodev/cryptodev_pmd.h
> > - /home/bruce/dpdk.org/lib/eventdev/rte_event_ring.h
> > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd.h
> > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd_pci.h
> > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd_vdev.h
> > - /home/bruce/dpdk.org/lib/kni/rte_kni_common.h
> > - /home/bruce/dpdk.org/lib/vhost/rte_vdpa.h
> > - /home/bruce/dpdk.org/lib/vhost/rte_vhost_async.h
> > - /home/bruce/dpdk.org/lib/vhost/rte_vhost_crypto.h
> > - /home/bruce/dpdk.org/lib/vhost/vdpa_driver.h
> > 
> > Regards,
> > /Bruce
> 
> The actual C++ guards are small and have no impact therefore we
> should accept patches as needed. Having more guards than needed is
> just noise.
> 
> But the responsibility of telling DPDK project where they are required
> should fall to any out-of-tree users who care.

The guards in the above files are all needed, I believe. The scan for
chkincs only runs on header files that are public, or part of the
driver_sdk set. Internal headers aren't scanned and don't need the guards.

/Bruce

Re: out of tree driver builds broken with C++

[Stephen Hemminger]

On Mon, 14 Feb 2022 18:49:48 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:

> 14/02/2022 18:t24, Stephen Hemminger:
> > On Mon, 14 Feb 2022 15:03:43 +0000
> > Bruce Richardson <bruce.richardson@intel.com> wrote:
> >   
> > > On Mon, Feb 14, 2022 at 11:43:35AM +0000, Bruce Richardson wrote:  
> > > > On Mon, Feb 14, 2022 at 12:19:24PM +0100, Thomas Monjalon wrote:    
> > > > > 14/02/2022 11:45, Bruce Richardson:    
> > > > > > On Mon, Feb 14, 2022 at 10:22:08AM +0100, Thomas Monjalon wrote:    
> > > > > > > 14/02/2022 10:13, Tyler Retzlaff:    
> > > > > > > > while the driver api is "internal" we agreed some time ago that drivers
> > > > > > > > could be built external to the dpdk tree. by enabling the meson setup
> > > > > > > > option -Denable_driver_sdk=true.
> > > > > > > > 
> > > > > > > > it was agreed that the driver api was internal and would attract no
> > > > > > > > binary compatibility support which was fine.  this change has now
> > > > > > > > imposed a further restriction that out of tree drivers have to be
> > > > > > > > authored in C only as non-C++ compatible code will invariably leak into
> > > > > > > > the internal structures.
> > > > > > > > 
> > > > > > > > you won't allow us to build C++ drivers in the dpdk tree and it seems
> > > > > > > > now you are preventing building of C++ drivers outside of the tree too.    
> > > > > > > 
> > > > > > > That's the problem of non-written assumptions, they are unknown or forgotten.
> > > > > > > Did we agree to support out-of-tree drivers in C++?
> > > > > > > 
> > > > > > > We really need to make things clear and written in documentation.
> > > > > > >     
> > > > > > > > could we please re-evaluate this.    
> > > > > > > 
> > > > > > > Yes we can re-evaluate.
> > > > > > > What is the list of impacted files?
> > > > > > >     
> > > > > > Hacking meson files a bit, the list of SDK header files is reported as below.
> > > > > > 
> > > > > > /Bruce
> > > > > > 
> > > > > > Message: SDK headers: 
> > > > > > Message: ethdev_driver.h
> > > > > > Message: ethdev_pci.h
> > > > > > Message: ethdev_vdev.h
> > > > > > Message: cryptodev_pmd.h
> > > > > > Message: eventdev_pmd.h
> > > > > > Message: eventdev_pmd_pci.h
> > > > > > Message: eventdev_pmd_vdev.h
> > > > > > Message: eventdev_trace.h
> > > > > > Message: event_timer_adapter_pmd.h
> > > > > > Message: rte_dmadev_pmd.h
> > > > > > Message: vdpa_driver.h    
> > > > > 
> > > > > I see no harm in supporting C++ include of these headers.
> > > > > Any objection?
> > > > > 
> > > > > Could we have a test in chkincs for the SDK headers?
> > > > >     
> > > > Yes. It may make things a little more complicated, though, as it seems
> > > > these headers also have a tendency to rely on some driver headers -
> > > > specifically bus driver headers.
> > > >     
> > > 
> > > Working on a patch to add these to checks. However, just running a c++
> > > compile does not check for valid 'extern "C"' blocks. Adding the following
> > > to chkincs/meson.build as a basic sanity check throws up a larger list of
> > > files to be looked at.
> > > 
> > > Diff:
> > > 
> > > +# check for extern C in files, since this is not detected as an error by the compiler
> > > +grep = find_program('grep', required: false)
> > > +if grep.found()
> > > +    errlist = run_command([grep, '--files-without-match', '^extern "C"', dpdk_chkinc_headers],
> > > +            check: true, capture: true).stdout().split()
> > > +    if errlist != []
> > > +        error('Files missing C++ \'extern "C"\' guards:\n- ' + '\n- '.join(errlist))
> > > +    endif
> > > +endif
> > > +
> > > 
> > > Output:
> > > ../buildtools/chkincs/meson.build:45:8: ERROR: Problem encountered: Files missing C++ 'extern "C"' guards:
> > > - /home/bruce/dpdk.org/lib/telemetry/rte_telemetry.h
> > > - /home/bruce/dpdk.org/lib/eal/include/rte_bitops.h
> > > - /home/bruce/dpdk.org/lib/eal/include/rte_branch_prediction.h
> > > - /home/bruce/dpdk.org/lib/eal/include/rte_compat.h
> > > - /home/bruce/dpdk.org/lib/eal/include/rte_hypervisor.h
> > > - /home/bruce/dpdk.org/lib/eal/include/rte_keepalive.h
> > > - /home/bruce/dpdk.org/lib/eal/include/rte_pci_dev_feature_defs.h
> > > - /home/bruce/dpdk.org/lib/eal/include/rte_pci_dev_features.h
> > > - /home/bruce/dpdk.org/lib/eal/include/rte_time.h
> > > - /home/bruce/dpdk.org/lib/eal/include/rte_trace_point_register.h
> > > - /home/bruce/dpdk.org/lib/eal/linux/include/rte_os.h
> > > - /home/bruce/dpdk.org/lib/ethdev/rte_dev_info.h
> > > - /home/bruce/dpdk.org/lib/ethdev/ethdev_driver.h
> > > - /home/bruce/dpdk.org/lib/ethdev/ethdev_pci.h
> > > - /home/bruce/dpdk.org/lib/ethdev/ethdev_vdev.h
> > > - /home/bruce/dpdk.org/lib/metrics/rte_metrics_telemetry.h
> > > - /home/bruce/dpdk.org/lib/acl/rte_acl_osdep.h
> > > - /home/bruce/dpdk.org/lib/bpf/bpf_def.h
> > > - /home/bruce/dpdk.org/lib/compressdev/rte_compressdev_internal.h
> > > - /home/bruce/dpdk.org/lib/cryptodev/cryptodev_pmd.h
> > > - /home/bruce/dpdk.org/lib/eventdev/rte_event_ring.h
> > > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd.h
> > > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd_pci.h
> > > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd_vdev.h
> > > - /home/bruce/dpdk.org/lib/kni/rte_kni_common.h
> > > - /home/bruce/dpdk.org/lib/vhost/rte_vdpa.h
> > > - /home/bruce/dpdk.org/lib/vhost/rte_vhost_async.h
> > > - /home/bruce/dpdk.org/lib/vhost/rte_vhost_crypto.h
> > > - /home/bruce/dpdk.org/lib/vhost/vdpa_driver.h
> > > 
> > > Regards,
> > > /Bruce  
> > 
> > The actual C++ guards are small and have no impact therefore we
> > should accept patches as needed. Having more guards than needed is
> > just noise.
> > 
> > But the responsibility of telling DPDK project where they are required
> > should fall to any out-of-tree users who care.  
> 
> Sorry Stephen I don't understand.
> Please could you rephrase your comment?

Since there are no in-tree DPDK drivers that use C++ it is an extra
effort to add infrastructure to test if headers are broken. As new
features get added to the DPDK, it doesn't make sense for the upstream
DPDK to guarantee that no driver API changed in a way that breaks C++.
But if a breakage happens in C++ driver, the upstream DPDK is willing
to accept patches to fix it.

Re: out of tree driver builds broken with C++

[Tyler Retzlaff]

On Mon, Feb 14, 2022 at 10:21:20AM -0800, Stephen Hemminger wrote:
> On Mon, 14 Feb 2022 18:49:48 +0100
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 14/02/2022 18:t24, Stephen Hemminger:
> > > On Mon, 14 Feb 2022 15:03:43 +0000
> > > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > >   
> > > > On Mon, Feb 14, 2022 at 11:43:35AM +0000, Bruce Richardson wrote:  
> > > > > On Mon, Feb 14, 2022 at 12:19:24PM +0100, Thomas Monjalon wrote:    
> > > > > > 14/02/2022 11:45, Bruce Richardson:    
> > > > > > > On Mon, Feb 14, 2022 at 10:22:08AM +0100, Thomas Monjalon wrote:    
> > > > > > > > 14/02/2022 10:13, Tyler Retzlaff:    
> > > > > > > > > while the driver api is "internal" we agreed some time ago that drivers
> > > > > > > > > could be built external to the dpdk tree. by enabling the meson setup
> > > > > > > > > option -Denable_driver_sdk=true.
> > > > > > > > > 
> > > > > > > > > it was agreed that the driver api was internal and would attract no
> > > > > > > > > binary compatibility support which was fine.  this change has now
> > > > > > > > > imposed a further restriction that out of tree drivers have to be
> > > > > > > > > authored in C only as non-C++ compatible code will invariably leak into
> > > > > > > > > the internal structures.
> > > > > > > > > 
> > > > > > > > > you won't allow us to build C++ drivers in the dpdk tree and it seems
> > > > > > > > > now you are preventing building of C++ drivers outside of the tree too.    
> > > > > > > > 
> > > > > > > > That's the problem of non-written assumptions, they are unknown or forgotten.
> > > > > > > > Did we agree to support out-of-tree drivers in C++?
> > > > > > > > 
> > > > > > > > We really need to make things clear and written in documentation.
> > > > > > > >     
> > > > > > > > > could we please re-evaluate this.    
> > > > > > > > 
> > > > > > > > Yes we can re-evaluate.
> > > > > > > > What is the list of impacted files?
> > > > > > > >     
> > > > > > > Hacking meson files a bit, the list of SDK header files is reported as below.
> > > > > > > 
> > > > > > > /Bruce
> > > > > > > 
> > > > > > > Message: SDK headers: 
> > > > > > > Message: ethdev_driver.h
> > > > > > > Message: ethdev_pci.h
> > > > > > > Message: ethdev_vdev.h
> > > > > > > Message: cryptodev_pmd.h
> > > > > > > Message: eventdev_pmd.h
> > > > > > > Message: eventdev_pmd_pci.h
> > > > > > > Message: eventdev_pmd_vdev.h
> > > > > > > Message: eventdev_trace.h
> > > > > > > Message: event_timer_adapter_pmd.h
> > > > > > > Message: rte_dmadev_pmd.h
> > > > > > > Message: vdpa_driver.h    
> > > > > > 
> > > > > > I see no harm in supporting C++ include of these headers.
> > > > > > Any objection?
> > > > > > 
> > > > > > Could we have a test in chkincs for the SDK headers?
> > > > > >     
> > > > > Yes. It may make things a little more complicated, though, as it seems
> > > > > these headers also have a tendency to rely on some driver headers -
> > > > > specifically bus driver headers.
> > > > >     
> > > > 
> > > > Working on a patch to add these to checks. However, just running a c++
> > > > compile does not check for valid 'extern "C"' blocks. Adding the following
> > > > to chkincs/meson.build as a basic sanity check throws up a larger list of
> > > > files to be looked at.
> > > > 
> > > > Diff:
> > > > 
> > > > +# check for extern C in files, since this is not detected as an error by the compiler
> > > > +grep = find_program('grep', required: false)
> > > > +if grep.found()
> > > > +    errlist = run_command([grep, '--files-without-match', '^extern "C"', dpdk_chkinc_headers],
> > > > +            check: true, capture: true).stdout().split()
> > > > +    if errlist != []
> > > > +        error('Files missing C++ \'extern "C"\' guards:\n- ' + '\n- '.join(errlist))
> > > > +    endif
> > > > +endif
> > > > +
> > > > 
> > > > Output:
> > > > ../buildtools/chkincs/meson.build:45:8: ERROR: Problem encountered: Files missing C++ 'extern "C"' guards:
> > > > - /home/bruce/dpdk.org/lib/telemetry/rte_telemetry.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_bitops.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_branch_prediction.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_compat.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_hypervisor.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_keepalive.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_pci_dev_feature_defs.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_pci_dev_features.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_time.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_trace_point_register.h
> > > > - /home/bruce/dpdk.org/lib/eal/linux/include/rte_os.h
> > > > - /home/bruce/dpdk.org/lib/ethdev/rte_dev_info.h
> > > > - /home/bruce/dpdk.org/lib/ethdev/ethdev_driver.h
> > > > - /home/bruce/dpdk.org/lib/ethdev/ethdev_pci.h
> > > > - /home/bruce/dpdk.org/lib/ethdev/ethdev_vdev.h
> > > > - /home/bruce/dpdk.org/lib/metrics/rte_metrics_telemetry.h
> > > > - /home/bruce/dpdk.org/lib/acl/rte_acl_osdep.h
> > > > - /home/bruce/dpdk.org/lib/bpf/bpf_def.h
> > > > - /home/bruce/dpdk.org/lib/compressdev/rte_compressdev_internal.h
> > > > - /home/bruce/dpdk.org/lib/cryptodev/cryptodev_pmd.h
> > > > - /home/bruce/dpdk.org/lib/eventdev/rte_event_ring.h
> > > > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd.h
> > > > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd_pci.h
> > > > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd_vdev.h
> > > > - /home/bruce/dpdk.org/lib/kni/rte_kni_common.h
> > > > - /home/bruce/dpdk.org/lib/vhost/rte_vdpa.h
> > > > - /home/bruce/dpdk.org/lib/vhost/rte_vhost_async.h
> > > > - /home/bruce/dpdk.org/lib/vhost/rte_vhost_crypto.h
> > > > - /home/bruce/dpdk.org/lib/vhost/vdpa_driver.h
> > > > 
> > > > Regards,
> > > > /Bruce  
> > > 
> > > The actual C++ guards are small and have no impact therefore we
> > > should accept patches as needed. Having more guards than needed is
> > > just noise.
> > > 
> > > But the responsibility of telling DPDK project where they are required
> > > should fall to any out-of-tree users who care.  
> > 
> > Sorry Stephen I don't understand.
> > Please could you rephrase your comment?
> 
> Since there are no in-tree DPDK drivers that use C++ it is an extra
> effort to add infrastructure to test if headers are broken. As new
> features get added to the DPDK, it doesn't make sense for the upstream
> DPDK to guarantee that no driver API changed in a way that breaks C++.
> But if a breakage happens in C++ driver, the upstream DPDK is willing
> to accept patches to fix it.

that is completely untenable. to have a policy where the consumers using
c++ have to detect the non-compatible changes and then react by
submitting patches to fix upstream is overly burdensome.

it is far less overhead to prevent the non-compatible code creaping into
the headers in the first place rather than pushing it down to the users
of dpdk.

Re: out of tree driver builds broken with C++

[Tyler Retzlaff]

On Mon, Feb 14, 2022 at 10:32:24AM -0800, Tyler Retzlaff wrote:
> On Mon, Feb 14, 2022 at 10:21:20AM -0800, Stephen Hemminger wrote:
> > On Mon, 14 Feb 2022 18:49:48 +0100
> > Thomas Monjalon <thomas@monjalon.net> wrote:
> > 
> > > 14/02/2022 18:t24, Stephen Hemminger:
> > > > On Mon, 14 Feb 2022 15:03:43 +0000
> > > > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > > >   
> > > > > On Mon, Feb 14, 2022 at 11:43:35AM +0000, Bruce Richardson wrote:  
> > > > > > On Mon, Feb 14, 2022 at 12:19:24PM +0100, Thomas Monjalon wrote:    
> > > > > > > 14/02/2022 11:45, Bruce Richardson:    
> > > > > > > > On Mon, Feb 14, 2022 at 10:22:08AM +0100, Thomas Monjalon wrote:    
> > > > > > > > > 14/02/2022 10:13, Tyler Retzlaff:    
> > > > > > > > > > while the driver api is "internal" we agreed some time ago that drivers
> > > > > > > > > > could be built external to the dpdk tree. by enabling the meson setup
> > > > > > > > > > option -Denable_driver_sdk=true.
> > > > > > > > > > 
> > > > > > > > > > it was agreed that the driver api was internal and would attract no
> > > > > > > > > > binary compatibility support which was fine.  this change has now
> > > > > > > > > > imposed a further restriction that out of tree drivers have to be
> > > > > > > > > > authored in C only as non-C++ compatible code will invariably leak into
> > > > > > > > > > the internal structures.
> > > > > > > > > > 
> > > > > > > > > > you won't allow us to build C++ drivers in the dpdk tree and it seems
> > > > > > > > > > now you are preventing building of C++ drivers outside of the tree too.    
> > > > > > > > > 
> > > > > > > > > That's the problem of non-written assumptions, they are unknown or forgotten.
> > > > > > > > > Did we agree to support out-of-tree drivers in C++?
> > > > > > > > > 
> > > > > > > > > We really need to make things clear and written in documentation.
> > > > > > > > >     
> > > > > > > > > > could we please re-evaluate this.    
> > > > > > > > > 
> > > > > > > > > Yes we can re-evaluate.
> > > > > > > > > What is the list of impacted files?
> > > > > > > > >     
> > > > > > > > Hacking meson files a bit, the list of SDK header files is reported as below.
> > > > > > > > 
> > > > > > > > /Bruce
> > > > > > > > 
> > > > > > > > Message: SDK headers: 
> > > > > > > > Message: ethdev_driver.h
> > > > > > > > Message: ethdev_pci.h
> > > > > > > > Message: ethdev_vdev.h
> > > > > > > > Message: cryptodev_pmd.h
> > > > > > > > Message: eventdev_pmd.h
> > > > > > > > Message: eventdev_pmd_pci.h
> > > > > > > > Message: eventdev_pmd_vdev.h
> > > > > > > > Message: eventdev_trace.h
> > > > > > > > Message: event_timer_adapter_pmd.h
> > > > > > > > Message: rte_dmadev_pmd.h
> > > > > > > > Message: vdpa_driver.h    
> > > > > > > 
> > > > > > > I see no harm in supporting C++ include of these headers.
> > > > > > > Any objection?
> > > > > > > 
> > > > > > > Could we have a test in chkincs for the SDK headers?
> > > > > > >     
> > > > > > Yes. It may make things a little more complicated, though, as it seems
> > > > > > these headers also have a tendency to rely on some driver headers -
> > > > > > specifically bus driver headers.
> > > > > >     
> > > > > 
> > > > > Working on a patch to add these to checks. However, just running a c++
> > > > > compile does not check for valid 'extern "C"' blocks. Adding the following
> > > > > to chkincs/meson.build as a basic sanity check throws up a larger list of
> > > > > files to be looked at.
> > > > > 
> > > > > Diff:
> > > > > 
> > > > > +# check for extern C in files, since this is not detected as an error by the compiler
> > > > > +grep = find_program('grep', required: false)
> > > > > +if grep.found()
> > > > > +    errlist = run_command([grep, '--files-without-match', '^extern "C"', dpdk_chkinc_headers],
> > > > > +            check: true, capture: true).stdout().split()
> > > > > +    if errlist != []
> > > > > +        error('Files missing C++ \'extern "C"\' guards:\n- ' + '\n- '.join(errlist))
> > > > > +    endif
> > > > > +endif
> > > > > +
> > > > > 
> > > > > Output:
> > > > > ../buildtools/chkincs/meson.build:45:8: ERROR: Problem encountered: Files missing C++ 'extern "C"' guards:
> > > > > - /home/bruce/dpdk.org/lib/telemetry/rte_telemetry.h
> > > > > - /home/bruce/dpdk.org/lib/eal/include/rte_bitops.h
> > > > > - /home/bruce/dpdk.org/lib/eal/include/rte_branch_prediction.h
> > > > > - /home/bruce/dpdk.org/lib/eal/include/rte_compat.h
> > > > > - /home/bruce/dpdk.org/lib/eal/include/rte_hypervisor.h
> > > > > - /home/bruce/dpdk.org/lib/eal/include/rte_keepalive.h
> > > > > - /home/bruce/dpdk.org/lib/eal/include/rte_pci_dev_feature_defs.h
> > > > > - /home/bruce/dpdk.org/lib/eal/include/rte_pci_dev_features.h
> > > > > - /home/bruce/dpdk.org/lib/eal/include/rte_time.h
> > > > > - /home/bruce/dpdk.org/lib/eal/include/rte_trace_point_register.h
> > > > > - /home/bruce/dpdk.org/lib/eal/linux/include/rte_os.h
> > > > > - /home/bruce/dpdk.org/lib/ethdev/rte_dev_info.h
> > > > > - /home/bruce/dpdk.org/lib/ethdev/ethdev_driver.h
> > > > > - /home/bruce/dpdk.org/lib/ethdev/ethdev_pci.h
> > > > > - /home/bruce/dpdk.org/lib/ethdev/ethdev_vdev.h
> > > > > - /home/bruce/dpdk.org/lib/metrics/rte_metrics_telemetry.h
> > > > > - /home/bruce/dpdk.org/lib/acl/rte_acl_osdep.h
> > > > > - /home/bruce/dpdk.org/lib/bpf/bpf_def.h
> > > > > - /home/bruce/dpdk.org/lib/compressdev/rte_compressdev_internal.h
> > > > > - /home/bruce/dpdk.org/lib/cryptodev/cryptodev_pmd.h
> > > > > - /home/bruce/dpdk.org/lib/eventdev/rte_event_ring.h
> > > > > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd.h
> > > > > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd_pci.h
> > > > > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd_vdev.h
> > > > > - /home/bruce/dpdk.org/lib/kni/rte_kni_common.h
> > > > > - /home/bruce/dpdk.org/lib/vhost/rte_vdpa.h
> > > > > - /home/bruce/dpdk.org/lib/vhost/rte_vhost_async.h
> > > > > - /home/bruce/dpdk.org/lib/vhost/rte_vhost_crypto.h
> > > > > - /home/bruce/dpdk.org/lib/vhost/vdpa_driver.h
> > > > > 
> > > > > Regards,
> > > > > /Bruce  
> > > > 
> > > > The actual C++ guards are small and have no impact therefore we
> > > > should accept patches as needed. Having more guards than needed is
> > > > just noise.
> > > > 
> > > > But the responsibility of telling DPDK project where they are required
> > > > should fall to any out-of-tree users who care.  
> > > 
> > > Sorry Stephen I don't understand.
> > > Please could you rephrase your comment?
> > 
> > Since there are no in-tree DPDK drivers that use C++ it is an extra
> > effort to add infrastructure to test if headers are broken. As new
> > features get added to the DPDK, it doesn't make sense for the upstream
> > DPDK to guarantee that no driver API changed in a way that breaks C++.
> > But if a breakage happens in C++ driver, the upstream DPDK is willing
> > to accept patches to fix it.
> 
> that is completely untenable. to have a policy where the consumers using
> c++ have to detect the non-compatible changes and then react by
> submitting patches to fix upstream is overly burdensome.
> 
> it is far less overhead to prevent the non-compatible code creaping into
> the headers in the first place rather than pushing it down to the users
> of dpdk.

s/creaping/creeping/

additionally on the topic of extra effort you should consider the
perpetual submission and review of patches has non-zero overhead.

Re: out of tree driver builds broken with C++

[Tyler Retzlaff]

On Mon, Feb 14, 2022 at 10:22:08AM +0100, Thomas Monjalon wrote:
> 14/02/2022 10:13, Tyler Retzlaff:
> > while the driver api is "internal" we agreed some time ago that drivers
> > could be built external to the dpdk tree. by enabling the meson setup
> > option -Denable_driver_sdk=true.
> > 
> > it was agreed that the driver api was internal and would attract no
> > binary compatibility support which was fine.  this change has now
> > imposed a further restriction that out of tree drivers have to be
> > authored in C only as non-C++ compatible code will invariably leak into
> > the internal structures.
> > 
> > you won't allow us to build C++ drivers in the dpdk tree and it seems
> > now you are preventing building of C++ drivers outside of the tree too.
> 
> That's the problem of non-written assumptions, they are unknown or forgotten.
> Did we agree to support out-of-tree drivers in C++?
> 
> We really need to make things clear and written in documentation.

how / where would you prefer this be documented?

thanks

Re: out of tree driver builds broken with C++

[Thomas Monjalon]

15/02/2022 07:10, Tyler Retzlaff:
> On Mon, Feb 14, 2022 at 10:22:08AM +0100, Thomas Monjalon wrote:
> > 14/02/2022 10:13, Tyler Retzlaff:
> > > while the driver api is "internal" we agreed some time ago that drivers
> > > could be built external to the dpdk tree. by enabling the meson setup
> > > option -Denable_driver_sdk=true.
> > > 
> > > it was agreed that the driver api was internal and would attract no
> > > binary compatibility support which was fine.  this change has now
> > > imposed a further restriction that out of tree drivers have to be
> > > authored in C only as non-C++ compatible code will invariably leak into
> > > the internal structures.
> > > 
> > > you won't allow us to build C++ drivers in the dpdk tree and it seems
> > > now you are preventing building of C++ drivers outside of the tree too.
> > 
> > That's the problem of non-written assumptions, they are unknown or forgotten.
> > Did we agree to support out-of-tree drivers in C++?
> > 
> > We really need to make things clear and written in documentation.
> 
> how / where would you prefer this be documented?

I think we could add a general explanation about driver SDK
in doc/guides/contributing/design.rst