(bzlmod): users have no way to opt out of hermetic toolchain downloads due to precompiling toolchain #1967
Comments
|
I might try the idea 2. first as that simplifies the code in general. EDIT: It seems that the full answer may be more nuanced because the toolchain debug log shows: The important lines are: It seems that we resolve 2 toolchains, one for |
|
Yeah, I think my suspicion is correct, see: It seems that because the toolchain for python file compilation exists, we'll depend on it and there is no way to get rid of it in the dependency graph other than:
|
With this change we set the `target_settings` to something that is not going to be ever matched if the `precompile` flag is not enabled. Users wanting to use the `precompile` feature will need to set the config_setting value to `enabled` instead of `auto`. Fixes #1967
With this change we set the `target_settings` to something that is not going to be ever matched if the `precompile` flag is not enabled. Users wanting to use the `precompile` feature will need to set the config_setting value to `enabled` instead of `auto`. Fixes #1967
aignas
changed the title
|
I need to verify what I say below, but with the logs aignas posted, I'm pretty sure this is what's happening.
Yeah, I agree with your analysis and expectation. The behavior I expect to see is: if precompiling isn't enabled, then the exec toolchain shouldn't be resolved, because those actions are never registered. In case it wasn't clear, the the exec_tools toolchain has its own reference to an interpreter. So this is where it's getting the hermetic runtime from. This is so that it has an exec-config interpreter (and is thus valid to run in an action) that has a Python version matching the target-config constraints. Removing the exec-config-interpreter from the exec_tools toolchain would be fine with me, however, I'm not sure if there's another way to get the correctly configured interpreter. Ideally, whatever binary the exec-config precompiler attribute points to would lookup the regular target-platform toolchain and use that, but I'm not sure if the constraints propagate correctly through such a relationship (if the precompiler is supposed to generate e.g. 3.11 pyc files, we need to make sure the 3.11 constraint is carried over to its toolchain resolution).
Yeha, I like this idea, too. Telling people to do e.g.
This idea has some appeal as an interim fix if the ideas below don't pan out. A variation of this idea would be a separate flag that controls registration instead of the extra config_setting hoops the PR jumps through. Precompiling is a new feature you have to opt-into anyways. A few ideas to look into: First idea: in common_bazel.bzl, the first thing it does is an exec tools toolchain lookup. Maybe only doing that access if precompiling is going to happen will fix the issue? The docs about auto exec groups gave me the impression that something magical was happening where AEGs caused Bazel to defer the actual resolution normally caused by Next idea: maybe any access of |
With this we can finally be correct with how we deal with the precompile toolchains. This is done in two ways: 1. Add a config_setting that explicitly enableds/disables the precompile toolchains. 2. Add an autodetecting toolchain for precompile in cases where users may want to use the autodetecting toolchain and we would like to ensure that the hermetic toolchain is not pulled in because it exists and is used in the target. To keep the behaviour backwards compatible, add an alias to the autodetecting toolchain with a deprecation notice. The API is new in 0.33.0 and it is unfortunate that we have to do this, but the users should still be able to use the code as is. Fixes #1967.
|
Thanks @rickeylev for the thorough analysis, I updated #1968 with:
I am curious if you would like to leave the ticket open to look into AEGs or if we should create a separate ticket here. |
|
Argh, yeah. It looks it doesn't matter if a rule uses the toolchains or not -- simply having them listed attempts resolving them, even if they're optional. This behavior makes some sense, but is kind of annoying in this case. In this case, we essentially have an erroneously defined optional toolchain, and are fine with pretending it doesn't exist. I tried using an exec group for the toolchain, but the behavior is the same. Whether the toolchain is used or not, it gets resolved. This is surprising and seems wrong? Exec groups are tied to actions, so if there aren't any actions triggering it, then resolving it is wasted effort. Ah, well, this behavior is out of our control, so just gotta roll with it. So yeah, the only trick I can think of is for the toolchain to be incompatible (resolution is still happening, it just doesn't traverse into the toolchain implementation, so the details of the toolchain implementation don't get resolved). I'm going to see if I can make the exec tools intepreter re-use the target toolchain interpreter. If that doesn't look promising, we'll go with the flag approach. Another thing I noticed is the |
|
I prototyped an alternative that looks promising. Basically, a cfg=exec attribute that looks up the target toolchain and returns it. This avoids the exec toolchain needing its own reference to an interpreter. I'll clean it up tonight and see how it looks. |
|
Ah, dang. Well my idea works to allow the exec_tools toolchain to use the target toolchain to find the runtime (the net effect being, |
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. --------- Co-authored-by: Richard Levasseur <rlevasseur@google.com>
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. Cherry-pick of cf1f36d. --------- Co-authored-by: Richard Levasseur <rlevasseur@google.com>
It seems that users will download the automatically registered precompile toolchain using the hermetic interpreter even if they would like to use the autodetecting toolchain.
For analysis see comments below and the attached PR for a possible beginning of a fix.
Initial description: The logic change in
python/private/py_toolchain_suite.bzlin #1837 probably results in theautodetectingtoolchain not being selected.The toolchain registration is based on the target_settings and constraint_values. If those are the same, then the last registered toolchain overwrites the previously added one. In bazelbuild/bazel#22718 there is usage of an extra toolchain via:
This means that in the previous logic (which would be sometimes incorrect) the autodetecting toolchain would overwrite any of the default toolchain that was registered by
rules_pythonin the default case.With the change, we now have 2 toolchains that match the default case when the
python_versionconfig setting value is unset - the hermetic and the autodetecting one and the hermetic one is more specialized.Fixing this without reverting idea 1
A likely fixed could be to auto-register the auto-detecting toolchain and have a config flag that allows the user to select the autodetecting toolchain explicitly over the hermetic toolchain and to fallback to the auto-detecting toolchain if allowed and if no toolchain matches.
Fixing this without reverting idea 2
Use the rules_python_config or the python extension to write data so that the default
python_versionsetting can be set instead. That way all python toolchains would have a version and there would be no special case of a hermetic toolchain which matches""config setting.The text was updated successfully, but these errors were encountered: