Use python3 instead of python

[keith]

This fixes a few stragglers after 6a17e84

[stephan-cr]

What if NINJA_PYTHON is empty, because it not found by find_package(Python3 COMPONENTS Interpreter) (and therefore Python3_EXECUTABLE is empty)?

[keith]

yea i think it would fail similarly to today, I can make it error in cmake if you'd like?

[digit-google]

I just tried this PR on top of upstream, and this change hard-codes /usr/bin/python3 (the value found by find_package() here) directly into the executable. This is not portable, because this will not run in certain environments where the interpreter is installed to a different location (we do have CI bots like that), and will generally ignore PATH, unlike the existing implementation.

A better change would simply to change python to python3.

[eli-schwartz]

@digit-google it sounds like maybe you didn't read the review comment that explained why hardcoding python3 is broken on Windows and needs to be fixed by using find_package()?

This is not portable, because this will not run in certain environments where the interpreter is installed to a different location (we do have CI bots like that), and will generally ignore PATH, unlike the existing implementation.

It will certainly run in any environment that matches the one you built ninja on, which means if you build ninja on a CI bot and test it on the same bot, everything works.

Who is "we" here, by the way? This PR passed the ninja project's CI. ;)

[digit-google]

Sorry, "we" in that specific case if the Fuchsia project, we have CI bots who run in an nsjail / container that doesn't have /usr/bin/python3 available (but have a vendored Python version in a different location, reachable from PATH, and this is done very intentionally to ensure the system version is never used by mistake).

Still, you make the produced Ninja binary less portable for no good reason. Just changing python to python3 should be enough to get what you want.

Besides, I recommend you document the exact issue that require this change, since "a few stragglers" doesn't seem an accurate justification.

[eli-schwartz]

Still, you make the produced Ninja binary less portable for no good reason. Just changing python to python3 should be enough to get what you want.

As mentioned in the above review comments, changing it to python3 fixes some platforms where there is no "python" binary but in turn breaks the case of Windows with the Microsoft Store version of python, where there is only a "python" binary and no "python3" binary.

Regardless, the name can be controlled by passing an explicit option to the build system so one could override it at compilation time with any choice of:

• python
• python3
• /usr/bin/python3
• C:/path/to/windows/store/python.exe
• /path/to/fuchsia/vendored/python3

The question here is really just about what should be autodetected.

Essentially this PR makes two changes:

• the default cmake option value changes from "python" to "use find_package() and return the answer"
• a test program starts respecting the option instead of hardcoding "python"

[eli-schwartz]

Also note that the production binary (as opposed to say the perf testing binary) IIRC doesn't really use python for anything other than the webserver embedded tool, which I suspect isn't relevant to fuchsia anyway. So embedding paths from a build machine that aren't relevant to the final runtime machine may not matter in practice.

[digit-google]

I believe that hard-coding paths that are specific to the build machine in binaries that will be distributed and run on a different machine is nearly always a bad idea.

This is the kind of stuff that will bite someone six months later, and will require hours or days of lost debugging time for developers who do not know enough about Ninja to solve this quickly (been there, done that, got the t-shirt :-)).

At a minimum there should be a way to override that value at runtime.

Using python for NINJA_PYTHON works sufficiently well because the system() call in browse.cc will search your PATH, which provides for at least one way to override the value in case of problem.

However, on systems that still link python to a python2 runtime, the system() call will succeed instead of returning ENOENT, hence the error message ninja: python is required for the browse tool will not be printed, and a Python2 stack trace will likely be the result, giving no useful information to the developer.

I suggest instead something a little bit more helpful:

• Keep using python + PATH search as the launcher name.
• Optionally use an environment variable like NINJA_PYTHON to point to a specific interpreter, or add an option to the -t browse tool to specify the python interpreter to use.
• Add a system() call to ${NINJA_PYTHON} --version in browse.cc to verify that the version is at least 3, and print an error message if this is not the case. Specify the tool option / env variable in the error message.
• Continue invoking ${NINJA_PYTHON} - --ninja-command <command> -f <input_file> ... after that.

This would be maximally portable, and will avoid wasting unknown amounts of developer time.

[digit-google]

A simpler alternative would be to detect python2 at runtime in the script, and print a user-friendly error message directly from it.
Checking sys.version_info.major >= 3 should be enough for that. This minimizes changes to browse.cc (and avoids the need to redirect stdout to a file and parsing it later).

[eli-schwartz]

I believe that hard-coding paths that are specific to the build machine in binaries that will be distributed and run on a different machine is nearly always a bad idea.

That's a very load-bearing "that are specific to the build machine". On Linux and *BSD systems it is a very good idea -- such a good idea in fact that it may be mandated via policy. If you depend on something you should ensure you use the thing you depend on -- not something that unpredictably gets found first because e.g. the user added anaconda to their shell login profile.

Certainly, Google may have problems that are atypical to the wider world of software development.

This is the kind of stuff that will bite someone six months later, and will require hours or days of lost debugging time for developers who do not know enough about Ninja to solve this quickly (been there, done that, got the t-shirt :-)).

That's totally backwards IMO.

The stated objection is that the command isn't guaranteed to exist. This is a trivially detectable issue -- it doesn't get easier to debug than "error: /usr/bin/python3: no such file or directory". There is nothing to debug, the only time spent will be time determining the right fix, whether that is trying to install the relevant interpreter or rebuilding ninja.

In comparison I can attest to people spending weeks fruitlessly debugging the case where the answer in the end turned out to be "the wrong python3 was found on PATH, and as a result the program tried to run but mysteriously bombed out 50 miles away with a nonsensical error message that provided zero clues as to why it happened and was even actively obfuscating the real cause in 7 different ways". Bugs where your software fails to tell you there was a problem and instead runs for a bit and then spews copious stack traces unrelated to the root cause are the absolute worst.

[eli-schwartz]

A simpler alternative would be to detect python2 at runtime in the script, and print a user-friendly error message directly from it. Checking sys.version_info.major >= 3 should be enough for that. This minimizes changes to browse.cc (and avoids the need to redirect stdout to a file and parsing it later).

The current error message is more likely to look like: error: python: no such file or directory

There is no opportunity to execute python code capable of detecting which version of python is being run.

[digit-google]

That's a very load-bearing "that are specific to the build machine". On Linux and *BSD systems it is a very good idea -- such a good idea in fact that it may be mandated via policy.

If you have such a policy, you can already implement it for the Ninja binaries that you produce by setting NINJA_PYTHON=/usr/bin/python3 in your CMake configuration step.

However, there is no need to impose such a policy in the official Ninja binaries released through Github, to other people who do not need it, or would be affected negatively by it.

And on Windows, where there is no standard installation location for the Python interpreter, the value embedded into the binary is essentially random (it depends too much on how the developer setup their system).

The stated objection is that the command isn't guaranteed to exist. This is a trivially detectable issue -- it doesn't get easier to debug than "error: /usr/bin/python3: no such file or directory".

No, a missing interpreter is already detected and will print an error message (see browse.cc). The only plausable issue is that invoking python on some (older) systems may invoke a Python2 interpreter, which in theory may fail with a user-hostile stack trace.

Looking at the content of browse.py, it looks like only the shebang line has been changed to use #!/usr/bin/env python3. That line is never used when launching the script from Ninja (only when starting it directly from a terminal), and its code already contains try/except and sys.version_info checks to support both Python2 and Python3 anyway.

So in the end, it looks like there is simply no use for this PR at all, as it doesn't fix anything, and likely regresses the tool on Windows.

[keith]

Updated to swap python for python3 instead of using cmake's discovery

[digit-google]

@keith , thanks for updating the PR.

This looks good to me, but https://peps.python.org/pep-0394/#exclusion-of-ms-windows seems to state that python3 is not guaranteed to be available on Windows. And there is python/cpython#99185 which seems to indicate that this is not solved :-(

We may want to cleanup the browse.py from Python2-specific code paths, but I'll let @jhasse comment on that.

And by the way, I just tried the previous version on my machine, with my PATH pointing to a non-standard location for the Python interpreter, and the build did embed that non-system location into the final executable. Hence the problem I described for Windows also existed for Unix anyway.

