diff mbox series

buildtools: fix build with meson 0.60

Message ID 20211026193239.113745-1-dmitry.kozliuk@gmail.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series buildtools: fix build with meson 0.60 | expand

Checks

Context Check Description
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance fail Performance Testing issues
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/intel-Testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/github-robot: build success github build: passed
ci/checkpatch success coding style OK

Commit Message

Dmitry Kozlyuk Oct. 26, 2021, 7:32 p.m. UTC
Meson 0.60 switched the format of uninstalled static libraries
to thin archives, that is, they contain only paths to object files,
not the files themselves. Files cannot be extracted in this case,
resulting in build errors:

    ar: `x' cannot be used on thin archives.

Handle thin archives when invoking pmdinfogen
by directly using the files referenced in the archive.

Bugzilla ID: 836
Fixes: e6e9730c7066 ("buildtools: support object file extraction for Windows")
Cc: stable@dpdk.org

Reported-by: Michal Berger <michallinuxstuff@gmail.com>
Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 buildtools/gen-pmdinfo-cfile.py | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

Comments

Bruce Richardson Oct. 27, 2021, 10:53 a.m. UTC | #1
On Tue, Oct 26, 2021 at 10:32:39PM +0300, Dmitry Kozlyuk wrote:
> Meson 0.60 switched the format of uninstalled static libraries
> to thin archives, that is, they contain only paths to object files,
> not the files themselves. Files cannot be extracted in this case,
> resulting in build errors:
> 
>     ar: `x' cannot be used on thin archives.
> 
> Handle thin archives when invoking pmdinfogen
> by directly using the files referenced in the archive.
> 
> Bugzilla ID: 836
> Fixes: e6e9730c7066 ("buildtools: support object file extraction for Windows")
> Cc: stable@dpdk.org
> 
> Reported-by: Michal Berger <michallinuxstuff@gmail.com>
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

Thanks for looking into this and proposing a fix. Some thoughts inline
below.

/Bruce

> ---
>  buildtools/gen-pmdinfo-cfile.py | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/buildtools/gen-pmdinfo-cfile.py b/buildtools/gen-pmdinfo-cfile.py
> index 58fe3ad152..3453e5b4b9 100644
> --- a/buildtools/gen-pmdinfo-cfile.py
> +++ b/buildtools/gen-pmdinfo-cfile.py
> @@ -8,13 +8,18 @@
>  import tempfile
>  
>  _, tmp_root, ar, archive, output, *pmdinfogen = sys.argv
> -with tempfile.TemporaryDirectory(dir=tmp_root) as temp:
> -    run_ar = lambda command: subprocess.run(
> -        [ar, command, os.path.abspath(archive)],
> -        stdout=subprocess.PIPE, check=True, cwd=temp
> -    )
> -    # Don't use "ar p", because its output is corrupted on Windows.
> -    run_ar("x")
> -    names = run_ar("t").stdout.decode().splitlines()
> -    paths = [os.path.join(temp, name) for name in names]
> +archive = os.path.abspath(archive)
> +names = subprocess.run([ar, "t", archive],
> +    stdout=subprocess.PIPE, check=True).stdout.decode().splitlines()
> +with open(archive, "rb") as f:
> +    is_thin = f.read(7) == b"!<thin>"

This part seems overly low-level, and a bit nasty to me, compared to the
rest of the script.

Since the thin archive files already exist, I wonder if we could work on a
file by file basis here, and ignore generally the type of the ".a" file.
For example, something like:

    o_files = []
    for n in names:
        if os.path.exists(n):
            o_files += n
        else
            <extract file>
            o_files += <extracted_path>

WDYT?
Bruce Richardson Oct. 27, 2021, 4:16 p.m. UTC | #2
On Tue, Oct 26, 2021 at 10:32:39PM +0300, Dmitry Kozlyuk wrote:
> Meson 0.60 switched the format of uninstalled static libraries
> to thin archives, that is, they contain only paths to object files,
> not the files themselves. Files cannot be extracted in this case,
> resulting in build errors:
> 
>     ar: `x' cannot be used on thin archives.
> 
> Handle thin archives when invoking pmdinfogen
> by directly using the files referenced in the archive.
> 
> Bugzilla ID: 836
> Fixes: e6e9730c7066 ("buildtools: support object file extraction for Windows")
> Cc: stable@dpdk.org
> 
> Reported-by: Michal Berger <michallinuxstuff@gmail.com>
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
>  buildtools/gen-pmdinfo-cfile.py | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
>

Here is an alternative fix that works in my testing, based on my earlier
suggestion:

Regards,
/Bruce

diff --git a/buildtools/gen-pmdinfo-cfile.py b/buildtools/gen-pmdinfo-cfile.py
index 58fe3ad152..5fbd51658a 100644
--- a/buildtools/gen-pmdinfo-cfile.py
+++ b/buildtools/gen-pmdinfo-cfile.py
@@ -9,12 +9,13 @@

 _, tmp_root, ar, archive, output, *pmdinfogen = sys.argv
 with tempfile.TemporaryDirectory(dir=tmp_root) as temp:
-    run_ar = lambda command: subprocess.run(
-        [ar, command, os.path.abspath(archive)],
-        stdout=subprocess.PIPE, check=True, cwd=temp
-    )
-    # Don't use "ar p", because its output is corrupted on Windows.
-    run_ar("x")
-    names = run_ar("t").stdout.decode().splitlines()
-    paths = [os.path.join(temp, name) for name in names]
+    paths = []
+    for name in subprocess.run([ar, "t", archive], stdout=subprocess.PIPE,
+                               check=True).stdout.decode().splitlines():
+        if os.path.exists(name):
+            paths.append(name)
+        else:
+            subprocess.run([ar, "x", os.path.abspath(archive), name],
+                           check=True, cwd=temp)
+            paths.append(os.path.join(temp, name))
     subprocess.run(pmdinfogen + paths + [output], check=True)
Dmitry Kozlyuk Oct. 27, 2021, 4:45 p.m. UTC | #3
2021-10-27 17:16 (UTC+0100), Bruce Richardson:
> On Tue, Oct 26, 2021 at 10:32:39PM +0300, Dmitry Kozlyuk wrote:
> > Meson 0.60 switched the format of uninstalled static libraries
> > to thin archives, that is, they contain only paths to object files,
> > not the files themselves. Files cannot be extracted in this case,
> > resulting in build errors:
> > 
> >     ar: `x' cannot be used on thin archives.
> > 
> > Handle thin archives when invoking pmdinfogen
> > by directly using the files referenced in the archive.
> > 
> > Bugzilla ID: 836
> > Fixes: e6e9730c7066 ("buildtools: support object file extraction for Windows")
> > Cc: stable@dpdk.org
> > 
> > Reported-by: Michal Berger <michallinuxstuff@gmail.com>
> > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > ---
> >  buildtools/gen-pmdinfo-cfile.py | 23 ++++++++++++++---------
> >  1 file changed, 14 insertions(+), 9 deletions(-)
> >  
> 
> Here is an alternative fix that works in my testing, based on my earlier
> suggestion:
> 
> Regards,
> /Bruce
> 
> diff --git a/buildtools/gen-pmdinfo-cfile.py b/buildtools/gen-pmdinfo-cfile.py
> index 58fe3ad152..5fbd51658a 100644
> --- a/buildtools/gen-pmdinfo-cfile.py
> +++ b/buildtools/gen-pmdinfo-cfile.py
> @@ -9,12 +9,13 @@
> 
>  _, tmp_root, ar, archive, output, *pmdinfogen = sys.argv
>  with tempfile.TemporaryDirectory(dir=tmp_root) as temp:
> -    run_ar = lambda command: subprocess.run(
> -        [ar, command, os.path.abspath(archive)],
> -        stdout=subprocess.PIPE, check=True, cwd=temp
> -    )
> -    # Don't use "ar p", because its output is corrupted on Windows.
> -    run_ar("x")
> -    names = run_ar("t").stdout.decode().splitlines()
> -    paths = [os.path.join(temp, name) for name in names]
> +    paths = []
> +    for name in subprocess.run([ar, "t", archive], stdout=subprocess.PIPE,
> +                               check=True).stdout.decode().splitlines():
> +        if os.path.exists(name):
> +            paths.append(name)
> +        else:
> +            subprocess.run([ar, "x", os.path.abspath(archive), name],
> +                           check=True, cwd=temp)
> +            paths.append(os.path.join(temp, name))
>      subprocess.run(pmdinfogen + paths + [output], check=True)
> 

