Some tests still require a `python` binary (Python 2)

fweikert commented 1 year ago

Description of the bug:

On our new M1 Mac Studio CI workers the following tests fail due to Python 2 no longer being installed by default:

//src/test/shell/bazel:bazel_example_test fails with io_bazel/src/test/shell/bazel/bazel_example_test: line 135: python: command not found log
//src/test/py/bazel:runfiles_test fails with Could not find python binary: python log

Obviously it would be easy to fix them by installing Python 2. However, I'd argue that the right thing is to switch to Python 3 given that it's 2022.

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

No response

Which operating system are you running Bazel on?

MacOS monterey 12.6 (Bazel CI)

What is the output of `bazel info release`?

5.3.2

If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.

No response

What's the output of `git remote get-url origin; git rev-parse master; git rev-parse HEAD` ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

fweikert commented 1 year ago

Some more failing tests:

fweikert commented 1 year ago

Most (all?) tests are actually PY3 compatible, i.e. a symlink to the python3 binary solved most of the problems.

//src/test/shell/bazel:python_version_test is the only remaining failing test since it requires a python2 binary.

fweikert commented 1 year ago

//src/test/py/bazel:runfiles_test is failing, too: logs

rickeylev commented 1 year ago

FYI: I recently (~Jan 2023) did some work cleaning out Python 2 in Bazel. The aforementioned tests are probably OK now (several look familiar from what I cleaned up). You should also be able to set --incompatible_python_disable_py2=true (with a recent bazel build) to fail earlier when a py2 target is detected.

larsrc-google commented 1 year ago

FWIW, several tests are failing this way today: https://buildkite.com/bazel/google-bazel-presubmit/builds/63119#01861694-4c8b-4d10-8232-67d46c3390cb

fweikert commented 1 year ago

Could not find python binary:

//src/test/shell/bazel/android:android_instrumentation_test_integration_test_with_platforms: log
//src/test/shell/bazel/android:android_instrumentation_test_integration_test: log
//src/test/shell/bazel/android:android_instrumentation_test_integration_test_with_head_android_tools: log
//src/test/py/bazel:runfiles_test: log

Error occurred while attempting to use the default Python toolchain (@rules_python//python:autodetecting_toolchain).\nNeither 'python3' nor 'python' were found on the target platform's PATH:

//tools/python:pywrapper_test: log

rickeylev commented 1 year ago

For runfiles_test: The problem is the test is specifying --incompatible_use_python_toolchains=false. This makes it fallback to try --python_top, which is empty, so then it trie --python_path, which defaults to python (the flag default is null, some Java codes computed this upon access). Removing that flag looks to make things work. Stated another way: this is probably due to tests disabling Python toolchains for some reason; some comments indicate this was done because python3 wasn't yet available on macs (somewhat ironic, given the situation is reversed now lol)

I'm pretty sure all the "Could not find python binary: python" errors are this same cause. From what I remember, the above behavior is the only way to get just "python" as the interpreter name to use. I found the spot in runfiles_test doing this and will prepare a PR. I haven't grepped the other tests for incompatible_use_python_toolchains yet.

The "Neither python or python3 was found on PATH" error is different; that will occur when toolchains are enabled. This is probably pywrapper_test doing weird setup; some of what it does is override PATH to run subprocesses to test logic.

rickeylev commented 1 year ago

For pywrapper_test, this is some sort of issue with copying the which binary to another location. The basic gist of this test is this:

mkdir /tmp/testdir
cp /usr/bin/which /tmp/testdir
touch /tmp/testdir/python
cd /tmp/testdir
PATH=/tmp/testdir which python

When copied to another location, the which binary simply fails with Killed: 9. The only mention I found about this behavior is in https://apple.stackexchange.com/questions/258623/how-to-fix-killed-9-error-in-mac-os, which says something about the OS not liking the binary being elsewhere and failing if so.

In any case, this shouldn't be too hard to work around in the test. I'll prep a PR.

rickeylev commented 1 year ago

Looks like src/test/shell/bazel/remote_helpers.sh is another culprit using python (this time directly as part of a shell script, not via build rules). The file it's running looks like it's py3 compatible, so it probably just needs to be sed/awk'd to use python3

rickeylev commented 1 year ago

I wasn't able to get the android stuff working and I have to put this down for now, so won't be sending any more PRs on this for at least a few weeks. That said:

I did some grepping and saw some places where --incompatible_use_python_toolchains is being disabled. Removing that flag will probably Just Work. If not, it's fairly easy to define a custom toolchain that works for the tests (define a py_runtime(), define a py_runtime_pair() pointing to the runtime, define a toolchain() pointing to the pair, then use --extra_toolchains to point to the toolchain; see tools/python/toolchains internally for some examples, or contact me and I can help walk through it)

The //src/test/shell/bazel:starlark_repository_test test might be fixed now; I recall seeing remote_helpers mentioned in its failures; the previous commit might have fixed that.

meteorcloudy commented 1 year ago

@rickeylev @fweikert Are there any more tests left broken due to this issue?

meteorcloudy commented 1 year ago

Oh, I guess:

"-//src/test/shell/bazel/android:android_instrumentation_test_integration_test"
"-//src/test/shell/bazel/android:android_instrumentation_test_integration_test_with_head_android_tools"
"-//src/test/shell/bazel/android:android_instrumentation_test_integration_test_with_platforms"

github-actions[bot] commented 3 months ago

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

github-actions[bot] commented 1 week ago

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please post @bazelbuild/triage in a comment here and we'll take a look. Thanks!

bazelbuild / bazel