Re: [PATCH v4 2/4] buildtools: use macro to embed information in binaries

[Andre Muezerie <andremue@linux.microsoft.com>]

On Wed, Jun 04, 2025 at 01:04:29PM +0200, David Marchand wrote:
> Hello Andre,
> 
> On Wed, Apr 16, 2025 at 11:45 PM Andre Muezerie
> <andremue@linux.microsoft.com> wrote:
> >
> > The archiver tool from the MSVC toolset is lib.exe. It has different
> > parameters then it's GNU counterpart "ar".
> >
> > buildtools\meson.build was updated to use lib.exe when MSVC compiler is
> > used. This is to allow the code to be built without requiring GNU "ar"
> > to be installed in that scenario.
> >
> > Script gen-pmdinfo-cfile.py was updated to use the correct parameters
> > when using lib.exe.
> >
> > Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> > ---
> >  buildtools/gen-pmdinfo-cfile.py | 24 +++++++++++++++++++-----
> >  buildtools/meson.build          |  4 +++-
> >  buildtools/pmdinfogen.py        | 11 ++++++-----
> >  3 files changed, 28 insertions(+), 11 deletions(-)
> >
> > diff --git a/buildtools/gen-pmdinfo-cfile.py b/buildtools/gen-pmdinfo-cfile.py
> > index 5fbd51658a..3a078ab6d1 100644
> > --- a/buildtools/gen-pmdinfo-cfile.py
> > +++ b/buildtools/gen-pmdinfo-cfile.py
> > @@ -7,15 +7,29 @@
> >  import sys
> >  import tempfile
> >
> > -_, tmp_root, ar, archive, output, *pmdinfogen = sys.argv
> > +_, tmp_root, archiver, archive, output, *pmdinfogen = sys.argv
> >  with tempfile.TemporaryDirectory(dir=tmp_root) as temp:
> >      paths = []
> > -    for name in subprocess.run([ar, "t", archive], stdout=subprocess.PIPE,
> > -                               check=True).stdout.decode().splitlines():
> > +    if archiver == "lib":
> > +        archiver_options = ["/LIST", "/NOLOGO"]
> > +    else:
> > +        archiver_options = ["t"]
> > +    for name in (
> > +        subprocess.run(
> > +            [archiver] + archiver_options + [archive],
> > +            stdout=subprocess.PIPE,
> > +            check=True,
> > +        )
> > +        .stdout.decode()
> > +        .splitlines()
> > +    ):
> >          if os.path.exists(name):
> >              paths.append(name)
> 
> Indent after the change seems strange.
> 
> And we need a rebase of this series in any case.
> Please send a new revision.
> 

I rebased the code and formatted it using Ruff. I agree that the formatting
looks substantially different than the original code and it takes a while to
get used to it, but an argument could be made that the new formatting looks
better.

> 
> 
> -- 
> David Marchand