It buys with simplicity.
I hoped to avoid creating temporary directories in vain
when all archives are thin, but maybe it's not that important.
Bruce Richardson Nov. 1, 2021, 4:54 p.m. UTC | #4
On Wed, Oct 27, 2021 at 07:45:28PM +0300, Dmitry Kozlyuk wrote:
> 2021-10-27 17:16 (UTC+0100), Bruce Richardson:
> > On Tue, Oct 26, 2021 at 10:32:39PM +0300, Dmitry Kozlyuk wrote:
> > > Meson 0.60 switched the format of uninstalled static libraries
> > > to thin archives, that is, they contain only paths to object files,
> > > not the files themselves. Files cannot be extracted in this case,
> > > resulting in build errors:
> > > 
> > >     ar: `x' cannot be used on thin archives.
> > > 
> > > Handle thin archives when invoking pmdinfogen
> > > by directly using the files referenced in the archive.
> > > 
> > > Bugzilla ID: 836
> > > Fixes: e6e9730c7066 ("buildtools: support object file extraction for Windows")
> > > Cc: stable@dpdk.org
> > > 
> > > Reported-by: Michal Berger <michallinuxstuff@gmail.com>
> > > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > > ---
> > >  buildtools/gen-pmdinfo-cfile.py | 23 ++++++++++++++---------
> > >  1 file changed, 14 insertions(+), 9 deletions(-)
> > >  
> > 
> > Here is an alternative fix that works in my testing, based on my earlier
> > suggestion:
> > 
> > Regards,
> > /Bruce
> > 
> > diff --git a/buildtools/gen-pmdinfo-cfile.py b/buildtools/gen-pmdinfo-cfile.py
> > index 58fe3ad152..5fbd51658a 100644
> > --- a/buildtools/gen-pmdinfo-cfile.py
> > +++ b/buildtools/gen-pmdinfo-cfile.py
> > @@ -9,12 +9,13 @@
> > 
> >  _, tmp_root, ar, archive, output, *pmdinfogen = sys.argv
> >  with tempfile.TemporaryDirectory(dir=tmp_root) as temp:
> > -    run_ar = lambda command: subprocess.run(
> > -        [ar, command, os.path.abspath(archive)],
> > -        stdout=subprocess.PIPE, check=True, cwd=temp
> > -    )
> > -    # Don't use "ar p", because its output is corrupted on Windows.
> > -    run_ar("x")
> > -    names = run_ar("t").stdout.decode().splitlines()
> > -    paths = [os.path.join(temp, name) for name in names]
> > +    paths = []
> > +    for name in subprocess.run([ar, "t", archive], stdout=subprocess.PIPE,
> > +                               check=True).stdout.decode().splitlines():
> > +        if os.path.exists(name):
> > +            paths.append(name)
> > +        else:
> > +            subprocess.run([ar, "x", os.path.abspath(archive), name],
> > +                           check=True, cwd=temp)
> > +            paths.append(os.path.join(temp, name))
> >      subprocess.run(pmdinfogen + paths + [output], check=True)
> > 
> 
> It buys with simplicity.
> I hoped to avoid creating temporary directories in vain
> when all archives are thin, but maybe it's not that important.

Since the temp directory is contained within the build directory, I don't
think it matters. If you don't mind then, I'll do up a v2 patch with my
alternative fix and submit that to the list.

Regards,
/Bruce
Dmitry Kozlyuk Nov. 1, 2021, 5:36 p.m. UTC | #5
2021-11-01 16:54 (UTC+0000), Bruce Richardson:
> On Wed, Oct 27, 2021 at 07:45:28PM +0300, Dmitry Kozlyuk wrote:
> > 2021-10-27 17:16 (UTC+0100), Bruce Richardson:  
> > > On Tue, Oct 26, 2021 at 10:32:39PM +0300, Dmitry Kozlyuk wrote:  
> > > > Meson 0.60 switched the format of uninstalled static libraries
> > > > to thin archives, that is, they contain only paths to object files,
> > > > not the files themselves. Files cannot be extracted in this case,
> > > > resulting in build errors:
> > > > 
> > > >     ar: `x' cannot be used on thin archives.
> > > > 
> > > > Handle thin archives when invoking pmdinfogen
> > > > by directly using the files referenced in the archive.
> > > > 
> > > > Bugzilla ID: 836
> > > > Fixes: e6e9730c7066 ("buildtools: support object file extraction for Windows")
> > > > Cc: stable@dpdk.org
> > > > 
> > > > Reported-by: Michal Berger <michallinuxstuff@gmail.com>
> > > > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > > > ---
> > > >  buildtools/gen-pmdinfo-cfile.py | 23 ++++++++++++++---------
> > > >  1 file changed, 14 insertions(+), 9 deletions(-)
> > > >    
> > > 
> > > Here is an alternative fix that works in my testing, based on my earlier
> > > suggestion:
> > > 
> > > Regards,
> > > /Bruce
> > > 
> > > diff --git a/buildtools/gen-pmdinfo-cfile.py b/buildtools/gen-pmdinfo-cfile.py
> > > index 58fe3ad152..5fbd51658a 100644
> > > --- a/buildtools/gen-pmdinfo-cfile.py
> > > +++ b/buildtools/gen-pmdinfo-cfile.py
> > > @@ -9,12 +9,13 @@
> > > 
> > >  _, tmp_root, ar, archive, output, *pmdinfogen = sys.argv
> > >  with tempfile.TemporaryDirectory(dir=tmp_root) as temp:
> > > -    run_ar = lambda command: subprocess.run(
> > > -        [ar, command, os.path.abspath(archive)],
> > > -        stdout=subprocess.PIPE, check=True, cwd=temp
> > > -    )
> > > -    # Don't use "ar p", because its output is corrupted on Windows.
> > > -    run_ar("x")
> > > -    names = run_ar("t").stdout.decode().splitlines()
> > > -    paths = [os.path.join(temp, name) for name in names]
> > > +    paths = []
> > > +    for name in subprocess.run([ar, "t", archive], stdout=subprocess.PIPE,
> > > +                               check=True).stdout.decode().splitlines():
> > > +        if os.path.exists(name):
> > > +            paths.append(name)
> > > +        else:
> > > +            subprocess.run([ar, "x", os.path.abspath(archive), name],
> > > +                           check=True, cwd=temp)
> > > +            paths.append(os.path.join(temp, name))
> > >      subprocess.run(pmdinfogen + paths + [output], check=True)
> > >   
> > 
> > It buys with simplicity.
> > I hoped to avoid creating temporary directories in vain
> > when all archives are thin, but maybe it's not that important.  
> 
> Since the temp directory is contained within the build directory, I don't
> think it matters. If you don't mind then, I'll do up a v2 patch with my
> alternative fix and submit that to the list.

