[v5,1/1] build: add libarchive to external deps

Message ID 20231106041243.13621-1-syalavarthi@marvell.com (mailing list archive)
State Accepted, archived
Delegated to: David Marchand
Headers
Series [v5,1/1] build: add libarchive to external deps |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/intel-Functional success Functional PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS

Commit Message

Srikanth Yalavarthi Nov. 6, 2023, 4:12 a.m. UTC
  In order to avoid linking with Libs.private, libarchive
is not added to ext_deps during the meson setup stage.

Since libarchive is not added to ext_deps, cross-compilation
or native compilation with libarchive installed in non-standard
location fails with errors related to "cannot find -larchive"
or "archive.h: No such file or directory". In order to fix the
build failures, user is required to define the 'c_args' and
'c_link_args' with '-I<includedir>' and '-L<libdir>'.

This patch adds libarchive to ext_deps and further would not
require setting c_args and c_link_args externally.

Fixes: 40edb9c0d36b ("eal: handle compressed firmware")
Cc: stable@dpdk.org

Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
---
v5:
  - Updated as per review comments
v4:
  - Rebase over latest main
v3:
  - Add to libarchive ext_deps
v2:
  - Update ml/cnxk meson config
v1:
  - Initial patch

 config/meson.build  | 5 -----
 lib/eal/meson.build | 3 +++
 2 files changed, 3 insertions(+), 5 deletions(-)
  

Comments

Bruce Richardson Nov. 6, 2023, 8:53 a.m. UTC | #1
On Sun, Nov 05, 2023 at 08:12:43PM -0800, Srikanth Yalavarthi wrote:
> In order to avoid linking with Libs.private, libarchive
> is not added to ext_deps during the meson setup stage.
> 
> Since libarchive is not added to ext_deps, cross-compilation
> or native compilation with libarchive installed in non-standard
> location fails with errors related to "cannot find -larchive"
> or "archive.h: No such file or directory". In order to fix the
> build failures, user is required to define the 'c_args' and
> 'c_link_args' with '-I<includedir>' and '-L<libdir>'.
> 
> This patch adds libarchive to ext_deps and further would not
> require setting c_args and c_link_args externally.
> 
> Fixes: 40edb9c0d36b ("eal: handle compressed firmware")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>

Acked-by: Bruce Richardson <bruce.richardson@intel.com>
  
Thomas Monjalon Nov. 6, 2023, 3:24 p.m. UTC | #2
06/11/2023 09:53, Bruce Richardson:
> On Sun, Nov 05, 2023 at 08:12:43PM -0800, Srikanth Yalavarthi wrote:
> > In order to avoid linking with Libs.private, libarchive
> > is not added to ext_deps during the meson setup stage.
> > 
> > Since libarchive is not added to ext_deps, cross-compilation
> > or native compilation with libarchive installed in non-standard
> > location fails with errors related to "cannot find -larchive"
> > or "archive.h: No such file or directory". In order to fix the
> > build failures, user is required to define the 'c_args' and
> > 'c_link_args' with '-I<includedir>' and '-L<libdir>'.
> > 
> > This patch adds libarchive to ext_deps and further would not
> > require setting c_args and c_link_args externally.
> > 
> > Fixes: 40edb9c0d36b ("eal: handle compressed firmware")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

I'm not sure to understand which new failure will happen.
Was there a problem solved in new libarchive packages?
  
Thomas Monjalon Nov. 6, 2023, 3:32 p.m. UTC | #3
06/11/2023 16:24, Thomas Monjalon:
> 06/11/2023 09:53, Bruce Richardson:
> > On Sun, Nov 05, 2023 at 08:12:43PM -0800, Srikanth Yalavarthi wrote:
> > > In order to avoid linking with Libs.private, libarchive
> > > is not added to ext_deps during the meson setup stage.
> > > 
> > > Since libarchive is not added to ext_deps, cross-compilation
> > > or native compilation with libarchive installed in non-standard
> > > location fails with errors related to "cannot find -larchive"
> > > or "archive.h: No such file or directory". In order to fix the
> > > build failures, user is required to define the 'c_args' and
> > > 'c_link_args' with '-I<includedir>' and '-L<libdir>'.
> > > 
> > > This patch adds libarchive to ext_deps and further would not
> > > require setting c_args and c_link_args externally.
> > > 
> > > Fixes: 40edb9c0d36b ("eal: handle compressed firmware")
> > > Cc: stable@dpdk.org
> > > 
> > > Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
> > 
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> I'm not sure to understand which new failure will happen.
> Was there a problem solved in new libarchive packages?

BTW applied as it fixes an obvious problem.
  
David Marchand Nov. 6, 2023, 4:03 p.m. UTC | #4
On Mon, Nov 6, 2023 at 5:12 AM Srikanth Yalavarthi
<syalavarthi@marvell.com> wrote:
>
> In order to avoid linking with Libs.private, libarchive
> is not added to ext_deps during the meson setup stage.
>
> Since libarchive is not added to ext_deps, cross-compilation
> or native compilation with libarchive installed in non-standard
> location fails with errors related to "cannot find -larchive"
> or "archive.h: No such file or directory". In order to fix the
> build failures, user is required to define the 'c_args' and
> 'c_link_args' with '-I<includedir>' and '-L<libdir>'.
>
> This patch adds libarchive to ext_deps and further would not
> require setting c_args and c_link_args externally.
>
> Fixes: 40edb9c0d36b ("eal: handle compressed firmware")
> Cc: stable@dpdk.org
>
> Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>

