Bazel: Revamp Python 2/3 mode selection

[brandjon]

This issue tracks discussion around, and implementation of, the proposal to change the Python mode configuration state from a tri-value to a boolean.

See also #6444 to track specific features/bugs relating to the Python mode.

Design doc
Discussion mail thread

[brandjon]

I've broken out a task list, based on the API changes / backward compatibility section of the doc.

Phase 1: New API and updated configuration transition

• [x] Add new --experimental_better_python_version_mixing flag for gating the new behavior and API. (Done but reverted -- new API is now available without flag.)
• [x] Add new --python_version flag.
• [x] Add accessor/setter methods for the Python version mode, that abstract over whether or not the experimental flag is enabled.
• [x] Update rule and transition logic to use these methods.
• [x] Update the output root name function as appropriate.
• [x] Introduce a new target to select() on in place of "force_python".
• [x] Add python_version attribute.

Phase 2: srcs_version validation

• [x] Add new fields has_py2_only_sources and has_py3_only_sources to py provider.
• [x] On new semantics, validate in py_binary instead of py_library.
• [x] Provide a way to find incompatible py_library targets in transitive deps of a py_binary.

Phase 3: Deprecation

• [x] Prohibit select()-ing on "force_python" when syntax migration flag is enabled.
• [x] Make this not break existing_rules() (see #7071)

Phase 4: Rollout

• [x] Document version behavior for rules/attributes and version flag
• [x] Document the incompatible change flags and migration process
• [x] Rename the disabling flag from "experimental" to "incompatible"
• [x] Flip incompatible flags

[brandjon]

We ran into a snag during implementation. There's no existing mechanism to inject new behavior into select()s over native flag names, so we can't make select()s on "force_python" conditionally defer to "python_version" when the experimental flag is enabled. This isn't surprising. But it looks like there used to be functionality for this called "late bound options" that was recently removed, so it sounds like my plan to add an extra level of dispatching may go against the configurability team's wishes.

On consultation with @serynth, what we can do instead is ask users to migrate their select()s over "force_python" to instead use a standard feature flag that we provide. The feature flag can then change its value based on the underlying cmdline flags, so any refactoring is transparent from the user's perspective. This would mean that when migrating for --experimental_better_python_version_mixing, you should also update select()s.

I'll update the design doc with both the feature flag definition and the impact on migration path.

[brandjon]

Plan of record is to add @bazel_tools//python:python_version to select on (via config_setting's flag_values attribute). Selecting on "force_python"/"python_version" will be disallowed as part of the incompatible change flag to come.

[brandjon]

We're changing the migration plan slightly, will update this thread soon.

Also ran into a blocker where the migration plan breaks some use cases of native.existing_rules(). Filed #7071.

[brandjon]

See updates to the design doc.

Due to #7071, we can't use a flag to remove an attribute based on whether or not the attribute was explicitly set. But we need to guard default_python_version with the incompatible change flag. So instead we'll have the attribute default to a sentinel value and check for the sentinel. For the python_version attribute, we won't bother with guarding it with an experimental flag, we'll just make it available immediately since the flag would be very short-lived anyway.

The other issue is how to report to the user when a py_library in the transitive deps of a py_binary has a bad srcs_version. The provider just propagates a boolean, so that doesn't tell you which transitive dep caused the problem. There's a few strategies:

1. Have the provider propagate a depset of all targets with the constraint, so the user can see everything in the transitive closure that's causing a problem. This feels like overkill.

2. Have the provider propagate a representative target with the constraint, e.g. the top-most left-most. I was going to do this, but it's ugly to document and describe, so I'd rather it not be a public interface.

3. Grab that representative target using an aspect. Then the error message for a bad srcs_version would instruct the user to rerun the command with the aspect. The problem is that aspects can't run on failed configured targets, so we'd either have to add extra machinery for this kind of case or somehow apply the aspect to the direct deps instead.

4. Emit a warning whenever a py_library directly depends on a py_library with more restrictive/incompatible srcs_version, saying what the exact bad dependency is. Emit the warning with low priority (INFO?) so it won't be seen by default on the console. Have the error message give users instructions on how to retrieve it.

I'll probably go with option 4.

[brandjon]

I'll probably go with option 4.

You know, there's a few problems here too. We can't just emit the warning whenever the current target has srcs_version = "PY3" and a dep with has_py2_only_sources = True, because the provider field propagates to every Python reverse-dep. A single problem with a deep dependency would generate warning spam all the way up the dependency chain. We could compare our srcs_version against our direct dependency's srcs_version in order to only report it at the point where the first offending dependency occurs. That would itself require an extra provider field.

Maybe we should go with the aspect approach after all, and instruct the user to run it on all the py_binary's direct dependencies since running it on the py_binary itself isn't possible.

[brandjon]

At this point the feature is fully implemented (to be in 0.23), and this issue is now blocked on migration and flipping the incompatible change flags (#7307, #7308), then cleanup of the legacy code paths.

[brandjon]

This should be closed imminently. Flipped the syntax flag today, and the semantics one tomorrow. Will be released in 0.25.

I'll file a separate bug to track cleaning up the legacy code paths / flag.

[brandjon]

Woohoo! Both flags have been flipped and this'll be in 0.25.

[clintharrison]

If this is being flipped in 0.25, can we try to get e84bb330a2f6624d2faabcc912f366b50427f0bf in 0.25 as well?

[brandjon]

We try to avoid cherrypicks where it's not a bugfix for a regression. At this point 0.25 has already baked for so long that the baseline for 0.26 is about to be cut, so I wouldn't want to delay it further over this. If you want the fix sooner for diagnostic purposes you can always build bazel at head.