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

Update on incompatible change process #16195

Closed
wants to merge 4 commits into from
Closed

Update on incompatible change process #16195

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b026361
Update on incompatible change process
meteorcloudy Aug 31, 2022
41a586c
Update site/en/contribute/breaking-changes.md
meteorcloudy Aug 31, 2022
7790838
Better formatting
meteorcloudy Aug 31, 2022
65038e8
Adding removing flag section and mention breaking-change-x.0 label
meteorcloudy Sep 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 28 additions & 27 deletions site/en/contribute/breaking-changes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ change in Bazel to adhere to this policy.

1. [Flip the incompatible flag.](#flip-flag)


## GitHub issue {:#github-issue}

[File a GitHub issue](https://github.com/bazelbuild/bazel/issues){: .external}
Expand All @@ -43,8 +42,6 @@ We recommend that:
update their code. Ideally, when the change is mechanical, include a link to a
migration tool.

* The description includes the intended length of migration window.

* The description includes an example of the error message users will get if
they don't migrate. This will make the GitHub issue more discoverable from
search engines. Make sure that the error message is helpful and actionable.
Expand All @@ -56,7 +53,6 @@ For the migration tool, consider contributing to
It is able to apply automated fixes to `BUILD`, `WORKSPACE`, and `.bzl` files.
It may also report warnings.


## Implementation {:#implementation}

Create a new flag in Bazel. The default value must be false. The help text
Expand All @@ -71,52 +67,57 @@ should contain the URL of the GitHub issue. As the flag name starts with

In the commit description, add a brief summary of the flag.
Also add [`RELNOTES:`](release-notes.md) in the following form:
`RELNOTES: --incompatible_name_of_flag has been added. See #yxz for details`
`RELNOTES: --incompatible_name_of_flag has been added. See #xyz for details`

The commit should also update the relevant documentation. As the documentation
is versioned, we recommend updating the documentation in the same commit
as the code change.


## Labels {:#labels}

Once the commit is merged, add the label
Once the commit is merged and the incompatible change is ready to be adopted, add the label
[`migration-ready`](https://github.com/bazelbuild/bazel/labels/migration-ready){: .external}
to the GitHub issue.

Later a [Bazel release manager](https://github.com/bazelbuild/continuous-integration/blob/master/docs/release-playbook.md){: .external}
will update the issue and replace the label with `migration-xx.yy`.

The label `breaking-change-xx.yy` communicates when we plan to flip the flag. If
a migration window needs to be extended, the author updates the label on GitHub
issue accordingly.

If a problem is found with the flag and users are not expected to migrate yet:
remove the flags `migration-xx.yy`.
remove the flags `migration-ready`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making people look at the issue to decide if it is migration ready or not it not going to be a great experience. A retraction of a plan to change behavior has to be acompannied by an announcmented on bazel-discuss that we are changing the plan.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making people look at the issue to decide if it is migration ready or not it not going to be a great experience.

Why? I think it's pretty easy to check what flags are migration ready by just opening this link instead of going through emails.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can still use the breaking-change-x.0 label to communicate which flags are meant to be flipped for a major release. Added a sentence about that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

breaking-change-x.0 will probably only be useful for archiving purposes (i.e. history), since we only do one major release every 9+ months and it's basically impossible for two open migration-ready issues to be targeted for different major releases.

Copy link
Member Author

@meteorcloudy meteorcloudy Sep 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true, but there could be incompatible flags that are not planned to be flipped for the next major release. For example, --incompatible_disallow_empty_glob and --incompatible_strict_action_env, they are ready for migration, but the migration might be very hard or blocked for other reason. By having this label, we can communicate which flags are actually going to be flipped in next major release, so that users can already start turning them on to prevent regression.

/cc @aiuto @alexeagle WDYT? Do you still think a custom bazelrc file is better? Can you parse this label to generate that bazelrc file?


## Updating repositories {:#update-repos}

1. Ensure that the core repositories are migrated. On the
[`bazelisk-plus-incompatible-flags` pipeline](https://buildkite.com/bazel/bazelisk-plus-incompatible-flags){: .external},
the flag should appear under "The following flags didn't break any passing Bazel team owned/co-owned projects".
1. Notify the owners of the other repositories.
1. Wait 14 days after the notifications to proceed, or until the flag is under
"The following flags didn't break any passing projects" in the CI.
Bazel CI tests a list of important projects at
[Bazel@HEAD + Downstream](https://buildkite.com/bazel/bazel-at-head-plus-downstream){: .external}. Most of them are often
dependencies of other Bazel projects, therefore it's important to migrate them to unblock the migration for the broader community.

To monitor the migration status of those projects, you can use the
[`bazelisk-plus-incompatible-flags` pipeline](https://buildkite.com/bazel/bazelisk-plus-incompatible-flags){: .external},
check how this pipeline works [here](https://github.com/bazelbuild/continuous-integration/tree/master/buildkite#checking-incompatible-changes-status-for-downstream-projects){: .external}.

Migrating projects in the downstream pipeline is NOT entirely the responsibility of the incompatible change author. But you can do the following to accelerate the migration and make life easier for both Bazel users and the Bazel Green Team.

1. File Github issues to notify the owners of the downstream projects broken by your incompatible change.
1. Send PRs to fix downstream projects.
1. Reach out to the Bazel community for help on migration (e.g. [Bazel Rules Authors SIG](https://bazel-contrib.github.io/SIG-rules-authors/)).

## Flipping the flag {:#flip-flag}

Before flipping the default value of the flag to true, please make sure that:

* The migration window is respected.
* User concerns and questions have been resolved.
* Core repositories in the ecosystem are migrated.

On the [`bazelisk-plus-incompatible-flags` pipeline](https://buildkite.com/bazel/bazelisk-plus-incompatible-flags){: .external},
the flag should appear under `The following flags didn't break any passing Bazel team owned/co-owned projects`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will people know they are bazel team owned. The pipeline should be all or nothing. That is, we should have a mandatory list that block a flag flip and an advisory one, which does not block us. It can't be up to analysis.
Unless we change it around and take the choice out of the developer's hands and give the choice to a release manager, who owns an LTS track and can allow or disallow any specific change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This information is recored in the ci downstream projects config: https://github.com/bazelbuild/continuous-integration/blob/534ca201c72b713a36afd017ec6bb38aff9cd4b5/buildkite/bazelci.py#L137

The list might be out of date now, but we can easily update them since @sventiffe @jdob and I already reviewed all repos under bazelbuild.


* User concerns and questions have been resolved.

When the flag is ready to flip in Bazel, but blocked on internal migration at Google, please consider setting the flag value to false in the internal `blazerc` file to unblock the flag flip. By doing this, we can ensure Bazel users depend on the new behaviour by default as early as possible.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for Google only. Should that be highlighted in some way?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most incompatible change authors are googler, so I think it's fine.


When changing the flag default to true, please:

* Use `RELNOTES[INC]` in the commit description, with the
* Use `RELNOTES[INC]` in the commit description, with the
following format:
`RELNOTES[INC]: --incompatible_name_of_flag is flipped to true. See #yxz for
`RELNOTES[INC]: --incompatible_name_of_flag is flipped to true. See #xyz for
details`
You can include additional information in the rest of the commit description.
* Use `Fixes #xyz` in the description, so that the GitHub issue gets closed
* Use `Fixes #xyz` in the description, so that the GitHub issue gets closed
when the commit is merged.
* Review and update documentation if needed.
* Review and update documentation if needed.
4 changes: 1 addition & 3 deletions site/en/release/backward-compatibility.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,4 @@ For every incompatible change, the issue specifies the following:
* Description of the changed functionality
* Migration recipe

The incompatible change issue is closed when the incompatible flag is flipped at
HEAD. All incompatible changes that are expected to happen in release X.Y
are marked with a label "breaking-change-X.Y"."
When an incompatible change is ready for migration with Bazel at HEAD (therefore, also with the next Bazel rolling release), it should be marked with the `migration-ready` label. The incompatible change issue is closed when the incompatible flag is flipped at HEAD.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migration ready is a signal that we are committed to the change and it will become permanent, but it is not necessarily in the next LTS release. We need to account for ones like the non-empty glob change, which has been migration ready for years, but we are still not ready to flip.

We should leave the bug open until we delete the code behind the flag, not just flip it. It is not over until it can't be reverted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, migration-ready doesn't imply it will be flipped in the next major release, it totally depends on the migration status and other potential blockers. Having this label tells the incompatible flags pipeline which flags to test so that we can check the migration statuses for downstream projects.

We should leave the bug open until we delete the code behind the flag, not just flip it. It is not over until it can't be reverted.

I agree with it, it's just there are already many issues closed without removing the flag, maybe we should reopen this. I'll update this line.