Detecting ninja as ninja_args for support python wrapper of ninja on …

[lygstate]

…Windows such as 'python3', '-B', 'E:/abspath/qemu.org-x64/ninjatool' This is used for fixing the building of qemu in msys2/mingw64/mingw32 on Windows.

QEMU need somethings like this

if [ "$MSYSTEM" = "MINGW64" -o  "$MSYSTEM" = "MINGW32" ]; then
PWD_WIN=`pwd -W`
NINJA_TOOL=$PWD_WIN/ninjatool
else
NINJA_TOOL=$PWD/ninjatool
fi
NINJA="$python $NINJA_TOOL" $meson setup \
        --prefix "${pre_prefix}$prefix" \
        --libdir "${pre_prefix}$libdir" \
        --libexecdir "${pre_prefix}$libexecdir" \
        --bindir "${pre_prefix}$bindir" \
        --includedir "${pre_prefix}$includedir" \
        --datadir "${pre_prefix}$datadir" \
        --mandir "${pre_prefix}$mandir" \
        --sysconfdir "${pre_prefix}$sysconfdir" \
        --localstatedir "${pre_prefix}$local_statedir" \
        -Doptimization=$(if test "$debug" = yes; then echo 0; else echo 2; fi) \
        -Ddebug=$(if test "$debug_info" = yes; then echo true; else echo false; fi) \
        -Dwerror=$(if test "$werror" = yes; then echo true; else echo false; fi) \
        -Dstrip=$(if test "$strip_opt" = yes; then echo true; else echo false; fi) \
        -Db_pie=$(if test "$pie" = yes; then echo true; else echo false; fi) \
        -Db_coverage=$(if test "$gcov" = yes; then echo true; else echo false; fi) \
	-Dsdl=$sdl -Dsdl_image=$sdl_image \
	-Dvnc=$vnc -Dvnc_sasl=$vnc_sasl -Dvnc_jpeg=$vnc_jpeg -Dvnc_png=$vnc_png \
	-Dgettext=$gettext \
        $cross_arg \
        "$PWD" "$source_path"

[eli-schwartz]

Why don't you just use ninja itself?

If you really feel the need for some wrapper tool, an executable python script should be able to be associated with the python interpreter and run properly on its own without needing to first specify "python" in the environment variable.

[lygstate]

@eli-schwartz It's all about Windows, on Windows you can not associate python script with python executable.

[bonzini]

I agree that in order to build the compdb on Windows QEMU could just be using ninja directly. (In fact I hope to get rid of ninjatool before the next version of QEMU is released. The version that was committed still has some makefile-based binaries so it needs ninjatool, but they can be converted pretty easily).

[bonzini]

This can be closed as far as QEMU is concerned. However there is another bug that was found in the NINJA variable on Windows: if a full path is included it must include the .exe suffix:

• NINJA=ninja - works
• NINJA=C:/msys64/mingw64/bin/ninja - fails
• NINJA=C:/msys64/mingw64/bin/ninja.exe - works

@lygstate you might want to look into fixing that instead?

[lygstate]

This can be closed as far as QEMU is concerned. However there is another bug that was found in the NINJA variable on Windows: if a full path is included it must include the .exe suffix:

• NINJA=ninja - works
• NINJA=C:/msys64/mingw64/bin/ninja - fails
• NINJA=C:/msys64/mingw64/bin/ninja.exe - works

@lygstate you might want to look into fixing that instead?

This is a temp fixes in qemu, may this patch provide a proper handling of ninja wrapper?

[bonzini]

If I understand correctly, this patch doesn't fix the issue with no .exe extension in the NINJA variable? That one is definitely a bug, while this patch on the other hand may cause issues if the path to ninja includes a space, which is something that Meson would certainly prefer to handle.

[lygstate]

If I understand correctly, this patch doesn't fix the issue with no .exe extension in the NINJA variable? That one is definitely a bug, while this patch on the other hand may cause issues if the path to ninja includes a space, which is something that Meson would certainly prefer to handle.

@

If I understand correctly, this patch doesn't fix the issue with no .exe extension in the NINJA variable? That one is definitely a bug, while this patch on the other hand may cause issues if the path to ninja includes a space, which is something that Meson would certainly prefer to handle.

I am using shlex.split(n) that will handle path with space if its quoted.

Are you talking about path like this C:/path with space/msys64/mingw64/bin/ninja.exe that are not quoted?

[bonzini]

Right (which would be bad if ninja was under Program Files). But adding. exe would not have the possibility of causing regressions.

[eli-schwartz]

I still don't see how this is a meson problem.

But if meson should "handle" this, it should do so using a mechanism that is consistent between $NINJA and other environment variables, and/or teach meson to recognize shebang lines in the ninja executable like it does in other places.

This pull request is wrong, simply because it shows the intent is to paper over the problem instead of properly handling it.

[lygstate]

This pull request is wrong, simply because it shows the intent is to paper over the problem instead of properly handling it.

Sounds a good idea, maybe getting the Popen to recognize the shebang lines?

[eli-schwartz]

You could probably take a look at how find_program() works.

[bonzini]

The mere existence of ninjatool in QEMU is a temporary hack and I don't think Meson should be complicated in order to support it.

Does find_program have the same issue where the lack of .exe on an absolute path causes problems? If so it would indeed be a good opportunity to fix the bug in two places and unify the code.

