Message ID | 20230812061540.5398-1-akihiko.odaki@daynix.com |
---|---|
State | New |
Headers | show |
Series | meson: Fix MESONINTROSPECT parsing | expand |
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
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>
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
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 > >
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
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
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
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 --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)
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(-)