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.

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

Add 'remove_legacy_create_init_py' flag #9271

Closed

Conversation

asafflesch
Copy link
Contributor

Does the first step detailed by @brandjon in #7386, adding a migration flag to set the value of legacy_create_init to False.

@asafflesch asafflesch requested a review from hlopko as a code owner August 28, 2019 14:07
@iirina iirina added the team-Rules-Python Native rules for Python label Aug 29, 2019
@iirina iirina requested a review from brandjon August 29, 2019 13:51
@hlopko hlopko removed their request for review September 2, 2019 14:40
@hlopko
Copy link
Member

hlopko commented Sep 2, 2019

I'm sure Jon will take good care of this PR :)

@brandjon
Copy link
Member

brandjon commented Sep 5, 2019

Thank you for this PR! The implementation looks good but I'd like to request a change to how this behavior is rolled out.

There are three levels of behavioral change to consider:

  1. Flipping the default value of legacy_create_init from true to false.
  2. Making legacy_create_init a no-op (always false regardless of what the target says).
  3. Deleting legacy_create_init (error if target tries to specify it).

This PR skips (1) and goes directly to implementing (2) but not (3). Skipping (1) means that users can't incrementally migrate their builds -- they can't temporarily opt out of the change for one or two stubborn targets, even if everything else in their build works. Skipping (3) means a separate incompatible change will still be needed just to cleanup the no-op attribute.

I suggest that instead we structure it so that this PR implements (1). Then a later change (to be flipped after Bazel 2.0!) could skip (2) and go directly to (3).

The biggest barrier I see is that migration for many builds may be cumbersome, since you have to add a __init__.py file to each Python package and include it in srcs. But in any case, we can implement this now and worry about whether migration is feasible yet later.

@asafflesch
Copy link
Contributor Author

@brandjon - I've re-implemented the change in line with your comment. I did have to change the type of "legacy_create_init" from Boolean to TriState, but the tests verify that anyone who has set the attribute using existing boolean values will still have it work.

I tried to utilize the codebase's existing "default value" mechanism instead, but the amount of private classes and methods there made me think this was not an encouraged method.

@asafflesch
Copy link
Contributor Author

@brandjon - ping.

@brandjon
Copy link
Member

Thanks for the changes. I'm merging this internally with a few tweaks:

  • Factored PyExecutable logic into a static helper method
  • Renamed the flag (and corresponding fields) to --incompatible_default_to_explicit_init_py
  • Moved tests to BazelPyBinaryConfiguredTargetTest.java

Awaiting internal code review, expect the push in a day or two.

@brandjon
Copy link
Member

FYI, downstream CI shows no problem with changing the type of legacy_create_init to tristate. (In theory someone could introspect on the attribute using native.existing_rules.)

@bazel-io bazel-io closed this in 89f8d37 Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Rules-Python Native rules for Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants