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.

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

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

Merged
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

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
* 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

} 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

@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

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
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants