[v2,0/4] add static ibverbs in meson

  This is the follow-up of the feature I added one year ago:
static linkage of libibverbs in mlx PMDs.
The first implementation was focused on "make".
This second round does the same with "meson".


changes in v2:
- split mlx patch for normal addition and workarounds
- fix ldflags for ibverbs installed in a standard directory
- fix libs order leading to undefined references
- add doc for hidden_deps
- improve explanations in commit logs


Thomas Monjalon (4):
  net/mlx: add static ibverbs linkage with meson
  buildtools: get static mlx dependencies for meson
  build: allow hiding dependencies from pkg-config
  net/mlx: workaround static linkage with meson


 buildtools/meson.build                   |  2 ++
 buildtools/options-ibverbs-static.sh     | 10 ++++++++--
 doc/guides/contributing/coding_style.rst |  6 ++++++
 doc/guides/nics/mlx4.rst                 |  4 ++++
 doc/guides/nics/mlx5.rst                 |  4 ++++
 drivers/meson.build                      |  9 ++++++---
 drivers/net/mlx4/meson.build             | 19 +++++++++++++++----
 drivers/net/mlx5/meson.build             | 19 +++++++++++++++----
 meson_options.txt                        |  4 ++--
 9 files changed, 62 insertions(+), 15 deletions(-)
  

On Mon, Jan 27, 2020 at 04:43:58PM +0100, Thomas Monjalon wrote:
> This is the follow-up of the feature I added one year ago:
> static linkage of libibverbs in mlx PMDs.
> The first implementation was focused on "make".
> This second round does the same with "meson".
> 
> 
> changes in v2:
> - split mlx patch for normal addition and workarounds
> - fix ldflags for ibverbs installed in a standard directory
> - fix libs order leading to undefined references
> - add doc for hidden_deps
> - improve explanations in commit logs
> 
> 
> Thomas Monjalon (4):
>   net/mlx: add static ibverbs linkage with meson
>   buildtools: get static mlx dependencies for meson
>   build: allow hiding dependencies from pkg-config
>   net/mlx: workaround static linkage with meson
> 
> 
Hi Thomas,

as we discussed offline, I really don't like the necessity of the
hidden_deps part of this, so I've tried coming up with some other
solutions. For example, in my testing I get the same result with the
following diff instead of the second two patches (just showing for mlx5 -
changes for mlx4 are identical):

--- a/drivers/net/mlx5/meson.build
+++ b/drivers/net/mlx5/meson.build
@@ -31,10 +31,7 @@ foreach libname:libnames
        endif
        if lib.found()
                libs += lib
-               if static_ibverbs
-                       # Build without adding shared libs to Requires.private
-                       hidden_deps += lib.partial_dependency(compile_args:true)
-               else
+               if not static_ibverbs
                        ext_deps += lib
                endif
        else
@@ -44,8 +41,10 @@ foreach libname:libnames
 endforeach
 if build and static_ibverbs
        # Add static deps ldflags to internal apps and Libs.private
-       ldflags = run_command(ldflags_ibverbs_static, check:true).stdout()
-       ext_deps += declare_dependency(link_args:ldflags.split())
+       ibv_ldflags = run_command(ldflags_ibverbs_static, check:true).stdout()
+       ibv_cflags = run_command(find_program('pkg-config'), '--cflags', 'libibverbs').stdout()
+       ext_deps += declare_dependency(compile_args: ibv_cflags.split(),
+                               link_args:ibv_ldflags.split())
 endif
 
 if build


By doing things this way - assuming it works in your tests too - we avoid
any need to change anything in the common drivers code.

How I tested this was by:
* doing a local rdma-core build as described in the docs and exporting
  PKG_CONFIG_PATH to point to the .pc file
* doing builds with all 4 of your patches applied (one static linking of
  apps, one shared, just in case)
* doing builds with the first two patches and the above diff.
* compare the meson.build files and libdpdk.pc files for the two cases. In
  my tests the second case as additional -I flags, but otherwise identical.

Regards,
/Bruce
  

04/02/2020 12:48, Bruce Richardson:
> as we discussed offline, I really don't like the necessity of the
> hidden_deps part of this, so I've tried coming up with some other
> solutions.

Thanks for looking closely at these patches.

> For example, in my testing I get the same result with the
> following diff instead of the second two patches (just showing for mlx5 -
> changes for mlx4 are identical):
[...]
> -                       # Build without adding shared libs to Requires.private
> -                       hidden_deps += lib.partial_dependency(compile_args:true)
[...]
> +       ibv_cflags = run_command(find_program('pkg-config'), '--cflags', 'libibverbs').stdout()
[...]
> By doing things this way - assuming it works in your tests too - we avoid
> any need to change anything in the common drivers code.

Yes, you hide the dependency by getting cflags directly with pkg-config.
I wanted to avoid such solution because I was trying to use as much
as possible the meson infrastructure.

I think your solution relying on one more call to pkg-config is acceptable.
I will test it and will review whether we get all corner cases.

In the meantime I discovered we are overlinking with meson
when using the dlopen linking option.
I will try to fix it as well with the same method.

Thanks
  

On Tue, Feb 04, 2020 at 03:27:50PM +0100, Thomas Monjalon wrote:
> 04/02/2020 12:48, Bruce Richardson:
> > as we discussed offline, I really don't like the necessity of the
> > hidden_deps part of this, so I've tried coming up with some other
> > solutions.
> 
> Thanks for looking closely at these patches.
> 
> > For example, in my testing I get the same result with the
> > following diff instead of the second two patches (just showing for mlx5 -
> > changes for mlx4 are identical):
> [...]
> > -                       # Build without adding shared libs to Requires.private
> > -                       hidden_deps += lib.partial_dependency(compile_args:true)
> [...]
> > +       ibv_cflags = run_command(find_program('pkg-config'), '--cflags', 'libibverbs').stdout()
> [...]
> > By doing things this way - assuming it works in your tests too - we avoid
> > any need to change anything in the common drivers code.
> 
> Yes, you hide the dependency by getting cflags directly with pkg-config.
> I wanted to avoid such solution because I was trying to use as much
> as possible the meson infrastructure.
> 
> I think your solution relying on one more call to pkg-config is acceptable.
> I will test it and will review whether we get all corner cases.
> 

Thanks.
It's not really ideal, but this is likely always going to be a bit flakey
since we can't use distro-supplied .a files, and your scripting is needed
to prevent even accidentally taking a non-custom-build rdma-core file.
Furthermore, I see that while meson tracks PKG_CONFIG_PATH value itself for
finding things, this does not get tracked between runs for shell calls.
This can catch one out, for example:

PKG_CONFIG_PATH=/path/to/pc/files meson build

will work correctly for everything. However, if one does a reconfigure
subsequently doing e.g. ninja reconfigure, meson will correctly pick up the
.pc files, but the ibverbs-static script, or any run_command calls to
pkg-config won't as it's not actually in the environment :-(

> In the meantime I discovered we are overlinking with meson
> when using the dlopen linking option.
> I will try to fix it as well with the same method.
> 
Right. Overlinking is probably less serious an issue though. Does it cause
any real-world problems?

/Bruce
  

04/02/2020 15:33, Bruce Richardson:
> On Tue, Feb 04, 2020 at 03:27:50PM +0100, Thomas Monjalon wrote:
> > 04/02/2020 12:48, Bruce Richardson:
> > > as we discussed offline, I really don't like the necessity of the
> > > hidden_deps part of this, so I've tried coming up with some other
> > > solutions.
> > 
> > Thanks for looking closely at these patches.
> > 
> > > For example, in my testing I get the same result with the
> > > following diff instead of the second two patches (just showing for mlx5 -
> > > changes for mlx4 are identical):
> > [...]
> > > -                       # Build without adding shared libs to Requires.private
> > > -                       hidden_deps += lib.partial_dependency(compile_args:true)
> > [...]
> > > +       ibv_cflags = run_command(find_program('pkg-config'), '--cflags', 'libibverbs').stdout()
> > [...]
> > > By doing things this way - assuming it works in your tests too - we avoid
> > > any need to change anything in the common drivers code.
> > 
> > Yes, you hide the dependency by getting cflags directly with pkg-config.
> > I wanted to avoid such solution because I was trying to use as much
> > as possible the meson infrastructure.
> > 
> > I think your solution relying on one more call to pkg-config is acceptable.
> > I will test it and will review whether we get all corner cases.
> > 
> 
> Thanks.
> It's not really ideal, but this is likely always going to be a bit flakey
> since we can't use distro-supplied .a files, and your scripting is needed
> to prevent even accidentally taking a non-custom-build rdma-core file.
> Furthermore, I see that while meson tracks PKG_CONFIG_PATH value itself for
> finding things, this does not get tracked between runs for shell calls.
> This can catch one out, for example:
> 
> PKG_CONFIG_PATH=/path/to/pc/files meson build
> 
> will work correctly for everything. However, if one does a reconfigure
> subsequently doing e.g. ninja reconfigure, meson will correctly pick up the
> .pc files, but the ibverbs-static script, or any run_command calls to
> pkg-config won't as it's not actually in the environment :-(

In my setup, I export PKG_CONFIG_PATH, so it is not an issue.
But I understand your point that we may hit the issue.
That's why I will work in meson upstream to avoid all of this in future.


> > In the meantime I discovered we are overlinking with meson
> > when using the dlopen linking option.
> > I will try to fix it as well with the same method.
> > 
> Right. Overlinking is probably less serious an issue though. Does it cause
> any real-world problems?

Overlinking defeats the benefit of dlopen.
The idea of dlopen is to avoid having ibverbs as mandatory DPDK dependency.
The ibverbs lib is loaded with dlopen only if probing a Mellanox device.
The dlopen solution makes the PMD really like an optional plugin,
same as for static linkage in the PMD.
  

On Tue, Feb 04, 2020 at 04:14:46PM +0100, Thomas Monjalon wrote:
> 04/02/2020 15:33, Bruce Richardson:
> > On Tue, Feb 04, 2020 at 03:27:50PM +0100, Thomas Monjalon wrote:
> > > 04/02/2020 12:48, Bruce Richardson:
> > > > as we discussed offline, I really don't like the necessity of the
> > > > hidden_deps part of this, so I've tried coming up with some other
> > > > solutions.
> > > 
> > > Thanks for looking closely at these patches.
> > > 
> > > > For example, in my testing I get the same result with the
> > > > following diff instead of the second two patches (just showing for mlx5 -
> > > > changes for mlx4 are identical):
> > > [...]
> > > > -                       # Build without adding shared libs to Requires.private
> > > > -                       hidden_deps += lib.partial_dependency(compile_args:true)
> > > [...]
> > > > +       ibv_cflags = run_command(find_program('pkg-config'), '--cflags', 'libibverbs').stdout()
> > > [...]
> > > > By doing things this way - assuming it works in your tests too - we avoid
> > > > any need to change anything in the common drivers code.
> > > 
> > > Yes, you hide the dependency by getting cflags directly with pkg-config.
> > > I wanted to avoid such solution because I was trying to use as much
> > > as possible the meson infrastructure.
> > > 
> > > I think your solution relying on one more call to pkg-config is acceptable.
> > > I will test it and will review whether we get all corner cases.
> > > 
> > 
> > Thanks.
> > It's not really ideal, but this is likely always going to be a bit flakey
> > since we can't use distro-supplied .a files, and your scripting is needed
> > to prevent even accidentally taking a non-custom-build rdma-core file.
> > Furthermore, I see that while meson tracks PKG_CONFIG_PATH value itself for
> > finding things, this does not get tracked between runs for shell calls.
> > This can catch one out, for example:
> > 
> > PKG_CONFIG_PATH=/path/to/pc/files meson build
> > 
> > will work correctly for everything. However, if one does a reconfigure
> > subsequently doing e.g. ninja reconfigure, meson will correctly pick up the
> > .pc files, but the ibverbs-static script, or any run_command calls to
> > pkg-config won't as it's not actually in the environment :-(
> 
> In my setup, I export PKG_CONFIG_PATH, so it is not an issue.
> But I understand your point that we may hit the issue.
> That's why I will work in meson upstream to avoid all of this in future.
> 
+1

> 
> > > In the meantime I discovered we are overlinking with meson
> > > when using the dlopen linking option.
> > > I will try to fix it as well with the same method.
> > > 
> > Right. Overlinking is probably less serious an issue though. Does it cause
> > any real-world problems?
> 
> Overlinking defeats the benefit of dlopen.
> The idea of dlopen is to avoid having ibverbs as mandatory DPDK dependency.
> The ibverbs lib is loaded with dlopen only if probing a Mellanox device.
> The dlopen solution makes the PMD really like an optional plugin,
> same as for static linkage in the PMD.
> 
Right, I understand the issue now.