This breaks static compilation of applications.
This can be reproduced with test-meson-builds.sh and in GHA (which was
not linking examples statically, I added a patch in my github repo):
https://github.com/david-marchand/dpdk/actions/runs/6772879600/job/18406442129#step:19:19572
  
Bruce Richardson Nov. 6, 2023, 4:23 p.m. UTC | #5
On Mon, Nov 06, 2023 at 05:03:10PM +0100, David Marchand wrote:
> On Mon, Nov 6, 2023 at 5:12 AM Srikanth Yalavarthi
> <syalavarthi@marvell.com> wrote:
> >
> > In order to avoid linking with Libs.private, libarchive is not added to
> > ext_deps during the meson setup stage.
> >
> > Since libarchive is not added to ext_deps, cross-compilation or native
> > compilation with libarchive installed in non-standard location fails
> > with errors related to "cannot find -larchive" or "archive.h: No such
> > file or directory". In order to fix the build failures, user is
> > required to define the 'c_args' and 'c_link_args' with '-I<includedir>'
> > and '-L<libdir>'.
> >
> > This patch adds libarchive to ext_deps and further would not require
> > setting c_args and c_link_args externally.
> >
> > Fixes: 40edb9c0d36b ("eal: handle compressed firmware") Cc:
> > stable@dpdk.org
> >
> > Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
> 
> This breaks static compilation of applications.  This can be reproduced
> with test-meson-builds.sh and in GHA (which was not linking examples
> statically, I added a patch in my github repo):
> https://github.com/david-marchand/dpdk/actions/runs/6772879600/job/18406442129#step:19:19572
> 
The libarchive-dev Ubuntu package does not install all its needed
dependencies for static linking. The errors can be resolved by manually
installing the 3 missing -dev packages.

It's less than ideal, but to my mind, DPDK is behaving correctly with this
fix - it is marking that it requires libarchive as a dependency. The fact
that the libarchive.pc file lists static libraries that aren't installed is
outside of our control. The previous implementation hacked around this by
just passing -larchive in all cases, rather than using pkg-config
information. This then caused other issues that the patch submitter hit.

/Bruce
  
David Marchand Nov. 6, 2023, 6:17 p.m. UTC | #6
On Mon, Nov 6, 2023 at 5:24 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Mon, Nov 06, 2023 at 05:03:10PM +0100, David Marchand wrote:
> > On Mon, Nov 6, 2023 at 5:12 AM Srikanth Yalavarthi
> > <syalavarthi@marvell.com> wrote:
> > >
> > > In order to avoid linking with Libs.private, libarchive is not added to
> > > ext_deps during the meson setup stage.
> > >
> > > Since libarchive is not added to ext_deps, cross-compilation or native
> > > compilation with libarchive installed in non-standard location fails
> > > with errors related to "cannot find -larchive" or "archive.h: No such
> > > file or directory". In order to fix the build failures, user is
> > > required to define the 'c_args' and 'c_link_args' with '-I<includedir>'
> > > and '-L<libdir>'.
> > >
> > > This patch adds libarchive to ext_deps and further would not require
> > > setting c_args and c_link_args externally.
> > >
> > > Fixes: 40edb9c0d36b ("eal: handle compressed firmware") Cc:
> > > stable@dpdk.org
> > >
> > > Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
> >
> > This breaks static compilation of applications.  This can be reproduced
> > with test-meson-builds.sh and in GHA (which was not linking examples
> > statically, I added a patch in my github repo):
> > https://github.com/david-marchand/dpdk/actions/runs/6772879600/job/18406442129#step:19:19572
> >
> The libarchive-dev Ubuntu package does not install all its needed
> dependencies for static linking. The errors can be resolved by manually
> installing the 3 missing -dev packages.

Same with fedora.

>
> It's less than ideal, but to my mind, DPDK is behaving correctly with this
> fix - it is marking that it requires libarchive as a dependency. The fact
> that the libarchive.pc file lists static libraries that aren't installed is
> outside of our control. The previous implementation hacked around this by
> just passing -larchive in all cases, rather than using pkg-config
> information. This then caused other issues that the patch submitter hit.

Ok, I'll see how to workaround this on my side.
  

Patch

diff --git a/config/meson.build b/config/meson.build
index 0968351740..250833d0a4 100644
--- a/config/meson.build
+++ b/config/meson.build
@@ -241,11 +241,6 @@  dpdk_conf.set('RTE_BACKTRACE', cc.has_header('execinfo.h') or is_windows)
 libarchive = dependency('libarchive', required: false, method: 'pkg-config')
 if libarchive.found()
     dpdk_conf.set('RTE_HAS_LIBARCHIVE', 1)
-    # Push libarchive link dependency at the project level to support
-    # statically linking dpdk apps. Details at:
-    # https://inbox.dpdk.org/dev/20210605004024.660267a1@sovereign/
-    add_project_link_arguments('-larchive', language: 'c')
-    dpdk_extra_ldflags += '-larchive'
 endif
 
 # check for libbsd
diff --git a/lib/eal/meson.build b/lib/eal/meson.build
index 9942104386..e1d6c4cf17 100644
--- a/lib/eal/meson.build
+++ b/lib/eal/meson.build
@@ -21,6 +21,9 @@  endif
 if dpdk_conf.has('RTE_USE_LIBBSD')
     ext_deps += libbsd
 endif
+if dpdk_conf.has('RTE_HAS_LIBARCHIVE')
+    ext_deps += libarchive
+endif
 if cc.has_function('getentropy', prefix : '#include <unistd.h>')
     cflags += '-DRTE_LIBEAL_USE_GETENTROPY'
 endif