Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Guarding an experimental attr with a flag breaks native.existing_rules() #7071

Closed
brandjon opened this issue Jan 9, 2019 · 1 comment
Closed

Guarding an experimental attr with a flag breaks native.existing_rules() #7071

brandjon opened this issue Jan 9, 2019 · 1 comment
Assignees
@brandjon
Labels
P1 I'll work on this now. (Assignee required) team-Rules-Python Native rules for Python type: bug

Comments

@brandjon
Copy link
Member

brandjon commented Jan 9, 2019

python_version is guarded by an experimental flag (currently --experimental_better_python_version_mixing, but that may be renamed). The rule logic checks if that attribute is set explicitly by the user, and if so, fails if the experimental flag isn't enabled.

We theoretically need the experimental flag in order to comply with our breaking change policy, otherwise if the attribute is available without the flag it would be considered a supported API.

But this breaks users who use native.existing_rules() to programmatically copy targets. When a target is copied, the information about whether an attribute is set explicitly or by default is lost. This means that copying any py_binary or py_test target will fail when the experimental flag is not present.
@brandjon brandjon added type: bug P1 I'll work on this now. (Assignee required) team-Rules-Python Native rules for Python labels Jan 9, 2019
@brandjon brandjon self-assigned this Jan 9, 2019
@brandjon brandjon mentioned this issue Jan 9, 2019
Closed
@brandjon
Copy link
Member Author

Resolution is to not guard the new attribute with a flag, but we still have a similar problem for the flag that removes the old attribute.

bazel-io pushed a commit that referenced this issue Jan 14, 2019
@brandjon
Rework flags for Python version migration
2483c11 
This CL implements the bulk of the changes to the migration section of the design doc, described by PR bazelbuild/rules_python#153. A subsequent CL will do the flag rename from --experimental_better_python_version_mixing to --experimental_allow_python_version_transitions.

The main changes in this CL are:

  1. Instead of one experimental/incompatible flag, we have two: one for syntax and one for semantics.

  2. The new API (--python_version flag and python_version attribute) is no longer guarded by an experimental flag, but rather is unconditionally available.

  3. The old API (--force_python flag and default_python_version attribute) is now disabled by an experimental flag. (This was always the plan but this CL implements it.)

The motivation for (1) is so that it's easier for users to migrate and easier for us to roll these changes out. It also simplifies some logic in PythonOptions.java and python_version.bzl, because the fallback from --python_version on to --force_python no longer cares about whether we're using the new or old semantics.

The motivation for (2) is that the current logic for gating an attribute on an experimental/incompatible flag breaks native.existing_rules(). See #7071. In this case the experimental flag would have been very short-lived anyway, since we want these features to be migration ready by February. We still have to face #7071 before we can enable the incompatible change, but at least this CL unblocks further work without hacking up existing_rules() with ad hoc blacklists.

Finally, I took this CL as an opportunity to downsize some of our shell integration tests by turning them into Java unit tests. This required updating the mocks a bit.

Work towards #6583.

RELNOTES: None
PiperOrigin-RevId: 229261233
weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
@brandjon @weixiao-huang
Make --experimental_remove_old_python_version_api not breaking existi…
459d347 
…ng_rules()

The current way of gating access to the deprecated attribute `default_python_version` is to have the rule check whether or not the attribute was set explicitly (AttributeMap#isAttributeValueExplicitlySpecified), and match that against whether or not the attribute is allowed. However, this breaks the use case of copying a rule definition programmatically via `native.existing_rules()`, because when the definition is copied all the attributes of the new target appear to be explicitly specified.

The solution is to use a sentinel value as the physical default value in the attribute definition. Then the rule just checks for equality with that sentinel. If the configuration permits use of that attribute, then the sentinel is interpreted the same as what the logical default would be.

This requires adding an item to the PythonVersion enum. Ugly, but necessary. We can remove it once the incompatible flag is flipped to remove the old API.

Work toward bazelbuild#6583. Fixes bazelbuild#7071.

RELNOTES: None
PiperOrigin-RevId: 230399577
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@brandjon brandjon

Labels
P1 I'll work on this now. (Assignee required) team-Rules-Python Native rules for Python type: bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@brandjon