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

Include error messages in incompatible change process #8483

Closed
EricBurnett opened this issue May 28, 2019 · 13 comments
Closed

Include error messages in incompatible change process #8483

EricBurnett opened this issue May 28, 2019 · 13 comments
Labels
P2 We'll consider working on this in future. (Assignee optional) stale Issues or PRs that are stale (no activity for 30 days) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: process

Comments

@EricBurnett
Copy link

Description of the problem / feature request:

The incompatible change process is centered on github labels/issues for a proactive testing workflow. Good error messages are needed to complement this, to address anyone doing reactive detection of bazel breakages (aka trying a new release and seeing what breaks; the most common pattern I see in the wild).

Feature requests: what underlying problem are you trying to solve with this feature?

Some breaking changes are going out with poor error messages, and it's not clear that that's being considered in the change approval process. I'd ideally like to see these changes have good error messages, and where not, have the error messages that users will see at least documented (for searchability), factored in as a cost to account for when these breaking changes are approved, and possibly highlighted centrally in the release notes as important to be aware of.

A couple current examples:

I should note that most rule changes like deprecating attributes do have good error messages; it's only a small subset that don't. Though the format is not standard - some errors link github issues, some link the --incompatible_... flag to override with, and some link both. Including examples in the incompatible change GH issues should help reviewers ensure both flag and issue URL are output.

What's the output of bazel info release?

Bazel 0.26 is most recent as of when I'm filing this.

@EricBurnett
Copy link
Author

@dslomov @aiuto

@laurentlb
Copy link
Contributor

Thanks, that's a good point. We can update our breaking-change process and add a note about error messages.

I cannot guarantee we can give good error messages for all of them. But we should be more careful about them and try to be more consistent. Maybe the author should always include an example error message in the commit description, to ensure it will get reviewed?

@EricBurnett
Copy link
Author

Do you review the issues themselves, or just the commits? Putting it into the commits sounds fine to me if that's what ends up getting reviewed, I was just assuming that someone signs off on the incompatible changes themselves and so the issue would be the best place to review the "experience" of it.

@laurentlb
Copy link
Contributor

We do systematic code reviews, but there's no process for reviewing GitHub issues. I think the best time for reviewing the error message is when the flag is implemented (most of the time, the code will include a throw ....).

We're supposed to create design docs for each significant change. At the moment, not all incompatible are associated with a design doc (mostly because there are lots of details we want to fix before Bazel 1.0). As we're moving out of beta, we could start requiring a document for all of them.

As a user, please file issues, or comment on existing ones, if an error message or a migration process is unclear. It will help other users (if they search for the same error message) and give us feedback.

@dslomov
Copy link
Contributor

dslomov commented May 29, 2019

I do not think this general request is very actionable. We are doing a reasonable job at providing informative error messages for the changes where it is possible. Sometimes it is not easy to attribute a specific error to a specific incompatible change though - the changes might have ripple effects where it is difficult to distinguish whether the specific problem is caused by the new behavior on the old input or the input just being incorrect - I trust that is what you see in the small set of examples that you refer to with opaque error messages.

I am going to close this general request as non-actionable. Please file issues for specific error messages that are not helpful.

@dslomov dslomov closed this as completed May 29, 2019
@EricBurnett
Copy link
Author

I'm not sure why you think this is non-actionable? You have a process; I'm suggesting this process include giving examples of error messages in issues.

Of the breaking changes for 0.27, exactly 1 gives an example of an error message you might see, and it's a helpful inclusion. Others, like #7407, give no indication at all of what breakages might look like. (And I confirmed there are no error messages in the codebase that reference its flag, so I don't think that's because it'd be self-explanatory).

@dslomov
Copy link
Contributor

dslomov commented May 29, 2019

As I said, I do not think we can always give a generic guidance about what the error messages will look like, so I am reluctant to add this to a generic process.
The best course of action would be to request this on specific issues where the error messages are unclear or non-actionable.

@laurentlb
Copy link
Contributor

We can add a recommendation in the process: "In the commit description, include an example of the error message a user might get. When possible, include the name of the flag" (or GitHub url?)
This will ensure that the author pays attention to it and encourage the reviewer to double-check it.

@dslomov
Copy link
Contributor

dslomov commented May 29, 2019

We can add a recommendation in the process: "In the commit description, include an example of the error message a user might get. When possible, include the name of the flag" (or GitHub url?)
This will ensure that the author pays attention to it and encourage the reviewer to double-check it.

This sgtm.

@dslomov
Copy link
Contributor

dslomov commented May 29, 2019

In the commit description, include an example of the error message a user might get. When possible, include the name of the flag

Well actually not in the commit description, but in the GitHub issue and migration recipe, right?

@dslomov dslomov reopened this May 29, 2019
@jin jin added team-Bazel General Bazel product/strategy issues untriaged labels Jul 22, 2019
@sventiffe sventiffe added P2 We'll consider working on this in future. (Assignee optional) and removed untriaged labels Feb 12, 2021
@gregestren gregestren added team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: process and removed team-Bazel General Bazel product/strategy issues labels Feb 1, 2022
@gregestren
Copy link
Contributor

This process has been deprecated for LTS. But people can still add --incompatible flags so the concerns are still valid.

Reassigned to team-Bazel which I think gets more attention.

We should find an actionable way to close out this bug, likely just following the advice in the above comments.

@github-actions
Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Jun 13, 2023
@github-actions
Copy link

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please reach out to the triage team (@bazelbuild/triage). Thanks!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 27, 2023
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) stale Issues or PRs that are stale (no activity for 30 days) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: process
Projects
None yet
Development

No branches or pull requests

6 participants