[lygstate]

The mere existence of ninjatool in QEMU is a temporary hack and I don't think Meson should be complicated in order to support it.

Does find_program have the same issue where the lack of .exe on an absolute path causes problems? If so it would indeed be a good opportunity to fix the bug in two places and unify the code.

its not only about qemu, but also about broader usage of meson

[bonzini]

its not only about qemu, but also about broader usage of meson

Unless there is a widespread Python implementation of ninja, it's not. Again, there are two issues that were uncovered due to QEMU's "unconventional" use of Meson:

• Python script as $NINJA - this is totally a hack and not worth spending one more minute on. First, because QEMU will require Ninja sooner or later on all platforms. Second, because MSYS is a secondary platform for QEMU and it's absolutely fine to require NInja right now to build QEMU on MSYS.

• absolute path without .exe as $NINJA - this is indeed a usability issue, and it's also worth checking if find_program has the same issue.

[mensinda]

I haven't really read this issue / PR in-depth, however, I want to add that #7518 also needs to run ninja (packaged in the AppImage) with a wrapper.

[bonzini]

run ninja (packaged in the AppImage) with a wrapper.

I don't think it's comparable since $NINJA in fact is completely ignored when using AppImage.

[mensinda]

Well, this PR looks fairly similar to 5c344f1. I just wanted to add that there is a use case for an arbitrary wrapper around ninja. I am not entirely sure how this will relate to "$NINJA" though, and the *.exe situation.

[bonzini]

Yes I agree that the code looks pretty much the same, but it's mostly a coincidence. In this patch there is no wrapper, only an interpreter. The issue of not having shebang interpretation is obviously not a problem for AppImage.

[lygstate]

Well, this PR looks fairly similar to 5c344f1. I just wanted to add that there is a use case for an arbitrary wrapper around ninja. I am not entirely sure how this will relate to "$NINJA" though, and the *.exe situation.

Seems your patch are very like mine, would the find_program solve your problem?

[lygstate]

Well, this PR looks fairly similar to 5c344f1. I just wanted to add that there is a use case for an arbitrary wrapper around ninja. I am not entirely sure how this will relate to "$NINJA" though, and the *.exe situation.

@eli-schwartz How about add a extra parameter, NINJA_ARGS, and preseve NINJA unchange with shebang fixes?
This both preseve the current sematic and fixes AppImage and qeme wrapper needs

[mensinda]

No, since there is no shebang since the AppImage is an ELF binary. Ninja is called via /path/to/Meson.AppImage --apprun-ninja

[lygstate]

No, since there is no shebang since the AppImage is an ELF binary. Ninja is called via /path/to/Meson.AppImage --apprun-ninja

You didn't got the idea, find_program also recognize ELF binary, but may also shebang , find_program can find elf program or exe, but not sure if it works for shebang, I think it's works, because I found something like

nm = find_program('nm')
undefsym = find_program('scripts/undefsym.py')

nm is elf binary, scripts/undefsym.py is shebang python script.

[bonzini]

The two patches have a common part (multi-arg invocation of ninja) but that's where the similarities end. Can we please stop chasing this red herring and fix a real bug?

[mensinda]

I don't really like using both NINJA and NINJA_ARGS, since shell splitting has to be done anyway with NINJA_ARGS and NINJA could point to something that is semantically definitely not ninja (python, the AppImage)

[lygstate]

I don't really like using both NINJA and NINJA_ARGS, since shell splitting has to be done anyway with NINJA_ARGS and NINJA could point to something that is semantically definitely not ninja (python, the AppImage)

But their intention is a wrapper for ninja, at least ninja related, if you using something else, would be more confusing...

[lygstate]

I don't really like using both NINJA and NINJA_ARGS, since shell splitting has to be done anyway with NINJA_ARGS and NINJA could point to something that is semantically definitely not ninja (python, the AppImage)

NINJA can not be changed, maybe using NINJA_WRAPPER_ARGS ?

[lygstate]

The two patches have a common part (multi-arg invocation of ninja) but that's where the similarities end. Can we please stop chasing this red herring and fix a real bug?

There is a real feature are asking for that,
#7518

[eli-schwartz]

This discussion is exhausting.

@bonzini Can you open a tracking bug for the missing .exe case, as it's totally unrelated to this current PR, and then we can let this PR succeed or most likely fail on its own merit?

There is a real feature are asking for that,
#7518

@lygstate you are literally responding to someone discussing that PR and saying it's not the same as this, by telling that person the other PR exists. This is a useless waste of time, by definition @bonzini knows it exists.

The only intellectually stimulating reply you can provide, would be to describe why you believe they are, in fact, the same.
NOT to tell people that the other PR exists.

[bonzini]

I opened #7659 for the .exe case.

[eli-schwartz]

Since we're now using ExternalProgram just like find_program does, this should now be automatically handled. Just use NINJA=/path/to/ninjatool.py and meson detects that it has a shebang and figures out how to run it for you. No need to manually specify the python executable in $NINJA, or do argument splitting.

[lygstate]

Since we're now using ExternalProgram just like find_program does, this should now be automatically handled. Just use NINJA=/path/to/ninjatool.py and meson detects that it has a shebang and figures out how to run it for you. No need to manually specify the python executable in $NINJA, or do argument splitting.

right