Use problem information in build failure message #31374

donat · 2024-11-21T13:06:58Z

Reviewing cheatsheet

Before merging the PR, comments starting with

❌ ❓must be fixed
🤔 💅 should be fixed
💭 may be fixed
🎉 celebrate happy things

donat · 2024-11-21T13:10:45Z

...ide/problems-api/src/main/java/org/gradle/api/problems/internal/InternalProblemReporter.java

+     * @return nothing, the method throws an exception
+     * @since 8.12
+     */
+    RuntimeException throwing(Throwable exception, Collection<? extends Problem> problems);


I'm going to do the API cleanup separately.

hegyibalint

Happy to see this shaping up!
Left some comments, PTAL.

hegyibalint · 2024-11-21T14:04:52Z

platforms/ide/problems-api/src/main/java/org/gradle/api/problems/internal/ProblemLookup.java

-     * @since 8.10
-     */
-    Collection<Problem> getProblems();
+public interface ProblemLookup {


🤔 Something doesn't click with me with this classname, but I cannot come up with something better either.
Any case, this is internal so if we come up with something better, we can change it anytime.

ProblemLocator?

...rojects/build-events/src/main/java/org/gradle/internal/build/event/types/DefaultFailure.java

hegyibalint · 2024-11-21T14:09:09Z

...rojects/build-events/src/main/java/org/gradle/internal/build/event/types/DefaultFailure.java

🎉 Nice cleanup, happy to see that exception mapping go away (at least from here)

...core/src/integTest/groovy/org/gradle/internal/buildevents/BuildFailureIntegrationTest.groovy

subprojects/core/src/main/java/org/gradle/internal/buildevents/BuildLoggerFactory.java

testing/architecture-test/src/changes/archunit-store/internal-api-nullability.txt

...distributions-integ-tests/src/integTest/groovy/org/gradle/DistributionIntegrationSpec.groovy

donat · 2024-11-21T14:20:09Z

@hegyibalint Thanks for the review, I'll clean up the items. Some of the issues come from cherry-picking changes from another (wip) branch.

hegyibalint

Thanks for the fixes, let's merge this!

...distributions-integ-tests/src/integTest/groovy/org/gradle/DistributionIntegrationSpec.groovy

bot-gradle · 2024-11-21T17:28:26Z

The following builds have failed:

Ready for Release (Trigger) (Check)
- Build Scan

donat · 2024-11-22T06:27:07Z

@bot-gradle merge

bot-gradle · 2024-11-22T06:27:18Z

WARN: Based on labels, this pull request addresses notable issue but no changes to release note found.

wolfs · 2024-11-22T17:00:21Z

...de/problems-api/src/main/java/org/gradle/api/problems/internal/ExceptionProblemRegistry.java

+                    }
+                }
+            } catch (RuntimeException e) {
+                e.printStackTrace();


❌ I don't think we should use e.printStackTrace() in our code base. Not sure if you want to do some debug logging here or what is the actual problem we guard against. I'd just throw the runtime exception if we don't have anything in particular to guard against here.

I agree, the printStackTrace() call should be deleted. I'd rather not rethrow the exception. We compare exception instances with user-supplied types that can have exotic problems that we don't anticipate. If the comparison fails, I'd not break the control flow but continue with returning null.

How can we find out that the exotic problems (or any problems) happen? Should we print out the problem on the info log?

wolfs · 2024-11-22T17:01:03Z

...de/problems-api/src/main/java/org/gradle/api/problems/internal/ExceptionProblemRegistry.java

+            StackTraceElement[] s1 = t1.getStackTrace();
+            StackTraceElement[] s2 = t2.getStackTrace();
+            for (int i = 0; i < s1.length && i < s2.length; i++) {
+                if (!isSackTraceElementEquals(s1[i], s2[i])) {


isStackTraceElementEquals instead of isSackTraceElementEquals?

wolfs · 2024-11-22T17:01:27Z

...de/problems-api/src/main/java/org/gradle/api/problems/internal/ExceptionProblemRegistry.java

+            return null;
+        }
+
+        private boolean deepEquals(Throwable t1, Throwable t2, List<Throwable> seen) {


I am really surprised we don't have that already somewhere.

I've spent some time looking around and I haven't found anything in Gradle nor in 3rd party libs (commons-lang, etc.).

wolfs · 2024-11-22T17:05:09Z

...de/problems-api/src/main/java/org/gradle/api/problems/internal/ExceptionProblemRegistry.java


-    private final Multimap<Throwable, Problem> problemsForThrowables = Multimaps.synchronizedMultimap(HashMultimap.<Throwable, Problem>create());
-
+    private final Multimap<Throwable, Problem> problemsForThrowables = Multimaps.synchronizedMultimap(MultimapBuilder.linkedHashKeys().linkedHashSetValues().<Throwable, Problem>build());


❓ Is it a performance problem to use a synchronized multimap here? Can we use some kind of concurrent maps instead?

I took a look an I think we cannot really improve the situation by using ConcurrentHashMap. We need to ensure that updating the list of values are update atomically, which requires synchronisation anyway.

Updating the list doesn't require synchronization if you use ConcurrentHashMap.compute(). ConcurrentHashMap should take care of that. Or are you referring to another list?

wolfs · 2024-11-22T17:05:38Z

...de/problems-api/src/main/java/org/gradle/api/problems/internal/ExceptionProblemRegistry.java

-    public Map<Throwable, Collection<Problem>> getProblemsForThrowables() {
-        return problemsForThrowables.asMap();
+    public ProblemLookup getProblemLookup() {
+        return new DefaultProblemLookup();


❓ Why do we recreate this expensive class on each call instead of keeping the last one around?

It raises concurrency issues if we cache the problem lookup. At the same time, we can make the expensive part (ie. initLookup() lazy).

wolfs · 2024-11-22T17:19:23Z

.../launcher/src/main/java/org/gradle/launcher/exec/RunAsBuildOperationBuildActionExecutor.java

-                        public Map<Throwable, Collection<Problem>> getProblemsForThrowables() {
-                            return problemContainer.getProblemsForThrowables();
+                        public ProblemLookup getProblemLookup() {
+                            return problemContainer.getProblemLookup();


🤔 That looks somewhat problematic: The problem lookup is only ready at the end, though we allow the lookup from the details object. Why is the problem lookup not a service?

I'll probably need some help with that. FWIW problemlookup needs to be accessible ClientForwardingBuildOperationListener which is created in the context of ToolingBuilderServices which has no access to build tree scope services.

wolfs · 2024-11-22T17:20:03Z

.../launcher/src/main/java/org/gradle/launcher/exec/RunAsBuildOperationBuildActionExecutor.java

-                        public Map<Throwable, Collection<Problem>> getProblemsForThrowables() {
-                            return problemContainer.getProblemsForThrowables();
+                        public ProblemLookup getProblemLookup() {
+                            return problemContainer.getProblemLookup();


💅 problemContainer should probably be called exceptionProblemRegistry.

tresat · 2024-11-27T19:32:34Z

...e/base-services/src/main/java/org/gradle/internal/exceptions/CompilationFailedIndicator.java

 public interface CompilationFailedIndicator extends NonGradleCause {
+
+    @Nullable
+    String getDiagnosticCounts();


💅 The name of this makes me think it is going to return structured data containing the counts. Maybe reportDiagnosticSummary() or something might be a better name.

Thanks for helping out with the naming!

getDiagnosticSummary or getReportDiagnosticSummary?

tresat · 2024-11-27T19:44:44Z

...de/problems-api/src/main/java/org/gradle/api/problems/internal/ExceptionProblemRegistry.java

+                String message = t.getMessage();
+                result = message == null ? "" : message;
+            } catch (RuntimeException ignore) {
+                // ignore exceptions with faulty getMessage() implementation


❓ Won't this cause issues finding the proper exception if there are multiple instances of an exception with a faulty getMessage() implementation that are thrown?

I don't see how. Both faulty exceptions would have still the same key in the lookup.

tresat · 2024-11-27T19:53:44Z

platforms/ide/problems-api/src/main/java/org/gradle/api/problems/internal/ProblemLookup.java

-     * @since 8.10
-     */
-    Collection<Problem> getProblems();
+public interface ProblemLookup {


ProblemLocator?

tresat · 2024-11-27T19:56:05Z

...uage-java/src/main/java/org/gradle/api/internal/tasks/compile/CompilationFatalException.java

+
+    @Override
+    public String getDiagnosticCounts() {
+        return null;


💅 Shouldn't this method be annotated @Nullable as well?

donat · 2024-11-28T16:31:41Z

@tresat @wolfs The changes addressing your review items are here: #31450

This comment has been minimized.

Sign in to view

donat changed the title ~~Donat/problems/render problems on cli~~ Use problem information in build failure message Nov 21, 2024

This comment has been minimized.

Sign in to view

donat force-pushed the donat/problems/render-problems-on-cli branch from 0cdd3e3 to 9613256 Compare November 21, 2024 13:09

donat commented Nov 21, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

donat force-pushed the donat/problems/render-problems-on-cli branch from 9613256 to 568dfab Compare November 21, 2024 13:30

donat requested a review from hegyibalint November 21, 2024 13:30

donat added 4 commits November 21, 2024 14:30

Introduce ProblemLookup

8978b16

Remove ResolutionProvider interface from CompilationFailedException

89b4b97

Use Problems API to access and render Java compilation errors

61bb61c

Adjust java compilation problem emitter code and fix tests

d15be93

donat force-pushed the donat/problems/render-problems-on-cli branch from 568dfab to d15be93 Compare November 21, 2024 13:30

This comment has been minimized.

Sign in to view

donat marked this pull request as ready for review November 21, 2024 13:31

This comment has been minimized.

Sign in to view

donat requested review from a team as code owners November 21, 2024 13:31

donat requested review from a team and tresat November 21, 2024 13:31

This comment has been minimized.

Sign in to view

hegyibalint requested changes Nov 21, 2024

View reviewed changes

Address review items

23c8dda

donat requested a review from hegyibalint November 21, 2024 14:37

This comment has been minimized.

Sign in to view

hegyibalint approved these changes Nov 21, 2024

View reviewed changes

...distributions-integ-tests/src/integTest/groovy/org/gradle/DistributionIntegrationSpec.groovy Show resolved Hide resolved

bot-gradle added this pull request to the merge queue Nov 22, 2024

bot-gradle added this to the 8.12 RC1 milestone Nov 22, 2024

Merged via the queue into master with commit bc745fa Nov 22, 2024
33 of 35 checks passed

bot-gradle deleted the donat/problems/render-problems-on-cli branch November 22, 2024 07:44

wolfs reviewed Nov 22, 2024

View reviewed changes

tresat reviewed Nov 27, 2024

View reviewed changes

donat mentioned this pull request Nov 28, 2024

Improve problem build failure association #31450

Merged


		private final Multimap<Throwable, Problem> problemsForThrowables = Multimaps.synchronizedMultimap(HashMultimap.<Throwable, Problem>create());

		private final Multimap<Throwable, Problem> problemsForThrowables = Multimaps.synchronizedMultimap(MultimapBuilder.linkedHashKeys().linkedHashSetValues().<Throwable, Problem>build());

Use problem information in build failure message #31374

Use problem information in build failure message #31374

Uh oh!

Conversation

donat commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewing cheatsheet

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

hegyibalint left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

donat commented Nov 21, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

hegyibalint left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bot-gradle commented Nov 21, 2024

Uh oh!

donat commented Nov 22, 2024

Uh oh!

bot-gradle commented Nov 22, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donat Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

donat commented Nov 21, 2024 •

edited

Loading

donat Nov 28, 2024 •

edited

Loading