OK, thank you!
I meant the time spent on creating those dirs rather than polluting any place.
But it won't be any worse then it is now.
diff mbox series

Patch

diff --git a/buildtools/gen-pmdinfo-cfile.py b/buildtools/gen-pmdinfo-cfile.py
index 58fe3ad152..3453e5b4b9 100644
--- a/buildtools/gen-pmdinfo-cfile.py
+++ b/buildtools/gen-pmdinfo-cfile.py
@@ -8,13 +8,18 @@ 
 import tempfile
 
 _, tmp_root, ar, archive, output, *pmdinfogen = sys.argv
-with tempfile.TemporaryDirectory(dir=tmp_root) as temp:
-    run_ar = lambda command: subprocess.run(
-        [ar, command, os.path.abspath(archive)],
-        stdout=subprocess.PIPE, check=True, cwd=temp
-    )
-    # Don't use "ar p", because its output is corrupted on Windows.
-    run_ar("x")
-    names = run_ar("t").stdout.decode().splitlines()
-    paths = [os.path.join(temp, name) for name in names]
+archive = os.path.abspath(archive)
+names = subprocess.run([ar, "t", archive],
+    stdout=subprocess.PIPE, check=True).stdout.decode().splitlines()
+with open(archive, "rb") as f:
+    is_thin = f.read(7) == b"!<thin>"
+if is_thin:
+    # Thin archive needs no unpacking, just use the paths within.
+    paths = [os.path.join(archive, name) for name in names]
     subprocess.run(pmdinfogen + paths + [output], check=True)
+else:
+    with tempfile.TemporaryDirectory(dir=tmp_root) as temp:
+        # Don't use "ar p", because its output is corrupted on Windows.
+        paths = [os.path.join(temp, name) for name in names]
+        subprocess.run([ar, "x", archive], check=True, cwd=temp)
+        subprocess.run(pmdinfogen + paths + [output], check=True)