diff mbox series

meson: Fix MESONINTROSPECT parsing

Message ID 20230812061540.5398-1-akihiko.odaki@daynix.com
State New
Headers show
Series meson: Fix MESONINTROSPECT parsing | expand

Commit Message

Akihiko Odaki Aug. 12, 2023, 6:15 a.m. UTC
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(-)

Comments

Michael Tokarev Aug. 12, 2023, 8:01 a.m. UTC | #1
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
Philippe Mathieu-Daudé Aug. 12, 2023, 8:45 a.m. UTC | #2
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>
Akihiko Odaki Aug. 12, 2023, 9:16 a.m. UTC | #3
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
Paolo Bonzini Aug. 12, 2023, 11:13 a.m. UTC | #4
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
>
>
Akihiko Odaki Aug. 12, 2023, 12:09 p.m. UTC | #5
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
Michael Tokarev Aug. 12, 2023, 1:31 p.m. UTC | #6
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
Peter Maydell March 25, 2024, 5:23 p.m. UTC | #7
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
Michael Tokarev March 25, 2024, 5:34 p.m. UTC | #8
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
diff mbox series

Patch

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)