[PATCH] meson: Fix MESONINTROSPECT parsing

[PATCH] meson: Fix MESONINTROSPECT parsing

[Akihiko Odaki]

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

[Michael Tokarev]

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

[Akihiko Odaki]

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

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 2506 bytes --]

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
>
>

[-- Attachment #2: Type: text/html, Size: 3748 bytes --]

Re: [PATCH] meson: Fix MESONINTROSPECT parsing

[Philippe Mathieu-Daudé]

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

[Akihiko Odaki]

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

[Michael Tokarev]

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

[Peter Maydell]

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

[Michael Tokarev]

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