-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Beautify Bazel configuration errors #11984
Comments
/sub |
Configuration IDs are long. Example: 401ac12588e4c9bbed7d8d880eded6fa015cc5c752cedb42fa0b1c177724bb76 This follows the spirit of Git commit hash prefixes to make cquery and "blaze config" smart enough to deduce full IDs from unambiguous prefixes. cquery now outputs short IDs and its config() function accepts them. "blaze config" outputs full IDs but accepts "$ blaze config <short ID>". Before: $ bazel cquery 'deps(//:foo)' //:foo (eabb6af2b39c468822f14aa928370856968c05a3936d5d90057948a5427c5892) //:bar (968c05a3936d5d9005798370856968b6af2b327c5898c050579487c5870f1504) After: $ bazel cquery 'deps(//:foo)' //:foo (eabb6af) //:bar (968c05a) $ bazel cquery 'config(//:foo, eab)' //:foo (eabb6af) Full IDs are computed at https://github.com/bazelbuild/bazel/blob/a9b118bbe1d7c6c4be4524547dddc1fc21ba60ad/src/main/java/com/google/devtools/build/lib/util/Fingerprint.java#L330. We could conceivably shorten them by using a SHA-1 hash. But that's still pretty long. And it's more consistent to hash all build entities the same way. Also beautified relevant errors for #11984. PiperOrigin-RevId: 330578952
No more need for a redundant config_setting. See bazelbuild#8583 for an example. This was a bit subtle. constraint_value can't directly export a ConfigMatchingProvider because it needs to know the platform to determine if it's a match. But platforms are built out of constraint_values, so the platform isn't available yet. So the parent target with the select() provides this detail. Also beautifies "invalid select() key" errors in support of bazelbuild#11984. Fixes bazelbuild#8583. RELNOTES[NEW]: select() directly supports constraint_value (no need for an intermediate config_setting). PiperOrigin-RevId: 327885099 Change-Id: Ifbdfa275ff83f0cb30207a5bd77aca317599fc21
No more need for a redundant config_setting. See #8583 for an example. This was a bit subtle. constraint_value can't directly export a ConfigMatchingProvider because it needs to know the platform to determine if it's a match. But platforms are built out of constraint_values, so the platform isn't available yet. So the parent target with the select() provides this detail. Also beautifies "invalid select() key" errors in support of #11984. Fixes #8583. RELNOTES[NEW]: select() directly supports constraint_value (no need for an intermediate config_setting). Closes #12071. PiperOrigin-RevId: 331181346
If you're collecting error messages that need fixing, here's another one:
This is not much better than "Your build failed". It will be totally opaque to users who do not know that "output" has special meaning for Bazel rules. It does not mean the compile failed; it might have failed, but it might have succeeded yet failed to put the output in the right place. Worst of all is "[not] created or valid" - doesn't Bazel know which? Best, Gregg |
Thanks, Gregg. Excellent and subtle point on the "A or B" phrasing. I'll dig in when I get a chance to see what options we have for those errors. |
Here's another useless error message:
I have Gregg |
In recent years, a frustrating error for Bazel users is the one caused by putting a reverse dependency rather than a package group into a target's visibility attribute. Such a mistake generally resulted in an error like this: ``` BUILD:7:10: in filegroup rule //pkg1:actual_target: cycle in dependency graph: //pkg1:actual_target //pkg1:dep_with_visibility .-> //pkg1:actual_target | //pkg1:dep_with_visibility `-- //pkg1:actual_target This cycle occurred because of a configuration option ``` This was confusing primarily because users don't think of visibility as causing a dependency—see, for instance, bazelbuild#11945. Additionally, the "This cycle occurred because of a configuration option" hint was not usually accurate or helpful. That the visibility mistake will be reported as a cycle is a fundamental property of how analysis is mapped onto Skyframe today, but a better hint was certainly possible. In fact, deep within Bazel's bowels there was already code to produce a better message, but it became mostly dormant during the analysis-loading interleaving work of 2017. This CL removes the "This cycle occurred because of a configuration option" hint completely and restores the old visibility hint when appropriate. The primary code change is to combine ConfiguredTargetCycleReporter and TransitiveTargetCycleReporter to form TargetCycleReporter. Now, a visibility cycle looks like this: ``` BUILD:2:10: in filegroup rule //pkg1:actual_target: cycle in dependency graph: //pkg1:actual_target //pkg1:dep_with_visibility .-> //pkg1:actual_target | //pkg1:dep_with_visibility `-- //pkg1:actual_target The cycle is caused by a visibility edge from //pkg1:dep_with_visibility to the non-package_group target //pkg1:actual_target. Note that visibility labels are supposed to be package_group targets, which prevents cycles of this form. ``` I humbly include this change in the bazelbuild#11984 program.
In recent years, a frustrating error for Bazel users is the one caused by putting a reverse dependency rather than a package group into a target's visibility attribute. Such a mistake generally resulted in an error like this: ``` BUILD:7:10: in filegroup rule //pkg1:actual_target: cycle in dependency graph: //pkg1:actual_target //pkg1:dep_with_visibility .-> //pkg1:actual_target | //pkg1:dep_with_visibility `-- //pkg1:actual_target This cycle occurred because of a configuration option ``` This was confusing primarily because users don't think of visibility as causing a dependency—see, for instance, bazelbuild#11945. Additionally, the "This cycle occurred because of a configuration option" hint was not usually accurate or helpful. That the visibility mistake will be reported as a cycle is a fundamental property of how analysis is mapped onto Skyframe today, but a better hint was certainly possible. In fact, deep within Bazel's bowels there was already code to produce a better message, but it became mostly dormant during the analysis-loading interleaving work of 2017. This CL removes the "This cycle occurred because of a configuration option" hint completely and restores the old visibility hint when appropriate. The primary code change is to combine ConfiguredTargetCycleReporter and TransitiveTargetCycleReporter to form TargetCycleReporter. Now, a visibility cycle looks like this: ``` BUILD:2:10: in filegroup rule //pkg1:actual_target: cycle in dependency graph: //pkg1:actual_target //pkg1:dep_with_visibility .-> //pkg1:actual_target | //pkg1:dep_with_visibility `-- //pkg1:actual_target The cycle is caused by a visibility edge from //pkg1:dep_with_visibility to the non-package_group target //pkg1:actual_target. Note that visibility labels are supposed to be package_group targets, which prevents cycles of this form. ``` I humbly include this change in the bazelbuild#11984 program.
In recent years, a frustrating error for Bazel users is the one caused by putting a reverse dependency rather than a package group into a target's visibility attribute. Such a mistake generally resulted in an error like this: ``` BUILD:7:10: in filegroup rule //pkg1:actual_target: cycle in dependency graph: //pkg1:actual_target //pkg1:dep_with_visibility .-> //pkg1:actual_target | //pkg1:dep_with_visibility `-- //pkg1:actual_target This cycle occurred because of a configuration option ``` This was confusing primarily because users don't think of visibility as causing a dependency—see, for instance, bazelbuild#11945. Additionally, the "This cycle occurred because of a configuration option" hint was not usually accurate or helpful. That the visibility mistake will be reported as a cycle is a fundamental property of how analysis is mapped onto Skyframe today, but a better hint was certainly possible. In fact, deep within Bazel's bowels there was already code to produce a better message, but it became mostly dormant during the analysis-loading interleaving work of 2017. This CL removes the "This cycle occurred because of a configuration option" hint completely and restores the old visibility hint when appropriate. The primary code change is to combine ConfiguredTargetCycleReporter and TransitiveTargetCycleReporter to form TargetCycleReporter. Now, a visibility cycle looks like this: ``` BUILD:2:10: in filegroup rule //pkg1:actual_target: cycle in dependency graph: //pkg1:actual_target //pkg1:dep_with_visibility .-> //pkg1:actual_target | //pkg1:dep_with_visibility `-- //pkg1:actual_target The cycle is caused by a visibility edge from //pkg1:dep_with_visibility to the non-package_group target //pkg1:actual_target. Note that visibility labels are supposed to be package_group targets, which prevents cycles of this form. ``` I humbly include this change in the bazelbuild#11984 program.
In recent years, a frustrating error for Bazel users is the one caused by putting a reverse dependency rather than a package group into a target's visibility attribute. Such a mistake generally resulted in an error like this: ``` BUILD:7:10: in filegroup rule //pkg1:actual_target: cycle in dependency graph: //pkg1:actual_target //pkg1:dep_with_visibility .-> //pkg1:actual_target | //pkg1:dep_with_visibility `-- //pkg1:actual_target This cycle occurred because of a configuration option ``` This was confusing primarily because users don't think of visibility as causing a dependency—see, for instance, bazelbuild#11945. Additionally, the "This cycle occurred because of a configuration option" hint was not usually accurate or helpful. That the visibility mistake will be reported as a cycle is a fundamental property of how analysis is mapped onto Skyframe today, but a better hint was certainly possible. In fact, deep within Bazel's bowels there was already code to produce a better message, but it became mostly dormant during the analysis-loading interleaving work of 2017. This CL removes the "This cycle occurred because of a configuration option" hint completely and restores the old visibility hint when appropriate. The primary code change is to combine ConfiguredTargetCycleReporter and TransitiveTargetCycleReporter to form TargetCycleReporter. Now, a visibility cycle looks like this: ``` BUILD:2:10: in filegroup rule //pkg1:actual_target: cycle in dependency graph: //pkg1:actual_target //pkg1:dep_with_visibility .-> //pkg1:actual_target | //pkg1:dep_with_visibility `-- //pkg1:actual_target The cycle is caused by a visibility edge from //pkg1:dep_with_visibility to the non-package_group target //pkg1:actual_target. Note that visibility labels are supposed to be package_group targets, which prevents cycles of this form. ``` I humbly include this change in the bazelbuild#11984 program.
In recent years, a frustrating error for Bazel users is the one caused by putting a reverse dependency rather than a package group into a target's visibility attribute. Such a mistake generally resulted in an error like this: ``` BUILD:7:10: in filegroup rule //pkg1:actual_target: cycle in dependency graph: //pkg1:actual_target //pkg1:dep_with_visibility .-> //pkg1:actual_target | //pkg1:dep_with_visibility `-- //pkg1:actual_target This cycle occurred because of a configuration option ``` This was confusing primarily because users don't think of visibility as causing a dependency—see, for instance, bazelbuild#11945. Additionally, the "This cycle occurred because of a configuration option" hint was not usually accurate or helpful. That the visibility mistake will be reported as a cycle is a fundamental property of how analysis is mapped onto Skyframe today, but a better hint was certainly possible. In fact, deep within Bazel's bowels there was already code to produce a better message, but it became mostly dormant during the analysis-loading interleaving work of 2017. This CL removes the "This cycle occurred because of a configuration option" hint completely and restores the old visibility hint when appropriate. The primary code change is to combine ConfiguredTargetCycleReporter and TransitiveTargetCycleReporter to form TargetCycleReporter. Now, a visibility cycle looks like this: ``` BUILD:2:10: in filegroup rule //pkg1:actual_target: cycle in dependency graph: //pkg1:actual_target //pkg1:dep_with_visibility .-> //pkg1:actual_target | //pkg1:dep_with_visibility `-- //pkg1:actual_target The cycle is caused by a visibility edge from //pkg1:dep_with_visibility to the non-package_group target //pkg1:actual_target. Note that visibility labels are supposed to be package_group targets, which prevents cycles of this form. ``` I humbly include this change in the bazelbuild#11984 program.
In recent years, a frustrating error for Bazel users is the one caused by putting a reverse dependency rather than a package group into a target's visibility attribute. Such a mistake generally resulted in an error like this: ``` BUILD:7:10: in filegroup rule //pkg1:actual_target: cycle in dependency graph: //pkg1:actual_target //pkg1:dep_with_visibility .-> //pkg1:actual_target | //pkg1:dep_with_visibility `-- //pkg1:actual_target This cycle occurred because of a configuration option ``` This was confusing primarily because users don't think of visibility as causing a dependency—see, for instance, #11945. Additionally, the "This cycle occurred because of a configuration option" hint was not usually accurate or helpful. That the visibility mistake will be reported as a cycle is a fundamental property of how analysis is mapped onto Skyframe today, but a better hint was certainly possible. In fact, deep within Bazel's bowels there was already code to produce a better message, but it became mostly dormant during the analysis-loading interleaving work of 2017. This CL removes the "This cycle occurred because of a configuration option" hint completely and restores the old visibility hint when appropriate. The primary code change is to combine ConfiguredTargetCycleReporter and TransitiveTargetCycleReporter to form TargetCycleReporter. Now, a visibility cycle looks like this: ``` BUILD:2:10: in filegroup rule //pkg1:actual_target: cycle in dependency graph: //pkg1:actual_target //pkg1:dep_with_visibility .-> //pkg1:actual_target | //pkg1:dep_with_visibility `-- //pkg1:actual_target The cycle is caused by a visibility edge from //pkg1:dep_with_visibility to the non-package_group target //pkg1:actual_target. Note that visibility labels are supposed to be package_group targets, which prevents cycles of this form. ``` I humbly include this change in the #11984 program. Closes #12272. PiperOrigin-RevId: 342638902
This is a common user problem: users see this error and don't understand what it means or how to diagnose. This change is an opening salvo in a broader plan to integrate more "beautiful" error messaging into Blaze, inspired by https://bazel.build/designs/2016/05/23/beautiful-error-messages.html Example: config_setting( name = "foo", values = {"cpu": "not_a_valid_cpu}, ) config_setting( name = "foo2", values = {"cpu": "also_not_a_valid_cpu"}, ) cc_binary( name = "cc", srcs = select({ ":foo": [], ":foo2": [], })) $ blaze build //testapp:cc Before: ----------- ERROR: /workspace/testapp/BUILD:16:10: Configurable attribute "srcs" doesn't match this configuration (would a default condition help?). Conditions checked: //testapp:foo //testapp:foo2 After: ----------- ERROR: /workspace/BUILD:16:10: configurable attribute "srcs" in //testapp:cc doesn't match this configuration. Would a default condition help? Conditions checked: //testapp:foo //testapp:foo2 //testapp:foo3 To see a condition's definition, run: bazel query --output=build <condition label>. This instance of //testapp:cc has configuration identifier 7f2b080. To inspect its configuration, run: bazel config 7f2b080. For more help, see https://docs.bazel.build/configurable-attributes.html#why-doesnt-my-select-choose-what-i-expect. Themes: ----------- - This is consciously longer and more verbose. The primary goal is *clarity*, which is not always aligned with shortness. This includes paragraph-style line spacing to de-densify potentially overwhelming content. - Uses casual but personable language, including actionable phrasing ("see", "run") to help guide understanding and diagnostics. - Provides a thorough walkthrough including proposed next steps for the immediate problem while leaving discussion on broader concepts to outside links. - Increases our commitment to "always supported URLs" (including in-page anchors!) - Main idea is to look like the cleanest, most helpful Stackoverflow-style answer possible. There's no intrinsic reason why internal messaging can't be held to the same accessibility standards. For #11984. PiperOrigin-RevId: 369470807
@mobileink - I just submitted 3d33d7b and looking for another error to polish. For #11984 (comment), could you sketch your version of the ideal message? In 3d33d7b 's description I laid out principles I'm trying to introduce: basically make error messages look like great StackOverflow answers (clear, helpful, readable, concise). Re: implementation, I'm not sure how much of the relevant context the code at that stage has. That error is generated at bazel/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java Line 1599 in a790c17
bazel/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java Line 1194 in a790c17
|
Here's one that's not confusing exactly, but just hard to read and easy to misread. Keep in mind that I shortened the paths to make the example simpler, but its even harder to read with real-world path and target lengths
Would be much more readable as:
|
This is great. Thanks for the suggestion @catskul. Some basic formatting standards could go a long way. |
This is a common user problem: users see this error and don't understand what it means or how to diagnose. This change is an opening salvo in a broader plan to integrate more "beautiful" error messaging into Blaze, inspired by https://bazel.build/designs/2016/05/23/beautiful-error-messages.html Example: config_setting( name = "foo", values = {"cpu": "not_a_valid_cpu}, ) config_setting( name = "foo2", values = {"cpu": "also_not_a_valid_cpu"}, ) cc_binary( name = "cc", srcs = select({ ":foo": [], ":foo2": [], })) $ blaze build //testapp:cc Before: ----------- ERROR: /workspace/testapp/BUILD:16:10: Configurable attribute "srcs" doesn't match this configuration (would a default condition help?). Conditions checked: //testapp:foo //testapp:foo2 After: ----------- ERROR: /workspace/BUILD:16:10: configurable attribute "srcs" in //testapp:cc doesn't match this configuration. Would a default condition help? Conditions checked: //testapp:foo //testapp:foo2 //testapp:foo3 To see a condition's definition, run: bazel query --output=build <condition label>. This instance of //testapp:cc has configuration identifier 7f2b080. To inspect its configuration, run: bazel config 7f2b080. For more help, see https://docs.bazel.build/configurable-attributes.html#why-doesnt-my-select-choose-what-i-expect. Themes: ----------- - This is consciously longer and more verbose. The primary goal is *clarity*, which is not always aligned with shortness. This includes paragraph-style line spacing to de-densify potentially overwhelming content. - Uses casual but personable language, including actionable phrasing ("see", "run") to help guide understanding and diagnostics. - Provides a thorough walkthrough including proposed next steps for the immediate problem while leaving discussion on broader concepts to outside links. - Increases our commitment to "always supported URLs" (including in-page anchors!) - Main idea is to look like the cleanest, most helpful Stackoverflow-style answer possible. There's no intrinsic reason why internal messaging can't be held to the same accessibility standards. For bazelbuild/bazel#11984. PiperOrigin-RevId: 369470807
@gregestren regarding the "was not created" error message, what would improve things the most for me would be some help for cases where the action, worked and the expected file was generated, just with the wrong name or in the wrong location. I've on more occasions than I care to count stuffed Just a working directory listing (or a listing of new files that the action created) would be an improvement. If that's too verbose, then dump that to a file and tell me where it is. For bonus points; pick the 3-10 closes "matches" and ask "did you want...".
In this case, while I learn something from the default message, there is nothing I can do with it to make the next attempt better. |
@bcsgh good points. Are you talking about a case where Bazel knows where the output is written and isn't telling the user? i.e. if you write a |
I'm thinking of the cast where bazel has no clue what went wrong, only that no result ended up in the expected location. I'm not expecting bazel to log file access or anything but just give me some actionable information about the final state. Something as simple as |
You're doing local builds (no remote executor)? Have you tried |
I've tried While I'd like to see at a minimum is the existing error message:
Becomes something like
Where |
tl;dr: apply beautiful error principles to build failures from analysis,
select()
, configurations, user-defined flags, transitions, and so on.We have enough experience to identify where current errors fall short. There are straightforward ways we make them clearer, more actionable, and more closely connected to relevant documentation.
I'm starting this for configuration-related errors since this is the Bazel area I know best. More broadly I'd like to extend these principles to Bazel error culture as a whole in the interest of making Bazel more accessible.
Crowd-sourced input on confusing messages welcome!
The text was updated successfully, but these errors were encountered: