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

Use real rules_python #17545

Closed
wants to merge 12 commits into from
Closed

Conversation

comius
Copy link
Contributor

@comius comius commented Feb 21, 2023

Initial motivation was to use py_proto_library from rules_python, but then a yak came along.

Fixes: #9029

@comius comius marked this pull request as ready for review February 24, 2023 12:43
@comius comius self-assigned this Feb 24, 2023
@meteorcloudy
Copy link
Member

This is great, I guess we can delete https://github.com/bazelbuild/bazel/tree/master/third_party/rules_python after merging this one?

@comius
Copy link
Contributor Author

comius commented Feb 24, 2023

Hey, @meteorcloudy,
I can't reproduce the failure in //tools/aquery_differ:aquery_differ_test locally. Could this possibly be a caching issue?

Fails:
bazel test --config=ubuntu1804_java11 -- //tools/aquery_differ:aquery_differ_test, log

Succeeds:
bazel test -- //tools/aquery_differ:aquery_differ_test

@meteorcloudy
Copy link
Member

meteorcloudy commented Feb 24, 2023

It doesn't look like a caching issue, did you try to reproduce in the docker container? This is also failing on other platforms without RBE enabled

@meteorcloudy
Copy link
Member

I can reproduce the issue in the docker container (docker run -it gcr.io/bazel-public/centos7-java11-devtoolset10) but not on my linux workstation:

[root@06cbde8452c1 bazel]# bazel test //tools/aquery_differ:aquery_differ_test
2023/02/24 13:07:54 Downloading https://releases.bazel.build/6.0.0/release/bazel-6.0.0-linux-x86_64...
Extracting Bazel installation...
Starting local Bazel server and connecting to it...
INFO: Analyzed target //tools/aquery_differ:aquery_differ_test (63 packages loaded, 1111 targets configured).
INFO: Found 1 test target...
FAIL: //tools/aquery_differ:aquery_differ_test (see /root/.cache/bazel/_bazel_root/c564d33b2d1d9ff3bd9f69877f355ccc/execroot/io_bazel/bazel-out/k8-fastbuild/testlogs/tools/aquery_differ/aquery_differ_test/test.log)
Target //tools/aquery_differ:aquery_differ_test up-to-date:
  bazel-bin/tools/aquery_differ/aquery_differ_test
INFO: Elapsed time: 52.583s, Critical Path: 14.12s
INFO: 208 processes: 7 internal, 201 processwrapper-sandbox.
INFO: Build completed, 1 test FAILED, 208 total actions
//tools/aquery_differ:aquery_differ_test                                 FAILED in 0.2s
  /root/.cache/bazel/_bazel_root/c564d33b2d1d9ff3bd9f69877f355ccc/execroot/io_bazel/bazel-out/k8-fastbuild/testlogs/tools/aquery_differ/aquery_differ_test/test.log

Executed 1 out of 1 test: 1 fails locally.
[root@06cbde8452c1 bazel]# /root/.cache/bazel/_bazel_root/c564d33b2d1d9ff3bd9f69877f355ccc/execroot/io_bazel/bazel-out/k8-fastbuild/testlogs/tools/aquery_differ/aquery_differ_test/test.log
[root@06cbde8452c1 bazel]# cat /root/.cache/bazel/_bazel_root/c564d33b2d1d9ff3bd9f69877f355ccc/execroot/io_bazel/bazel-out/k8-fastbuild/testlogs/tools/aquery_differ/aquery_differ_test/test.log
exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from //tools/aquery_differ:aquery_differ_test
-----------------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/.cache/bazel/_bazel_root/c564d33b2d1d9ff3bd9f69877f355ccc/sandbox/processwrapper-sandbox/200/execroot/io_bazel/bazel-out/k8-fastbuild/bin/tools/aquery_differ/aquery_differ_test.runfiles/io_bazel/tools/aquery_differ/aquery_differ_test.py", line 23, in <module>
    from src.main.protobuf import analysis_v2_pb2
  File "/root/.cache/bazel/_bazel_root/c564d33b2d1d9ff3bd9f69877f355ccc/sandbox/processwrapper-sandbox/200/execroot/io_bazel/bazel-out/k8-fastbuild/bin/tools/aquery_differ/aquery_differ_test.runfiles/io_bazel/src/main/protobuf/analysis_v2_pb2.py", line 5, in <module>
    from google.protobuf.internal import builder as _builder
ModuleNotFoundError: No module named 'google'

@meteorcloudy
Copy link
Member

I guess this is because I do have the protobuf pip package installed on my workstation, but not in the docker container:

