Skip to content

Conversation

jwnimmer-tri
Copy link
Contributor

@jwnimmer-tri jwnimmer-tri commented Nov 24, 2024

See bazel-contrib/rules_python#2226 for some background. There is now an exec_interpreter that we need to actively disable, or else Bazel will try to download and run something random from the internet (e.g., a Python 3.11 tarball) which fails when running as root (e.g., in CI), due to this code.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri force-pushed the module-bump branch 2 times, most recently from f18c597 to c68c916 Compare November 24, 2024 21:33
@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review November 24, 2024 21:35
@jwnimmer-tri jwnimmer-tri force-pushed the module-bump branch 2 times, most recently from c8c7ea1 to a4f0249 Compare November 24, 2024 21:48
@jwnimmer-tri
Copy link
Contributor Author

+@xuchenhan-tri we have basically no subject matter experts on Bazel toolchains. Would you be willing to make an attempt at both reviews of this PR, please?

@xuchen-han
Copy link

For sure, on it now.

Copy link

@xuchen-han xuchen-han left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: x2
I got most background info I need from https://bazel.build/extending/toolchains, but I still have one question (that the linked rules_python PR didn't quite help me understand).

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, platform LGTM from [xuchenhan-tri] (waiting on @jwnimmer-tri)


drake_bazel_download/BUILD.bazel line 13 at r1 (raw file):

py_exec_tools_toolchain(
    name = "python_no_exec_tools",
    exec_interpreter = "@rules_python//python:none",

BTW, what's missing from my understanding is what exactly is exec_interpreterand when/where it's used.

Copy link
Contributor Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, platform LGTM from [xuchenhan-tri] (waiting on @xuchenhan-tri)


drake_bazel_download/BUILD.bazel line 13 at r1 (raw file):

Previously, xuchenhan-tri wrote…

BTW, what's missing from my understanding is what exactly is exec_interpreterand when/where it's used.

It's used for precomputing the *.pyc bytecode files as part of the bazel build //... action in order to reduce startup times of python programs. If you are cross-compiling python code (where your interpreter differs between your build platform and the built platform), it might need to be different than the main toolchain.

I am not sure what / whether any of that should go into code comments here?

@xuchen-han
Copy link

drake_bazel_download/BUILD.bazel line 13 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It's used for precomputing the *.pyc bytecode files as part of the bazel build //... action in order to reduce startup times of python programs. If you are cross-compiling python code (where your interpreter differs between your build platform and the built platform), it might need to be different than the main toolchain.

I am not sure what / whether any of that should go into code comments here?

Ok, AFAICT this is a new feature that rules_python introduced that we don't need but is hurting CI. Probably making that clear is helpful in the comment.

As I was reading the change I was wondering: "we disabled something; would that cause something to fail?". From what I understand now, the answer is no.

Upgrade rules_cc to latest 0.0.x release 0.0.17.

Upgrade rules_python to latest release 0.40.0; this requires some
additional toolchain shenanigans.
Copy link
Contributor Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, platform LGTM from [xuchenhan-tri] (waiting on @xuchenhan-tri)


drake_bazel_download/BUILD.bazel line 13 at r1 (raw file):

Previously, xuchenhan-tri wrote…

Ok, AFAICT this is a new feature that rules_python introduced that we don't need but is hurting CI. Probably making that clear is helpful in the comment.

As I was reading the change I was wondering: "we disabled something; would that cause something to fail?". From what I understand now, the answer is no.

I've expanded the top-of-file comment to hopefully help.

Copy link

@xuchen-han xuchen-han left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [xuchenhan-tri] (waiting on @jwnimmer-tri)


drake_bazel_download/BUILD.bazel line 13 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I've expanded the top-of-file comment to hopefully help.

Thanks.

@jwnimmer-tri jwnimmer-tri merged commit 6445188 into RobotLocomotion:main Nov 25, 2024
7 of 9 checks passed
@jwnimmer-tri jwnimmer-tri deleted the module-bump branch November 25, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants