Improve error message for unresolved toolchains #15135

cpsauer · 2022-03-29T02:42:08Z

Hi wonderful Bazel folks,

I happened to look back at #10134 (comment), and #10134 more generally, and noticed that people had been reacting, requesting a change I'd floated, so I PR'd it. In brief, this PR proposes a fix that aims to resolve the issues that lead to user frustration there.

Please do read that issue for context before reviewing, but first, briefly some context around it:

Background: It's common to need to debug toolchain matching problems when getting set up with Bazel platforms.
Problem: The current error message doesn't point users to Bazel's (great!) debugging mechanism (--toolchain_resolution_debug). Instead the error links people to what's more likely to be a red herring (that issue, #10134), leading to lots of user frustration there. The original message seems like it was probably the right one back when flipping the flag seemed imminent, but the general consensus now seems to be that the link wasn't helpful and should be removed. Besides some flaming, a bunch of people had 👍🏻'd or ❤️'d the --toolchain_resolution_debug debugging actions I'd posted to indicate that they wanted it added to the error message. (Those reacts are what I'd happened to see when someone posted on the issue, sending a notification.)
Solution: So this PR changes the error message to (1) add actionable advice to use the debugging flag and (2) remove the link to the issue. [Minor: It also makes capitalization and punctuation consistent between the error cases.]

I'd love your thoughtful consideration and review!

Thanks, wonderful Bazel folks, for all you do!

Cheers,
Chris
(ex-Googler)

P.S. If you'd rather leave the link to #10134 in the error message--and just add the actionable debugging suggestion, we could totally do that, too. I agonized a bit over whether to remove the link, before deciding to because (1) it seemed pretty unlikely that someone would flip the flag given its status, (2) people had explicitly suggested it be removed and (3) there's the opportunity to add instructions if and when the flag is picked back up. All that said, if you'd like to add the messaging back in, we could totally just append it to the error message! (I've checked the "allow edits by maintainers" box, too, so you can just do so and merge!) Might make sense to wordsmith a little at the same time, to reduce confusion? And also, if we re-add it, should the condition be instead whether the cpp toolchain type is anywhere in the list?

cpsauer · 2022-03-29T03:04:04Z

cc'ing @gregestren because we've interacted before on platforms stuff, and @lberki and @ahumesky since they were active on that other issue.
{please coach me if that's not right}

gregestren · 2022-04-04T21:19:55Z

Thanks for this contribution @cpsauer. I'm digging out of an inbox hole but this remains in my inbox and I'll get to it with a little bit of patience.

FWIW I think this is entirely orthogonal but you still might find it interesting: #14726

cpsauer · 2022-04-05T03:43:19Z

You're very welcome. Thank you.
(I have a newfound appreciation for the inbox hole...newly maintaining, a tool for Bazel users and having to move fast to keep up.)

Thanks also for the crosslink. Haven't delved too deep into implementing toolchains yet--just the basics of selecting and using. LMK if you think these will collide somehow.

-Chris

gregestren · 2022-04-26T23:06:54Z

This all looks good to me, and I think your reasons are sound. I definitely support more generically actionable messaging (beyond just C++).

I really don't know if the old link is still meaningfully relevant. Given no clear argument that it is, I'm happy to exclude it by default.

The only nitty thing I wonder about is I think of --toolchain_resolution_debug are more of a power user feature, i.e. people who are actively working designing toolchains and rules. For regular users who run a build and get that error I suspect they'd find the output baffling. What about also an actionable reference to the basic documentation on platforms/toolchains to at least give regular users an anchor point for framing followup help requests?

CC @katre just FYI.

gregestren · 2022-04-26T23:07:59Z

Like https://bazel.build/concepts/platforms-intro?

cpsauer · 2022-04-27T00:35:52Z

Thanks, @gregestren! Really appreciate your thoughtfulness and your getting back to me.

Sounds like we're well aligned on shipping this. Awesome.

On the link: Happy to add if you'd like. But I do earnestly think that users will be ready for --toolchain_resolution_debug by the time they get here. --toolchain_resolution_debug was, at least, fantastically helpful to me at this stage, and seems to have been also for the other folks encountering the error. My mental model is that you'd very likely arrive at that error because you've already read about platforms/toolchains and have been fiddling around with them, triggering that error upon making a mistake or encountering out of date docs or a bug. I'd anticipate that most users would know that the action they just took caused the error, but perhaps not what platform value they should have specified instead. And --toolchain_resolution_debug is awesome for helping them discover that. Plus, the main docs are pretty googlable and discoverable, but that flag, I think, much less so.

All that said, I'll of course happily defer to you if you still think we should add the link! In that case, what about appending an additional line something like: "If platforms or toolchains are a new concept to you, we'd encourage reading https://bazel.build/concepts/platforms-intro." ?

gregestren · 2022-04-27T15:00:37Z

I imagine (hope) that over time it's more likely that novice users will use platform-enabled builds, even if they don't realize it (i.e. just by using a rule set the implicitly use toolchains). I think a concise link would be helpful. Although it could be in --toolchain_resolution_debug's output if we don't want to spam the user error.

katre · 2022-05-04T16:01:52Z

src/test/shell/integration/toolchain_test.sh

@@ -436,7 +436,7 @@ use_toolchain(
 EOF

  bazel build "//${pkg}/demo:use" &> $TEST_log && fail "Build failure expected"
-  expect_log "While resolving toolchains for target //${pkg}/demo:use: No matching toolchains found for types //${pkg}/toolchain:test_toolchain. To debug, rerun with --toolchain_resolution_debug='//${pkg}/toolchain:test_toolchain'"
+  expect_log "While resolving toolchains for target //${pkg}/demo:use: No matching toolchains found for types //${pkg}/toolchain:test_toolchain.\nTo debug, rerun with --toolchain_resolution_debug='//${pkg}/toolchain:test_toolchain\nIf platforms or toolchains are a new concept to you, we'd encourage reading https://bazel.build/concepts/platforms-intro.'"


expect_log does regex matching, so you really only need the "To debug" part, not the tailing bit.

Ah, cool. Thanks, John. Will slim down to just the "To debug" part, per your suggestion.

[I see the grep-based implementation now, which I think precludes having newlines in the pattern--and while I'd aimed for completeness before, I suppose that one line does fully test the mismatch. Sorry also that you saw the test failures--figured I'd just quickly run the change by CI and then tap back today after it finished.]

Err, actually, if we're just looking to check that there's a mismatch error with the right toolchain type, probably I should change the expect_log to just be the first error line. Same spirit, but more directly checking the error. Hopefully that's okay.

Add link to general platforms docs in response to feedback

cpsauer · 2022-05-05T00:00:42Z

Great! New commit up and green, hopefully responding well to your good feedback.

@gregestren, sounds like you'd still like that link: It's in--with a minor wording improvement from what I'd put up before.

katre

LGTM, let's see if Greg has anything left to say.

gregestren

LGTM. Thanks, Chris.

gregestren · 2022-05-05T21:08:13Z

Strange: on merge I'm getting CI failures like https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/c0202940-cb70-42de-816e-a9b3fa85cf83/src%5Ctest%5Cjava%5Ccom%5Cgoogle%5Cdevtools%5Cbuild%5Clib%5Cskyframe%5CSkyframeTests%5Cshard_15_of_20%5Ctest_attempts%5Cattempt_1.log.

which correlates to

bazel/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java

Line 192 in e6fbe5b

.contains("no matching toolchains found for types //toolchain:test_toolchain");

.

Why didn't the PR checks catch this?

katre · 2022-05-05T21:25:59Z

Was the PR not merged to head?

…

On Thu, May 5, 2022, 5:08 PM Greg ***@***.***> wrote: Strange: on merge I'm getting CI failures like https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/c0202940-cb70-42de-816e-a9b3fa85cf83/src%5Ctest%5Cjava%5Ccom%5Cgoogle%5Cdevtools%5Cbuild%5Clib%5Cskyframe%5CSkyframeTests%5Cshard_15_of_20%5Ctest_attempts%5Cattempt_1.log . which correlates to https://github.com/bazelbuild/bazel/blob/e6fbe5b6cbc33375731bf0d6315fb31264211a80/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java#L192 . Why didn't the PR checks catch this? — Reply to this email directly, view it on GitHub <#15135 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACPW75FGXCPNPBMC2TOTMTVIQ2ERANCNFSM5R46SNGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

cpsauer · 2022-05-05T21:42:33Z

@gregestren, looks like it's a new test @katre added several days ago, so it wasn't on the feature branch corresponding to this PR.

cpsauer · 2022-05-05T21:44:28Z

How about if I merge in master and fix?
Also, after working on some Windows clangd things, I think we should use double quotes in the flag in the error message to support Windows CMD shell, so I'll make that change, too.

introduced in bazelbuild@05747cf

… cmd shell

cpsauer · 2022-05-05T21:57:26Z

Great. Done. Tests rerunning!

cpsauer · 2022-05-05T22:24:03Z

And we're green! Good to merge?

gregestren · 2022-05-06T17:42:13Z

Trying again. Thanks, Chris.

brentleyjones · 2022-05-06T18:21:58Z

@bazel-io flag

ckolli5 · 2022-05-09T03:58:25Z

@bazel-io fork 5.2.0

Closes #15135. PiperOrigin-RevId: 447028054 Co-authored-by: Christopher Peterson Sauer <christophersauer@pacbell.net>

cpsauer force-pushed the patch-2 branch from bda8424 to 2c852ed Compare March 29, 2022 03:01

cpsauer mentioned this pull request Mar 29, 2022

incompatible_use_cc_configure_from_rules_cc: Use cc_configure from @rules_cc #10134

Open

cpsauer force-pushed the patch-2 branch from 2c852ed to aa0deb2 Compare March 29, 2022 03:25

Improve error message for unresolved toolchains

348c85d

cpsauer force-pushed the patch-2 branch from aa0deb2 to 348c85d Compare March 29, 2022 04:05

sgowroji added the team-Rules-CPP Issues for C++ rules label Apr 5, 2022

sgowroji requested a review from oquenchil April 21, 2022 05:07

sgowroji added the awaiting-review PR is awaiting review from an assigned reviewer label Apr 21, 2022

sventiffe requested a review from gregestren April 26, 2022 12:27

gregestren added team-Configurability platforms, toolchains, cquery, select(), config transitions and removed awaiting-review PR is awaiting review from an assigned reviewer team-Rules-CPP Issues for C++ rules labels Apr 26, 2022

gregestren self-assigned this Apr 26, 2022

katre reviewed May 4, 2022

View reviewed changes

Improve unresolved toolchains error

7aaeab6

Add link to general platforms docs in response to feedback

cpsauer force-pushed the patch-2 branch from e80defa to 7aaeab6 Compare May 4, 2022 23:24

oquenchil removed their request for review May 5, 2022 07:56

katre approved these changes May 5, 2022

View reviewed changes

gregestren approved these changes May 5, 2022

View reviewed changes

cpsauer and others added 3 commits May 5, 2022 14:44

Merge branch 'bazelbuild:master' into patch-2

0812a29

Update new test

d097dea

introduced in bazelbuild@05747cf

Change to double quoting flag pattern to properly escape | in Windows…

3c3f22a

… cmd shell

bazel-io closed this in 45f4628 May 6, 2022

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label May 6, 2022

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label May 9, 2022

bazel-io mentioned this pull request May 9, 2022

[5.2.0] Improve error message for unresolved toolchains #15424

Closed

ckolli5 mentioned this pull request May 10, 2022

Improve error message for unresolved toolchains #15451

Merged

ckolli5 added a commit that referenced this pull request May 10, 2022

Improve error message for unresolved toolchains (#15451)

4dca905

Closes #15135. PiperOrigin-RevId: 447028054 Co-authored-by: Christopher Peterson Sauer <christophersauer@pacbell.net>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error message for unresolved toolchains #15135

Improve error message for unresolved toolchains #15135

cpsauer commented Mar 29, 2022 •

edited

Loading

cpsauer commented Mar 29, 2022

gregestren commented Apr 4, 2022

cpsauer commented Apr 5, 2022 •

edited

Loading

gregestren commented Apr 26, 2022 •

edited

Loading

gregestren commented Apr 26, 2022

cpsauer commented Apr 27, 2022

gregestren commented Apr 27, 2022

katre May 4, 2022

cpsauer May 4, 2022 •

edited

Loading

cpsauer May 4, 2022

cpsauer commented May 5, 2022 •

edited

Loading

katre left a comment

gregestren left a comment

gregestren commented May 5, 2022

katre commented May 5, 2022 via email

cpsauer commented May 5, 2022

cpsauer commented May 5, 2022

cpsauer commented May 5, 2022

cpsauer commented May 5, 2022

gregestren commented May 6, 2022

brentleyjones commented May 6, 2022

ckolli5 commented May 9, 2022

Improve error message for unresolved toolchains #15135

Improve error message for unresolved toolchains #15135

Conversation

cpsauer commented Mar 29, 2022 • edited Loading

Please do read that issue for context before reviewing, but first, briefly some context around it:

cpsauer commented Mar 29, 2022

gregestren commented Apr 4, 2022

cpsauer commented Apr 5, 2022 • edited Loading

gregestren commented Apr 26, 2022 • edited Loading

gregestren commented Apr 26, 2022

cpsauer commented Apr 27, 2022

gregestren commented Apr 27, 2022

katre May 4, 2022

Choose a reason for hiding this comment

cpsauer May 4, 2022 • edited Loading

Choose a reason for hiding this comment

cpsauer May 4, 2022

Choose a reason for hiding this comment

cpsauer commented May 5, 2022 • edited Loading

katre left a comment

Choose a reason for hiding this comment

gregestren left a comment

Choose a reason for hiding this comment

gregestren commented May 5, 2022

katre commented May 5, 2022 via email

cpsauer commented May 5, 2022

cpsauer commented May 5, 2022

cpsauer commented May 5, 2022

cpsauer commented May 5, 2022

gregestren commented May 6, 2022

brentleyjones commented May 6, 2022

ckolli5 commented May 9, 2022

cpsauer commented Mar 29, 2022 •

edited

Loading

cpsauer commented Apr 5, 2022 •

edited

Loading

gregestren commented Apr 26, 2022 •

edited

Loading

cpsauer May 4, 2022 •

edited

Loading

cpsauer commented May 5, 2022 •

edited

Loading