[v3] meson: remove unnecessary explicit link to libpcap

  libpcap is already found and registered as a dependency by meson, and
the dependency is already correctly used in librte_port. This line is
just unnecessary.

It also has the side effect of messing with the meson link line: dpdk
link will be declared twice: manually and then through pkg-config. If
you configure meson to prefer static linking over dynamic, this will
cause the build to fail on librte_port, since the pcap deps are not yet
seen by the linker.

Signed-off-by: Gabriel Ganne <gabriel.ganne@6wind.com>
---
 config/meson.build | 1 -
 1 file changed, 1 deletion(-)
  

26/03/2021 09:22, Gabriel Ganne:
> libpcap is already found and registered as a dependency by meson, and
> the dependency is already correctly used in librte_port. This line is
> just unnecessary.
> 
> It also has the side effect of messing with the meson link line: dpdk
> link will be declared twice: manually and then through pkg-config. If
> you configure meson to prefer static linking over dynamic, this will
> cause the build to fail on librte_port, since the pcap deps are not yet
> seen by the linker.
> 
> Signed-off-by: Gabriel Ganne <gabriel.ganne@6wind.com>

When sending a new revision of a patch,
please add a changelog below "---"
and use --in-reply-to for threading, thanks.

> ---
>  config/meson.build | 1 -
>  1 file changed, 1 deletion(-)
  

2021-03-26 09:22 (UTC+0100), Gabriel Ganne:
> libpcap is already found and registered as a dependency by meson, and
> the dependency is already correctly used in librte_port. This line is
> just unnecessary.
> 
> It also has the side effect of messing with the meson link line: dpdk
> link will be declared twice: manually and then through pkg-config. If
> you configure meson to prefer static linking over dynamic, this will
> cause the build to fail on librte_port, since the pcap deps are not yet
> seen by the linker.
> 
> Signed-off-by: Gabriel Ganne <gabriel.ganne@6wind.com>
> ---
>  config/meson.build | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/config/meson.build b/config/meson.build
> index 66a2edcc47f5..95777cf33169 100644
> --- a/config/meson.build
> +++ b/config/meson.build
> @@ -183,7 +183,6 @@ if not pcap_dep.found()
>  endif
>  if pcap_dep.found() and cc.has_header('pcap.h', dependencies: pcap_dep)
>  	dpdk_conf.set('RTE_PORT_PCAP', 1)
> -	dpdk_extra_ldflags += '-lpcap'
>  endif
>  
>  # for clang 32-bit compiles we need libatomic for 64-bit atomic ops

This patch also simplifies future changes to discover libpcap on Windows.

Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
  

Thank you, the fix looks good.
I would like to improve the explanation.

If I understand the issue, the title should be
"build: remove redundant libpcap link"

26/03/2021 09:22, Gabriel Ganne:
> libpcap is already found and registered as a dependency by meson, and
> the dependency is already correctly used in librte_port. This line is
> just unnecessary.

To be precise, the pcap PMD and the librte_port both declare their
dependency to libpcap with a line "ext_deps += pcap_dep"
and meson automatically adds this dependency to the pkg-config file
in the private section for static builds.
That's why it is not needed to add the dependency explicitly
in dpdk_extra_ldflags (involved in static build with pkg-config).

> It also has the side effect of messing with the meson link line: dpdk

Which "link line"?

> link will be declared twice: manually and then through pkg-config. If

What is "manually"?

> you configure meson to prefer static linking over dynamic, this will

No need to configure meson for that. Static linking can be tested with
make in an example. Please avoid messing with meson explanation
for application linking, it is already complicate enough :)

> cause the build to fail on librte_port, since the pcap deps are not yet
> seen by the linker.

Sorry it is not clear to me.
I think it would help to see a before/after effect on the link command.
Something like:

before:
	Libs.private: -lpcap
	Requires.private: libpcap
after:
	Libs.private:
	Requires.private: libpcap

[...]
> -	dpdk_extra_ldflags += '-lpcap'
  

Hi Thomas,

Thanks for the review.

The use case that made me see this is that I configured the dpdk using meson
option "-- default_library=static" in conjunction with buildroot which is
patching
meson to prefer static libraries in this case.
This causes the link line generated from the dependency() directive to
result
in an expanded "/path/to/libpcap.a".
The dpdk_extra_ldflags += '-lpcap' is a direct (manual) addition to the
link line
that cannot be changed in the same way.

In this specific setup, the change is the following:
Before:
  -lpcap /path/to/libpcap.a
After:
  /path/to/libpcap.a

I admit this is quite the corner case (and probably not a great idea).

I will replace that confusing second paragraph with the comment you made
regarding " ext_deps += pcap_dep".

Best regards,

On Fri, Apr 9, 2021 at 12:39 AM Thomas Monjalon <thomas@monjalon.net> wrote:

> Thank you, the fix looks good.
> I would like to improve the explanation.
>
> If I understand the issue, the title should be
> "build: remove redundant libpcap link"
>
> 26/03/2021 09:22, Gabriel Ganne:
> > libpcap is already found and registered as a dependency by meson, and
> > the dependency is already correctly used in librte_port. This line is
> > just unnecessary.
>
> To be precise, the pcap PMD and the librte_port both declare their
> dependency to libpcap with a line "ext_deps += pcap_dep"
> and meson automatically adds this dependency to the pkg-config file
> in the private section for static builds.
> That's why it is not needed to add the dependency explicitly
> in dpdk_extra_ldflags (involved in static build with pkg-config).
>
> > It also has the side effect of messing with the meson link line: dpdk
>
> Which "link line"?
>
> > link will be declared twice: manually and then through pkg-config. If
>
> What is "manually"?
>
> > you configure meson to prefer static linking over dynamic, this will
>
> No need to configure meson for that. Static linking can be tested with
> make in an example. Please avoid messing with meson explanation
> for application linking, it is already complicate enough :)
>
> > cause the build to fail on librte_port, since the pcap deps are not yet
> > seen by the linker.
>
> Sorry it is not clear to me.
> I think it would help to see a before/after effect on the link command.
> Something like:
>
> before:
>         Libs.private: -lpcap
>         Requires.private: libpcap
> after:
>         Libs.private:
>         Requires.private: libpcap
>
> [...]
> > -     dpdk_extra_ldflags += '-lpcap'
>
>
>
>
  

09/04/2021 10:31, Gabriel Ganne:
> Hi Thomas,
> 
> Thanks for the review.
> 
> The use case that made me see this is that I configured the dpdk using meson
> option "-- default_library=static" in conjunction with buildroot which is
> patching
> meson to prefer static libraries in this case.
> This causes the link line generated from the dependency() directive to
> result
> in an expanded "/path/to/libpcap.a".
> The dpdk_extra_ldflags += '-lpcap' is a direct (manual) addition to the
> link line
> that cannot be changed in the same way.

No it is only changing the generated pkg-config file.
I think you are messing between what happens in DPDK build,
and what your specific environment/application does.

> In this specific setup, the change is the following:
> Before:
>   -lpcap /path/to/libpcap.a
> After:
>   /path/to/libpcap.a

The real before/after is in the pkg-config file as I showed below.

> I admit this is quite the corner case (and probably not a great idea).
> 
> I will replace that confusing second paragraph with the comment you made
> regarding " ext_deps += pcap_dep".

You missed my other comments.
I will propose a v5 with my explanations.

PS: please avoid top-post.


> On Fri, Apr 9, 2021 at 12:39 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> > Thank you, the fix looks good.
> > I would like to improve the explanation.
> >
> > If I understand the issue, the title should be
> > "build: remove redundant libpcap link"
> >
> > 26/03/2021 09:22, Gabriel Ganne:
> > > libpcap is already found and registered as a dependency by meson, and
> > > the dependency is already correctly used in librte_port. This line is
> > > just unnecessary.
> >
> > To be precise, the pcap PMD and the librte_port both declare their
> > dependency to libpcap with a line "ext_deps += pcap_dep"
> > and meson automatically adds this dependency to the pkg-config file
> > in the private section for static builds.
> > That's why it is not needed to add the dependency explicitly
> > in dpdk_extra_ldflags (involved in static build with pkg-config).
> >
> > > It also has the side effect of messing with the meson link line: dpdk
> >
> > Which "link line"?
> >
> > > link will be declared twice: manually and then through pkg-config. If
> >
> > What is "manually"?
> >
> > > you configure meson to prefer static linking over dynamic, this will
> >
> > No need to configure meson for that. Static linking can be tested with
> > make in an example. Please avoid messing with meson explanation
> > for application linking, it is already complicate enough :)
> >
> > > cause the build to fail on librte_port, since the pcap deps are not yet
> > > seen by the linker.
> >
> > Sorry it is not clear to me.
> > I think it would help to see a before/after effect on the link command.
> > Something like:
> >
> > before:
> >         Libs.private: -lpcap
> >         Requires.private: libpcap
> > after:
> >         Libs.private:
> >         Requires.private: libpcap
> >
> > [...]
> > > -     dpdk_extra_ldflags += '-lpcap'