Report Bazel incompatibilities even with syntax and eval errors#28364
Report Bazel incompatibilities even with syntax and eval errors#28364fmeum wants to merge 13 commits intobazelbuild:masterfrom
Conversation
This reverts commit d13a541.
| public void throwDelayedExceptionIfAny( | ||
| @Nullable EvalException evalException, ExtendedEventHandler eventHandler) | ||
| throws EvalException { | ||
| if (incompatibilityMessage != null && (delayedSyntaxError != null || evalException != null)) { |
There was a problem hiding this comment.
This isn't ideal yet: I think that the UX would be better if we did emit the original eval/syntax error, but augmented with the note that the current module isn't compatible with the current Bazel version. What do you think?
|
@Wyverald This can likely be cleaned up more, but I wanted to run this by you first since it's a bit of an unusual change. :-) |
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the user experience by improving error reporting for Bazel incompatibilities in MODULE.bazel files. The refactoring of the BazelVersion class centralizes compatibility logic, and the new error handling mechanism intelligently prioritizes and reports relevant incompatibility messages even when syntax or evaluation errors are present. The changes are well-implemented and directly address the goal of providing more helpful feedback to users.
This ensures that users see a helpful error message pointing out that their Bazel version isn't compatible with a particular module even if the MODULE.bazel file contains constructs unsupported by that version of Bazel. This also extends to the
bazel_compatibilitysyntax itself.The implementation achieves this by:
module()call if syntax errors are encountered. These errors are stored and only emitted after the Bazel compatibility constraints of the truncated file have been obtained and evaluated.module()a lazy error in the same way.EvalExceptions thrown after themodule()call and emitting the incompatibility information.Fixes #28350