1. Patchew
  2. QEMU
  3. View series

[PATCH] meson: Fix MESONINTROSPECT parsing

Akihiko Odaki posted 1 patch 9 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230812061540.5398-1-akihiko.odaki@daynix.com
Maintainers: John Snow <jsnow@redhat.com>, Cleber Rosa <crosa@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Thomas Huth <thuth@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>
There is a newer version of this series
scripts/symlink-install-tree.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[PATCH] meson: Fix MESONINTROSPECT parsing
Posted by Akihiko Odaki 9 months, 1 week ago 
The arguments in MESONINTROSPECT are quoted with shlex.quote() so it
must be parsed with shlex.split().

Fixes: cf60ccc330 ("cutils: Introduce bundle mechanism")
Reported-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 scripts/symlink-install-tree.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/symlink-install-tree.py b/scripts/symlink-install-tree.py
index 8ed97e3c94..b72563895c 100644
--- a/scripts/symlink-install-tree.py
+++ b/scripts/symlink-install-tree.py
@@ -4,6 +4,7 @@
 import errno
 import json
 import os
+import shlex
 import subprocess
 import sys
 
@@ -14,7 +15,7 @@ def destdir_join(d1: str, d2: str) -> str:
     return str(PurePath(d1, *PurePath(d2).parts[1:]))
 
 introspect = os.environ.get('MESONINTROSPECT')
