fix: venv should require exec toolchain

[thesayyn]

Type of change

• Bug fix (change which fixes an issue)

Test plan

• New test cases added

• TODO: add test

[thesayyn]

I'll talk about this with @alexeagle tmr

[alexeagle]

I think there's some useful feedback on the slack thread, let's see if someone like @fmeum can provide a more thorough and correct review than I can

[fmeum]

This is very interesting. I will need to think this through a bit more before I can provide meaningful comments.

[fmeum]

This appears to be a pretty complicated workaround for an underlying issue with toolchain registration and I am not sure it will work properly in cross-platform setups.

I noticed that your primary toolchain definition at

rules_py/py/private/toolchain/autodetecting.bzl

Line 43 in 218a202

name = "py_toolchain",

lacks a target_compatible_with attribute. With that added, you could presumably just define a py_binary target that serves as the venv tool and add that as an exec dep of the venv rule.

It's possible that I'm missing something and this requires a more complex solution, but a toolchain without constraints is almost never correct.

[thesayyn]

eh this bit me again, didn't know this was already here...

[thesayyn]

It's possible that I'm missing something and this requires a more complex solution, but a toolchain without constraints is almost never correct.

good point.

you could presumably just define a py_binary target that serves as the venv tool and add that as an exec dep of the venv rule.

wouldn't this almost be same as what I do right now with getting the python toolchain for the exec but a level more indirection?

Also, i presume auto exec groups ought to fix this, maybe correct solution is to wait for that?

[fmeum]

It's similar, but avoids having to introduce a current_runtime rule, which ends up hiding the actual source of a toolchain requirement and thus makes it harder to debug toolchain resolution issues.

I don't know auto exec groups well enough to assess whether they could help here.

[alexeagle]

We'll solve for this by introducing a new toolchain pair, since we actually need to run a different command in the exec cfg than we do in the target cfg. See pycrust branch.