pcloudy@pcloudy:~/workspace/bazel (use-py_proto_library)
$ python3
Python 3.10.9 (main, Dec  7 2022, 13:47:07) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import google
>>> exit()
pcloudy@pcloudy:~/workspace/bazel (use-py_proto_library)
$ docker exec -it 06cbde8452c1  /bin/bash
[root@06cbde8452c1 /]# python3
Python 3.6.8 (default, Nov 16 2020, 16:55:22)
[GCC 4.8.5 20150623 (Red Hat 4.8.5-44)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import google
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'google'

@meteorcloudy
Copy link
Member

Do we expect users to have protobuf pip package preinstalled when using py_proto_library?

@sgowroji sgowroji added the team-Rules-Python Native rules for Python label Feb 24, 2023
@ted-xie
Copy link
Contributor

ted-xie commented Feb 24, 2023

android-related changes LGTM (just //tools/python -> @rules_python replacement for tools/android/BUILD and tools/android/BUILD.tools). As long as presubmit passes then it should be fine.

@rickeylev
Copy link
Contributor

Do we expect users to have protobuf pip package preinstalled when using py_proto_library?

No, that's a bug that was fixed but hasn't been released yet (see bazelbuild/rules_python#1046). I'll do a 0.19 release Monday.

To workaround it in the meantime...I'm not sure there's an easy way in the mock environment. The files are there in runfiles, the path to them just has to get onto sys.path. So either manually modify sys.path at runtime, or, maybe manually depend on the proto repo's py_library target (the bug causing the missing sys.path entry is in py_proto_library, so anything coming through that won't propagate the necessary sys.path entry)

@comius
Copy link
Contributor Author

comius commented Feb 24, 2023

Thank @meteorcloudy for the analysis, it saved my day!

I found out there is a bug, in py_proto_library. I worked around it and submitted a patch to rules_python. (bazelbuild/rules_python#1091)

@comius
Copy link
Contributor Author

comius commented Feb 24, 2023

Ups, I missed comment from @rickeylev. My fix is not actually needed.

@comius comius added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Feb 24, 2023
@sgowroji
Copy link
Member

Hi @comius, Could you please guide, Whether to import third-party changes first or the other changes in the CL. Thanks!

@comius
Copy link
Contributor Author

comius commented Feb 27, 2023

Hi @comius, Could you please guide, Whether to import third-party changes first or the other changes in the CL. Thanks!

Hey, I didn't realize before, but the import is more complex than I thought. I'll prepare separate parts for it.

@copybara-service copybara-service bot closed this in 78729c0 Mar 3, 2023
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Mar 6, 2023
comius added a commit to comius/rules_python that referenced this pull request Apr 17, 2023
rickeylev referenced this pull request Apr 18, 2023
This changes the default to rejecting Python 2 values. For more
information, see #17293

Fixes #17293

RELNOTES[INC]: --incompatible_python_disable_py2 is flipped to true. See #17293 for details.

PiperOrigin-RevId: 507897246
Change-Id: I67cbadbb0f543d8153ad0355af22e48ead9083ef
werkt added a commit to werkt/bazel-buildfarm that referenced this pull request Apr 22, 2023
Since bazelbuild/bazel#17545, bazel_tools apparantly requires
rules_python and rules_proto to successfully parse under bazel (load
lines only). We only use affected bazel_tools (src/main/protobuf) in
persistentworkers. Import these repositories to reinstate downstream CI
passing.
werkt added a commit to werkt/bazel-buildfarm that referenced this pull request Apr 22, 2023
Since bazelbuild/bazel#17545, bazel_tools apparantly requires
rules_python and rules_proto to successfully parse under bazel (load
lines only). We only use affected bazel_tools (src/main/protobuf) in
persistentworkers. Import these repositories to reinstate downstream CI
passing.

Ignore rules_oss_audit setup and invoke install_deps omitted from it in
a generated.bzl (move maven here as well). Relocated all WORKSPACE-scope
invocations to deps/defs/generated.
werkt added a commit to werkt/bazel-buildfarm that referenced this pull request Apr 22, 2023
Since bazelbuild/bazel#17545, bazel_tools apparantly requires
rules_python and rules_proto to successfully parse under bazel (load
lines only). We only use affected bazel_tools (src/main/protobuf) in
persistentworkers. Import these repositories to reinstate downstream CI
passing.

Ignore rules_oss_audit setup and invoke install_deps omitted from it in
a generated.bzl (move maven here as well). Relocated all WORKSPACE-scope
invocations to deps/defs/generated.
rickeylev pushed a commit to bazelbuild/rules_python that referenced this pull request Apr 24, 2023
…1173)

The file was removed in Bazel@HEAD in
bazelbuild/bazel#17545
This fixes failures when using rules_python with Bazel@HEAD.

Addresses: bazelbuild/bazel#17874
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
Initial motivation was to use py_proto_library from rules_python, but then a yak came along.

Fixes: bazelbuild#9029

Closes bazelbuild#17545.

PiperOrigin-RevId: 513834100
Change-Id: I11a99381e1169a9fb7a7a3eaa733ddd348ebac2b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-Python Native rules for Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Bazel itself depend on @rules_python
5 participants