Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some tests still require a python binary (Python 2) #16526

Closed
fweikert opened this issue Oct 21, 2022 · 14 comments
Closed

Some tests still require a python binary (Python 2) #16526

fweikert opened this issue Oct 21, 2022 · 14 comments
Labels
macos-infra-update P3 We're not considering working on this, but happy to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Rules-Python Native rules for Python type: bug

Comments

@fweikert
Copy link
Member

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 fweikert added the m1-ci label Oct 21, 2022
@fweikert fweikert changed the title //src/test/shell/bazel:bazel_example_test and //src/test/py/bazel:runfiles_test still require Python 2 Some tests still require Python 2 Oct 24, 2022
@fweikert
Copy link
Member Author

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 fweikert changed the title Some tests still require Python 2 Some tests still require a python binary (Python 2) Oct 25, 2022
@comius comius added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed untriaged labels Nov 17, 2022
@fweikert
Copy link
Member Author

fweikert commented Dec 9, 2022

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

@rickeylev
Copy link
Contributor

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
Copy link
Contributor

@fweikert
Copy link
Member Author

fweikert commented Feb 3, 2023

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

copybara-service bot pushed a commit that referenced this issue Feb 3, 2023
This change should unblock both pre- and post-submit.

Related to #16526, #17407, #17408 and #17409.

PiperOrigin-RevId: 506882975
Change-Id: If704c398546265eb22d58906eb28363b30ac00a2
@rickeylev
Copy link
Contributor

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
Copy link
Contributor

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.

coeuvre pushed a commit to coeuvre/bazel that referenced this issue Feb 6, 2023
This change should unblock both pre- and post-submit.

Related to bazelbuild#16526, bazelbuild#17407, bazelbuild#17408 and bazelbuild#17409.

PiperOrigin-RevId: 506882975
Change-Id: If704c398546265eb22d58906eb28363b30ac00a2
coeuvre pushed a commit to coeuvre/bazel that referenced this issue Feb 6, 2023
This change should unblock both pre- and post-submit.

Related to bazelbuild#16526, bazelbuild#17407, bazelbuild#17408 and bazelbuild#17409.

PiperOrigin-RevId: 506882975
Change-Id: If704c398546265eb22d58906eb28363b30ac00a2
ShreeM01 pushed a commit that referenced this issue Feb 6, 2023
* Bazel CI: Disable tests that are failing due to infra upgrades.

This change should unblock both pre- and post-submit.

Related to #16526, #17407, #17408 and #17409.

PiperOrigin-RevId: 506882975
Change-Id: If704c398546265eb22d58906eb28363b30ac00a2

* Bazel CI: Disable more tests that are likely failing due to infra updates.

Related to #17410 and #17411.

PiperOrigin-RevId: 506903607
Change-Id: Ic88de4caea5c14336774e53a9063a6beb260d515

* Disable python_version_test

---------

Co-authored-by: Googler <[email protected]>
copybara-service bot pushed a commit that referenced this issue Feb 6, 2023
This fixes the failures on MacOS after the recent upgrade to CI.

When `//src/test/py/baze:runfiles_test` sets
`--incompatible_use_python_toolchains=false`, it eventuallys tries to search
`PATH` for `python`. This is because it falls back to the `--python_path`
flag, which has a computed default of "python". Since new Mac versions no
longer provide Python 2, this doesn't work.

To fix, enable Python toolchains. Comments indicate they were only disabled
because Python 3 wasn't available on the Mac CI machines at the time.

For pywrapper_test: this was failing because it copies system utilities (e.g
`/usr/bin/which`) to another location and tries to run them. For newer MacOS
versions, this results in a failure due AMFI (a security mechanism, see
https://theevilbit.github.io/posts/amfi_launch_constraints/)

To fix, instead of copying the binary, write a wrapper that re-execs the
original binary.

Work towards #16526, #8169

PiperOrigin-RevId: 507526814
Change-Id: Ifaacc30cb155af30af606254eb7ffcd9304478f6
@rickeylev
Copy link
Contributor

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

copybara-service bot pushed a commit that referenced this issue Feb 8, 2023
The new Mac images no longer have `python` (Python 2)

Work towards #16526

PiperOrigin-RevId: 508136299
Change-Id: Ib4251081f79ac8deb48ce2f961527bb8de820b12
@rickeylev
Copy link
Contributor

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.

hvadehra pushed a commit that referenced this issue Feb 14, 2023
This change should unblock both pre- and post-submit.

Related to #16526, #17407, #17408 and #17409.

PiperOrigin-RevId: 506882975
Change-Id: If704c398546265eb22d58906eb28363b30ac00a2
hvadehra pushed a commit that referenced this issue Feb 14, 2023
This fixes the failures on MacOS after the recent upgrade to CI.

When `//src/test/py/baze:runfiles_test` sets
`--incompatible_use_python_toolchains=false`, it eventuallys tries to search
`PATH` for `python`. This is because it falls back to the `--python_path`
flag, which has a computed default of "python". Since new Mac versions no
longer provide Python 2, this doesn't work.

To fix, enable Python toolchains. Comments indicate they were only disabled
because Python 3 wasn't available on the Mac CI machines at the time.

For pywrapper_test: this was failing because it copies system utilities (e.g
`/usr/bin/which`) to another location and tries to run them. For newer MacOS
versions, this results in a failure due AMFI (a security mechanism, see
https://theevilbit.github.io/posts/amfi_launch_constraints/)

To fix, instead of copying the binary, write a wrapper that re-execs the
original binary.

Work towards #16526, #8169

PiperOrigin-RevId: 507526814
Change-Id: Ifaacc30cb155af30af606254eb7ffcd9304478f6
hvadehra pushed a commit that referenced this issue Feb 14, 2023
The new Mac images no longer have `python` (Python 2)

Work towards #16526

PiperOrigin-RevId: 508136299
Change-Id: Ib4251081f79ac8deb48ce2f961527bb8de820b12
@meteorcloudy
Copy link
Member

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

@meteorcloudy
Copy link
Member

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"

Copy link

github-actions bot commented Jun 8, 2024

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 github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Jun 8, 2024
Copy link

github-actions bot commented Sep 6, 2024

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!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macos-infra-update P3 We're not considering working on this, but happy to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Rules-Python Native rules for Python type: bug
Projects
None yet
Development

No branches or pull requests

6 participants