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.

Sign up for GitHub

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

Jump to bottom

[MNG-8177] Add contextual info for model warnings #1633

Merged
merged 5 commits into from
Aug 12, 2024
Merged

[MNG-8177] Add contextual info for model warnings #1633

merged 5 commits into from
Aug 12, 2024

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Aug 9, 2024

As they really can come from anywhere. In case of this issue even from some eliminated POM that was read while collecting dirty tree, and was later eliminated. So confusing for users.

https://issues.apache.org/jira/browse/MNG-8177

@cstamas
[MNG-8177] Add context to descriptor warnings
d96ac44 
As they really can come from anywhere. In case of this
issue even from some eliminated POM that was read while
collecting dirty tree, and was later eliminated. So
confusing for users.

---

https://issues.apache.org/jira/browse/MNG-8177
@cstamas cstamas self-assigned this Aug 9, 2024
@cstamas cstamas added this to the 3.9.9 milestone Aug 9, 2024
slawekjaranowski
slawekjaranowski reviewed Aug 9, 2024
View reviewed changes
...resolver-provider/src/main/java/org/apache/maven/repository/internal/RequestTraceHelper.java Outdated
* Method that creates some informational string based on passed in {@link RequestTrace}. The contents of request
* trace can literally be anything, but this class tries to cover "most common" cases that are happening in Maven.
*/
public static String interpretTrace(boolean explain, RequestTrace requestTrace) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explain is always true

...resolver-provider/src/main/java/org/apache/maven/repository/internal/RequestTraceHelper.java Outdated
} else if (data instanceof CollectStepData) {
CollectStepData stepData = (CollectStepData) data;
return (explain ? "collect path " : "")
+ stepData.getPath().stream().map(Object::toString).collect(Collectors.joining(" -> "));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can generate very long one line message ... maybe more readable will be separated by new line

@cstamas
Copy link
Member Author

cstamas commented Aug 9, 2024

Updated PR: in "normal" invocation, it is one liner:

[WARNING] 1 problem was encountered while building the effective model for org.jboss:jboss-dmr:jar:1.3.0.Final during dependency collection step for project

When mvn -X used, it explains:

[WARNING] 1 problem was encountered while building the effective model for org.jboss:jboss-dmr:jar:1.3.0.Final during dependency collection step for project
Path to offending node from root:
 -> com.acme:example-subsystem:jar:1.0-SNAPSHOT
 -> org.wildfly.core:wildfly-subsystem-test:pom:24.0.0.Final (test)
 -> org.wildfly.core:wildfly-subsystem-test-framework:jar:24.0.0.Final (compile)
 -> org.wildfly.core:wildfly-model-test:jar:24.0.0.Final (compile)
 -> org.wildfly.legacy.test:wildfly-legacy-versions:jar:8.0.1.Final (compile)
[WARNING] 'dependencyManagement.dependencies.dependency.systemPath' for com.sun:tools:jar refers to a non-existing file /home/cstamas/.sdkman/candidates/java/21.0.4-tem/../lib/tools.jar. Please verify that you run Maven using a JDK and not just a JRE. @ org.jboss:jboss-dmr:1.3.0.Final

@WolfgangHG
Copy link

(I am the original reporter of this issue) I preferred the first approach to print full details out of the box. Could you add a hint " (use -X for full details)" to the short version?

@cstamas
Copy link
Member Author

cstamas commented Aug 10, 2024 •
edited
Loading

@WolfgangHG I think always printing out path would be "too much", Maven is already way way too chatty... but here are few facts to tinker about:

  • when descriptor reader reports this error all Maven "knows" that some POM somewhere has some issue (in this case: uses system scoped dependency pointing at file that does not exists)
  • later on, when collection happened (built the "dirty graph" -> conflicts resolved -> dep tree), just as in this example, the reported POM may not even be present...
  • still, IMO developer should be aware of issues like these, as any dependency change/reshuffle may make the offending POM end up in the build (in reproducer is "just lurking somewhere" but is not entering -- is eliminated)

So IMO one liner is fine, and will add "use -X for full details"

@cstamas
Copy link
Member Author

cstamas commented Aug 10, 2024 •
edited
Loading

Updated messages:

Without -X:

[WARNING] 1 problem was encountered while building the effective model for org.jboss:jboss-dmr:jar:1.3.0.Final during dependency collection step for project (use -X to see details)

With -X:

[WARNING] 1 problem was encountered while building the effective model for org.jboss:jboss-dmr:jar:1.3.0.Final during dependency collection step for project. Path to offending node from root:
 -> com.acme:example-subsystem:jar:1.0-SNAPSHOT
 -> org.wildfly.core:wildfly-subsystem-test:pom:24.0.0.Final (test)
 -> org.wildfly.core:wildfly-subsystem-test-framework:jar:24.0.0.Final (compile)
 -> org.wildfly.core:wildfly-model-test:jar:24.0.0.Final (compile)
 -> org.wildfly.legacy.test:wildfly-legacy-versions:jar:8.0.1.Final (compile)
 => org.jboss:jboss-dmr:jar:1.3.0.Final (compile)
Problem
* 'dependencyManagement.dependencies.dependency.systemPath' for com.sun:tools:jar refers to a non-existing file /home/cstamas/.sdkman/candidates/java/21.0.4-tem/../lib/tools.jar. Please verify that you run Maven using a JDK and not just a JRE. @ org.jboss:jboss-dmr:1.3.0.Final

@WolfgangHG
Copy link

One more thought: you wrote that the problematic dependency might be replaced later anyway for the effective dependency tree (as happened in my sample). Would it make sense to add another hint like "(problem might be resolved when building effective dependency tree)"?
I probably would still be confused by the new and improved warning message when seeing it the first time.

@cstamas
Copy link
Member Author

cstamas commented Aug 12, 2024

IMO adding that info may be superfluous (and would lengthen the message much much more). I just assume users are aware what "verbose" tree is and will be able to track it down, after all, the path is present now in message (in -X mode).

@cstamas cstamas changed the title [MNG-8177] Add context to descriptor warnings [MNG-8177] Add contextual info for model warnings Aug 12, 2024
@cstamas cstamas merged commit ecd577b into apache:maven-3.9.x Aug 12, 2024
12 checks passed
@cstamas cstamas deleted the maven-3.9.x-MNG-8177 branch August 12, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slawekjaranowski slawekjaranowski slawekjaranowski left review comments

@gnodet gnodet gnodet approved these changes

@michael-o michael-o Awaiting requested review from michael-o

Assignees

@cstamas cstamas

Labels
None yet
Projects
None yet
Milestone
3.9.9
Development

Successfully merging this pull request may close these issues.
4 participants
@cstamas @WolfgangHG @gnodet @slawekjaranowski