-out = subprocess.run([*introspect.split(' '), '--installed'],
+out = subprocess.run([*shlex.split(introspect), '--installed'],
                      stdout=subprocess.PIPE, check=True).stdout
 for source, dest in json.loads(out).items():
     bundle_dest = destdir_join('qemu-bundle', dest)
-- 
2.41.0
Re: [PATCH] meson: Fix MESONINTROSPECT parsing
Posted by Akihiko Odaki 9 months, 1 week ago 
On 2023/08/12 15:15, Akihiko Odaki wrote:
> The arguments in MESONINTROSPECT are quoted with shlex.quote() so it
> must be parsed with shlex.split().
> 
> Fixes: cf60ccc330 ("cutils: Introduce bundle mechanism")
> Reported-by: Michael Tokarev <mjt@tls.msk.ru>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   scripts/symlink-install-tree.py | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/symlink-install-tree.py b/scripts/symlink-install-tree.py
> index 8ed97e3c94..b72563895c 100644
> --- a/scripts/symlink-install-tree.py
> +++ b/scripts/symlink-install-tree.py
> @@ -4,6 +4,7 @@
>   import errno
>   import json
>   import os
> +import shlex
>   import subprocess
>   import sys
>   
> @@ -14,7 +15,7 @@ def destdir_join(d1: str, d2: str) -> str:
>       return str(PurePath(d1, *PurePath(d2).parts[1:]))
>   
>   introspect = os.environ.get('MESONINTROSPECT')
> -out = subprocess.run([*introspect.split(' '), '--installed'],
> +out = subprocess.run([*shlex.split(introspect), '--installed'],
>                        stdout=subprocess.PIPE, check=True).stdout
>   for source, dest in json.loads(out).items():
>       bundle_dest = destdir_join('qemu-bundle', dest)

Please do NOT merge this. This will break Windows builds. I'm putting 
this patch on hold.

The problem is that Meson uses a different logic for escaping arguments 
in MESONINTROSPECT on Windows. I'll wait till Meson maintainers figure 
out how MESONINTROSPECT should be used. For details, see:
https://github.com/mesonbuild/meson/pull/12115#issuecomment-1675863266

Regards,
Akihiko Odaki
Re: [PATCH] meson: Fix MESONINTROSPECT parsing
Posted by Peter Maydell 1 month, 3 weeks ago 
On Sat, 12 Aug 2023 at 13:10, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2023/08/12 15:15, Akihiko Odaki wrote:
> > The arguments in MESONINTROSPECT are quoted with shlex.quote() so it
> > must be parsed with shlex.split().
> >
> > Fixes: cf60ccc330 ("cutils: Introduce bundle mechanism")
> > Reported-by: Michael Tokarev <mjt@tls.msk.ru>
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> >   scripts/symlink-install-tree.py | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/symlink-install-tree.py b/scripts/symlink-install-tree.py
> > index 8ed97e3c94..b72563895c 100644
> > --- a/scripts/symlink-install-tree.py
> > +++ b/scripts/symlink-install-tree.py
> > @@ -4,6 +4,7 @@
> >   import errno
> >   import json
> >   import os
> > +import shlex
> >   import subprocess
> >   import sys
> >
> > @@ -14,7 +15,7 @@ def destdir_join(d1: str, d2: str) -> str:
> >       return str(PurePath(d1, *PurePath(d2).parts[1:]))
> >
> >   introspect = os.environ.get('MESONINTROSPECT')
> > -out = subprocess.run([*introspect.split(' '), '--installed'],
> > +out = subprocess.run([*shlex.split(introspect), '--installed'],
> >                        stdout=subprocess.PIPE, check=True).stdout
> >   for source, dest in json.loads(out).items():
> >       bundle_dest = destdir_join('qemu-bundle', dest)
>
> Please do NOT merge this. This will break Windows builds. I'm putting
> this patch on hold.
>
> The problem is that Meson uses a different logic for escaping arguments
> in MESONINTROSPECT on Windows. I'll wait till Meson maintainers figure
> out how MESONINTROSPECT should be used. For details, see:
> https://github.com/mesonbuild/meson/pull/12115#issuecomment-1675863266

Am I correct in understanding from
https://github.com/mesonbuild/meson/pull/12807
that the eventual resolution that Meson upstream decided was
to restore the behaviour that regardless of platform the right
way to split the file is shlex.split()?

If so, then I think we should resurrect and apply this patch,
since at the moment configuring QEMU fails if, for instance,
the build tree directory name has a '~' character in it.

thanks
-- PMM
Re: [PATCH] meson: Fix MESONINTROSPECT parsing
Posted by Michael Tokarev 1 month, 3 weeks ago 
25.03.2024 20:23, Peter Maydell :
> On Sat, 12 Aug 2023 at 13:10, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
...
>> The problem is that Meson uses a different logic for escaping arguments
>> in MESONINTROSPECT on Windows. I'll wait till Meson maintainers figure
>> out how MESONINTROSPECT should be used. For details, see:
>> https://github.com/mesonbuild/meson/pull/12115#issuecomment-1675863266
> 
> Am I correct in understanding from
> https://github.com/mesonbuild/meson/pull/12807
> that the eventual resolution that Meson upstream decided was
> to restore the behaviour that regardless of platform the right
> way to split the file is shlex.split()?
> 
> If so, then I think we should resurrect and apply this patch,
> since at the moment configuring QEMU fails if, for instance,
> the build tree directory name has a '~' character in it.

Yes, meson upstream went for shlex.split() finally.
However this change is pretty recent (Feb-11) if applying, we have
to upgrade meson too, which is not a good idea at this stage during
RCs.

The whole issue is very minor though.  I think so far it only happened
on debian due to its usage of tilde (~) char in 9.0.0~rc0 (instead of
a minus sign), because in debian, a tilde has special meaning in version
string, it sorts before anything else, even before 9.0.0 in this case.

I don't think this issue is a problem in practice in any other context.
So for now, I'll apply this patch to qemu in debian to let the RCs
build, it wont be needed after actual release, and we can rethink
this for 9.1 or a later version.

Thanks,

/mjt
Re: [PATCH] meson: Fix MESONINTROSPECT parsing
Posted by Michael Tokarev 9 months, 1 week ago 
12.08.2023 15:09, Akihiko Odaki wrote:

> Please do NOT merge this. This will break Windows builds. I'm putting this patch on hold.

I don't think this is something to rush about, it definitely can wait
8.2 development cycle.

/mjt
Re: [PATCH] meson: Fix MESONINTROSPECT parsing
Posted by Philippe Mathieu-Daudé 9 months, 1 week ago 
On 12/8/23 08:15, Akihiko Odaki wrote:
> The arguments in MESONINTROSPECT are quoted with shlex.quote() so it
> must be parsed with shlex.split().
> 
> Fixes: cf60ccc330 ("cutils: Introduce bundle mechanism")
> Reported-by: Michael Tokarev <mjt@tls.msk.ru>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   scripts/symlink-install-tree.py | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Re: [PATCH] meson: Fix MESONINTROSPECT parsing
Posted by Michael Tokarev 9 months, 1 week ago 
12.08.2023 09:15, Akihiko Odaki wrote:
> The arguments in MESONINTROSPECT are quoted with shlex.quote() so it
> must be parsed with shlex.split().
> 
> Fixes: cf60ccc330 ("cutils: Introduce bundle mechanism")
> Reported-by: Michael Tokarev <mjt@tls.msk.ru>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   scripts/symlink-install-tree.py | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/symlink-install-tree.py b/scripts/symlink-install-tree.py
> index 8ed97e3c94..b72563895c 100644
> --- a/scripts/symlink-install-tree.py
> +++ b/scripts/symlink-install-tree.py
> @@ -4,6 +4,7 @@
>   import errno
>   import json
>   import os
> +import shlex
>   import subprocess
>   import sys
>   
> @@ -14,7 +15,7 @@ def destdir_join(d1: str, d2: str) -> str:
>       return str(PurePath(d1, *PurePath(d2).parts[1:]))
>   
>   introspect = os.environ.get('MESONINTROSPECT')
> -out = subprocess.run([*introspect.split(' '), '--installed'],
> +out = subprocess.run([*shlex.split(introspect), '--installed'],
>                        stdout=subprocess.PIPE, check=True).stdout
>   for source, dest in json.loads(out).items():
>       bundle_dest = destdir_join('qemu-bundle', dest)

This fixes one of the two issues, - the script is being run
now without failures.

Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
Tested-by: Michael Tokarev <mjt@tls.msk.ru>

There's one more possible problem which is worth to fix, I'd say:
it is the fact that script failure is not detected in any way.
Shouldn't subprocess.run raise an exception in case of failure?
I think it needs check=True (since python 3.5 iirc).

Thanks,

/mjt
Re: [PATCH] meson: Fix MESONINTROSPECT parsing
Posted by Akihiko Odaki 9 months, 1 week ago 
On 2023/08/12 17:01, Michael Tokarev wrote:
> 12.08.2023 09:15, Akihiko Odaki wrote:
>> The arguments in MESONINTROSPECT are quoted with shlex.quote() so it
>> must be parsed with shlex.split().
>>
>> Fixes: cf60ccc330 ("cutils: Introduce bundle mechanism")
>> Reported-by: Michael Tokarev <mjt@tls.msk.ru>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   scripts/symlink-install-tree.py | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/symlink-install-tree.py 
>> b/scripts/symlink-install-tree.py
>> index 8ed97e3c94..b72563895c 100644
>> --- a/scripts/symlink-install-tree.py
>> +++ b/scripts/symlink-install-tree.py
>> @@ -4,6 +4,7 @@
>>   import errno
>>   import json
>>   import os
>> +import shlex
>>   import subprocess
>>   import sys
>> @@ -14,7 +15,7 @@ def destdir_join(d1: str, d2: str) -> str:
>>       return str(PurePath(d1, *PurePath(d2).parts[1:]))
>>   introspect = os.environ.get('MESONINTROSPECT')
>> -out = subprocess.run([*introspect.split(' '), '--installed'],
>> +out = subprocess.run([*shlex.split(introspect), '--installed'],
>>                        stdout=subprocess.PIPE, check=True).stdout
>>   for source, dest in json.loads(out).items():
>>       bundle_dest = destdir_join('qemu-bundle', dest)
> 
> This fixes one of the two issues, - the script is being run
> now without failures.
> 
> Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
> Tested-by: Michael Tokarev <mjt@tls.msk.ru>
> 
> There's one more possible problem which is worth to fix, I'd say:
> it is the fact that script failure is not detected in any way.
> Shouldn't subprocess.run raise an exception in case of failure?
> I think it needs check=True (since python 3.5 iirc).

I missed that you noted this failure is not detected by configure. It is 
certainly better to fix.

It does have check=True but it's rather a obfuscated way to say that 
when you can just use subprocess.check_output(). I sent another patch to 
use subprocess.check_output().

The reason why configure does not detect the failure is that Meson 
ignores postconf script failures. I opened a pull request upstream:
https://github.com/mesonbuild/meson/pull/12115

Regards,
Akihiko Odaki
Re: [PATCH] meson: Fix MESONINTROSPECT parsing
Posted by Paolo Bonzini 9 months, 1 week ago 
Please apply this patch without my intervention, I won't have access to
Gitlab for a few days.

Paolo

Il sab 12 ago 2023, 11:16 Akihiko Odaki <akihiko.odaki@daynix.com> ha
scritto:

> On 2023/08/12 17:01, Michael Tokarev wrote:
> > 12.08.2023 09:15, Akihiko Odaki wrote:
> >> The arguments in MESONINTROSPECT are quoted with shlex.quote() so it
> >> must be parsed with shlex.split().
> >>
> >> Fixes: cf60ccc330 ("cutils: Introduce bundle mechanism")
> >> Reported-by: Michael Tokarev <mjt@tls.msk.ru>
> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >> ---
> >>   scripts/symlink-install-tree.py | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/scripts/symlink-install-tree.py
> >> b/scripts/symlink-install-tree.py
> >> index 8ed97e3c94..b72563895c 100644
> >> --- a/scripts/symlink-install-tree.py
> >> +++ b/scripts/symlink-install-tree.py
> >> @@ -4,6 +4,7 @@
> >>   import errno
> >>   import json
> >>   import os
> >> +import shlex
> >>   import subprocess
> >>   import sys
> >> @@ -14,7 +15,7 @@ def destdir_join(d1: str, d2: str) -> str:
> >>       return str(PurePath(d1, *PurePath(d2).parts[1:]))
> >>   introspect = os.environ.get('MESONINTROSPECT')
> >> -out = subprocess.run([*introspect.split(' '), '--installed'],
> >> +out = subprocess.run([*shlex.split(introspect), '--installed'],
> >>                        stdout=subprocess.PIPE, check=True).stdout
> >>   for source, dest in json.loads(out).items():
> >>       bundle_dest = destdir_join('qemu-bundle', dest)
> >
> > This fixes one of the two issues, - the script is being run
> > now without failures.
> >
> > Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
> > Tested-by: Michael Tokarev <mjt@tls.msk.ru>
> >
> > There's one more possible problem which is worth to fix, I'd say:
> > it is the fact that script failure is not detected in any way.
> > Shouldn't subprocess.run raise an exception in case of failure?
> > I think it needs check=True (since python 3.5 iirc).
>
> I missed that you noted this failure is not detected by configure. It is
> certainly better to fix.
>
> It does have check=True but it's rather a obfuscated way to say that
> when you can just use subprocess.check_output(). I sent another patch to
> use subprocess.check_output().
>
> The reason why configure does not detect the failure is that Meson
> ignores postconf script failures. I opened a pull request upstream:
> https://github.com/mesonbuild/meson/pull/12115
>
> Regards,
> Akihiko Odaki
>
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.