In conclusion, embedding auto-detected host paths into binaries is still a terrible idea.

Having a policy of enforcing standard locations like /usr/bin/python3, when they exist, is a different topic, which is probably not needed here.

[eli-schwartz]

No, a missing interpreter is already detected and will print an error message (see browse.cc). The only plausable issue is that invoking python on some (older) systems may invoke a Python2 interpreter, which in theory may fail with a user-hostile stack trace.

The plausible issue is exactly that it will print an error message, in a circumstance where it is trivially proven that the default is bad and, as you say, user-hostile -- and the correct answer is to use python3.

So in the end, it looks like there is simply no use for this PR at all, as it doesn't fix anything, and likely regresses the tool on Windows.

It fixes the default error message in browse.cc on macOS, as has been repeatedly pointed out to you.

In exchange, it adds a default error message in fuchsia, and

And on Windows, where there is no standard installation location for the Python interpreter, the value embedded into the binary is essentially random (it depends too much on how the developer setup their system).

in the version that hardcodes "python3" instead of "python" also regresses the tool on Windows, where Microsoft provides "python" and doesn't provide "python3". Yes, Microsoft provides a standard installation location for python.exe! It is part of the operating system. It chainloads into the Microsoft Store installable redistributable.

Honestly I get the feeling that your stance is anything that works on fuchsia is the one true way and people who have issues in other environments are imagining it.

I understand that embedding paths is occasionally awkward. My belief is that although your usual environment has this as the norm, it's rarely the norm elsewhere, since Linux, *BSD, macOS, and Windows all have canonical locations where if python3 is installed using the OS mechanism, the binary will exist in that precise path.

If the purpose of a default is to make the most users happy, it makes sense to optimize for the default system locations and expect non-default users to configure it to suit themselves.

Another approach which you haven't mentioned in your eagerness to say nothing should be done at all -- detect whether Python is available as "python3" or as "python" and set that as the default. I'm not sure how to do that in cmake. It is already what configure.py does, though.

I'm very loudly refraining from making any comment about cmake users and the build system scripts they write.

[digit-google]

It looks like changing python to python3 is only desirable on Unix at that point. A simple CMake check should be enough to do that, e.g.:

# Default value for NINJA_PYTHON depends on the target system, since there is
# no `python` on MacOS, and no `python3` on Windows.
if(WIN32)
  set(DEFAULT_NINJA_PYTHON "python")
else()
  set(DEFAULT_NINJA_PYTHON "python3")
endif()
set(NINJA_PYTHON ${DEFAULT_NINJA_PYTHON} CACHE STRING "Python interpreter to use for the browse tool")

Seems to fix ninja -t browse on MacOS for me. Before that, the command prints ninja: python is required for the browse tool.

[jhasse]

what about checking for python3 on the PATH and if it isn't fallback to python - during runtime?

[digit-google]

Because the PATH at build time may not be the same when the produced Ninja binary is actually run.

[digit-google]

Ah sorry, I misread your message. Doing runtime checks should be ok in theory. But doing that properly on all platforms is non trivial through, iirc.

Adding an option -t browse to specify the python interpreter would be another option that requires less work. The error message in case of ENOENT could be changed from ninja: python3 is required for the browse tool. to something like ninja: Could not find python3 in your PATH, use '-t browse --python <program>' option to specify the path to a Python3 interpreter.

[digit-google]

For example on Windows, you would have to check the registry to do things correctly: https://learn.microsoft.com/en-us/windows/win32/shell/app-registration?redirectedfrom=MSDN#finding-an-application-executable :-(

[eli-schwartz]

Windows is also additionally interesting because you can run py as the python command, and it will search all sorts of locations for you, including versions of python recorded in the Windows Registry as Software\Python\<Company>\<Tag>, and run the latest one for you (or a specific version via py -3 etc.).

But the "py" tool isn't installed by default, whereas "python" is. If you're going to search anything on Windows, you'd probably want to reimplement py as a builtin logic following https://peps.python.org/pep-0514/