fix(toolchain): disable exec toolchain by default

[aignas]

This makes the exec tools toolchain disabled by default to prevent toolchain resolution
from matching it and inadvertently pulling in a dependency on the hermetic runtimes.
While the hermetic runtime wouldn't actually be used (precompiling is disabled
by default), the dependency triggered downloading of the runtimes, which breaks
environments which forbid remote downloads they haven't vetted (such a case is
Bazel's own build process).

To fix this, a flag is added to control if the exec tools toolchain is enabled or not.
When disabled (the default), the toolchain won't match, and the remote dependency isn't
triggered.

Fixes #1967.

[aignas]

FYI @fmeum, @meteorcloudy, I think this should fix the dependence on the hermetic toolchain, but I did not have time to check with bazelbuild/bazel#22718 yet and I am not sure when I'll get to it. You could check the toolchain dependencies upon the failing targets like I described in #1967 if you would like confirmation that this was indeed a cause.

[aignas]

I think my toolchain-fu ends here, I am not sure how to create a toolchain that is only compatible with py_library targets that have enabled pre-compilation. The global string_flag usage is good if we want to force the toolchain dependency on others based on the flag. If we include auto in the is_precompile_enabled, then users will need to set the string flag to disabled in order to not pull the hermetic toolchain.

Maybe the right way to fix this is to actually have an autodetecting toolchain for the py compilation? We could have a noop compilation toolchain that never compiles anything.

[fmeum]

Maybe the right way to fix this is to actually have an autodetecting toolchain for the py compilation? We could have a noop compilation toolchain that never compiles anything.

That sounds like the right approach to me!

[rickeylev]

I'm going to continue discussion on the issue. I'm not sure there's any particularly great solution if what's happening is what I think it is. I'll post more on the issue.

[aignas]

Thanks to @rickeylev commenting in #1967, I think we have a workable solution moving forward:

$ bazel cquery --@rules_python//python/config_settings:precompile_toolchain=enabled 'deps(@dev_pip//charset_normalizer)' --transitions=lite --extra_toolchains=@rules_python//python/autodetecting_toolchain:all | grep "toolchain dependency"

Computing main repo mapping:
Loading:
Loading: 0 packages loaded
WARNING: Build options --//python/config_settings:precompile_toolchain and --extra_toolchains have changed, discarding analysis cache (this can be expensive, see https://bazel.build/advanced/performance/iteration-speed).
Analyzing: target @@_main~pip_internal~dev_pip//charset_normalizer:charset_normalizer (0 packages loaded, 0 targets configured)
INFO: Analyzed target @@_main~pip_internal~dev_pip//charset_normalizer:charset_normalizer (2 packages loaded, 1537 targets configured).
INFO: Found 1 target...
  [toolchain dependency]#//python/autodetecting_toolchain:_autodetecting_py3_exec_toolchain#NoTransition ->
  [toolchain dependency]#//python/autodetecting_toolchain:_autodetecting_py_runtime_pair#NoTransition ->
INFO: Elapsed time: 1.120s, Critical Path: 0.00s
INFO: 0 processes.
INFO: Build completed successfully, 0 total actions
aignas@panda ~/src/github/aignas/rules_python fix/precompile-toolchain-dep
$ bazel cquery 'deps(@dev_pip//charset_normalizer)' --transitions=lite --extra_toolchains=@bazel_tools//tools/python:autodetecting_toolchain | grep "toolchain dependency"

Computing main repo mapping:
Loading:
Loading: 0 packages loaded
WARNING: Build options --//python/config_settings:precompile_toolchain and --extra_toolchains have changed, discarding analysis cache (this can be expensive, see https://bazel.build/advanced/performance/iteration-speed).
Analyzing: target @@_main~pip_internal~dev_pip//charset_normalizer:charset_normalizer (0 packages loaded, 0 targets configured)
INFO: Analyzed target @@_main~pip_internal~dev_pip//charset_normalizer:charset_normalizer (0 packages loaded, 1527 targets configured).
INFO: Found 1 target...
  [toolchain dependency]#@bazel_tools//tools/python:_autodetecting_py_runtime_pair#NoTransition ->
INFO: Elapsed time: 0.600s, Critical Path: 0.00s
INFO: 0 processes.
INFO: Build completed successfully, 0 total actions
aignas@panda ~/src/github/aignas/rules_python fix/precompile-toolchain-dep
$ bazel cquery --@rules_python//python/config_settings:precompile_toolchain=enabled 'deps(@dev_pip//charset_normalizer)' --transitions=lite | grep "toolchain dependency"

Computing main repo mapping:
Loading:
Loading: 0 packages loaded
WARNING: Build options --//python/config_settings:precompile_toolchain and --extra_toolchains have changed, discarding analysis cache (this can be expensive, see https://bazel.build/advanced/performance/iteration-speed).
Analyzing: target @@_main~pip_internal~dev_pip//charset_normalizer:charset_normalizer (0 packages loaded, 0 targets configured)
INFO: Analyzed target @@_main~pip_internal~dev_pip//charset_normalizer:charset_normalizer (0 packages loaded, 3888 targets configured).
INFO: Found 1 target...
  [toolchain dependency]#@@_main~python~python_3_11_x86_64-unknown-linux-gnu//:py_exec_tools_toolchain#NoTransition ->
  [toolchain dependency]#@@_main~python~python_3_11_x86_64-unknown-linux-gnu//:python_runtimes#NoTransition ->
INFO: Elapsed time: 0.692s, Critical Path: 0.00s
INFO: 0 processes.
INFO: Build completed successfully, 0 total actions

So the latest iteration adds:

• Moved the toolchain lookup down the call stack, but observed no difference,
  not sure if we should keep it.
• autodetecting_toolchain for the precompile.
• config setting to explicitly enable precompile toolchains.
• updates the docs.

[rickeylev]

Thanks for this PR. I think this has the core of what we have to do -- add a constraint on the toolchain to prevent it from matching.

I'm going to rename the flag from precompile_toolchain to enable_exec_tools_toolchain (the toolchain isn't just for precompiling).

I'm going to remove the autodetecting toolchain impl, too. The impl itself looks fine, I just don't want to continue calling it "autodetecting", since it doesn't actually automatically do anything or detect anything.

[rickeylev]

I can't figure out what the conditions:default branch here is doing.

It's saying...

If the --precompile_toolchain=enabled, then use target_settings for the toolchain.

Otherwise, use --precompile_toolchain=enabled for the target settings. i.e. this toolchain will only match if --precompile_toolchain=enabled, but that's covered by the condition above.

is this a copy/paste error?

[aignas]

It is not, I whish I could write:

target_settings = select({
    is_precompile_enabled: target_settings,
    "//conditions:default": ["@platforms//:incompatible"],
})

However, that does not work, or maybe I am missing something. The code above is the simplest way to have a list of target_settings that is not empty and that will ensure that nothing will be matched.

[rickeylev]

Huh. Yeah, that code is exactly what I was going to rewrite it to. It doesn't work? What error does it give?

[rickeylev]

Idea: maybe because @platforms//:incompatible is a constraint-thing (from the constraint() rule, which are like...a different subtly of constraint that config settings 🤷) instead of config_setting? I'll give that a try.

[rickeylev]

Ah. I see. That's annoying. target_settings apparently can only accept config_settings, so the incompatible target can't be used. Meanwhile, target_compatible_with can't be used because its nonconfigurable.

So I get the trick the original code was doing. Clever!

[rickeylev]

Updated it as I described earlier. LGTM

[rickeylev]

And just to verify: I ran the cquery commands with the flag enabled/disabled and confirmed the hermetic runtime dependency isn't there by default