-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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-7758] Report dependency problems for all repository #1563
Conversation
How does it look now? |
Now looks:
only problem with last repository is reported - everything in one line ... |
IT |
I also would like to cherry pick it to 3.9.x |
result, "Could not resolve dependencies for project " + project.getId() + ": " + e.getMessage(), e); | ||
result, | ||
"Could not collect dependencies for project " + project.getId(), | ||
logger.isDebugEnabled() ? e : null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why omitting the exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we have exception in stack all message from all exception will be added to output in normal run we will have some information duplicate now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But isn't that a problem of the catcher, not the thrower?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have more detailed info at debug level, especially in case of an error, it may make sense to indicate so in the non detailed error message ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
detailed contains more info which is not available in o.e.aether.resolution.ArtifactResolutionException
ArtifactResolutionException.getMessage
returns error for last repository
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without it some of message will be duplicated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without passing exception (current proposition) we will have:
[ERROR] Failed to execute goal on project test: Could not resolve dependencies for project org.apache.maven.its.mng3477:test:jar:1
[ERROR] dependency: org.apache.maven.its.mng3477:dep:jar:1.0 (compile)
[ERROR] Could not find artifact org.apache.maven.its.mng3477:dep:jar:1.0 in maven-core-it (http://localhost:64468/repo)
[ERROR] Could not find artifact org.apache.maven.its.mng3477:dep:jar:1.0 in central (http://localhost:64468/repo)
[ERROR] -> [Help 1]
with exception will be as:
[ERROR] Failed to execute goal on project test: Could not resolve dependencies for project org.apache.maven.its.mng3477:test:jar:1
[ERROR] dependency: org.apache.maven.its.mng3477:dep:jar:1.0 (compile)
[ERROR] Could not find artifact org.apache.maven.its.mng3477:dep:jar:1.0 in central (http://localhost:64398/repo)
[ERROR] Could not find artifact org.apache.maven.its.mng3477:dep:jar:1.0 in maven-core-it (http://localhost:64398/repo): The following artifacts could not be resolved: org.apache.maven.its.mng3477:dep:jar:1.0 (absent): Could not find artifact org.apache.maven.its.mng3477:dep:jar:1.0 in central (http://localhost:64398/repo)
[ERROR] -> [Help 1]
so it is a reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope that simply message will be more readable for user, if user need more details for debug or investigation can execute in verbose/debug mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VERY UGLY. I guess this needs a long term solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can think about it ... how to reporting a problems to user but it is another issue for me, now I did as simple as possible
we also have many other UGLY code ... 😸
maven-core/src/main/java/org/apache/maven/project/DependencyResolutionException.java
Show resolved
Hide resolved
Give me a day or two, I try to try this out in a realworld env. |
I would be happy to have it in 3.9.x also So I fix ITs first 😄 |
Fixed IT - apache/maven-integration-testing#346 |
26695ff
to
ea7309b
Compare
@michael-o any progress on it |
No, I can do that only at work were I have the setup. Will happen tomorrow. Maven Release kept me busy. I will update you. |
Trying... |
I have done some further testing. Good news: DOXIASITETOOLS-303 seems to be solved by this. Bad news: It does not work the way advertised. I have in my
tried:
ran:
Is it what you expect? I would expect to try all four repos. It is trying:
so does
Thoughts? The issue can be easily recreated by setting up web sever denying all requests with 403. |
Resolver handles differently 403, as it assumes "user error" in those cases (and user would fix/update access credentials, or remove repo). |
Resolver "knows" 404 and "everything else" (like 403) |
403 is not an authentication problem, but solely authorization: https://www.rfc-editor.org/rfc/rfc9110#status.403 Unfornately, I cannot configure Nexus to send 404 in this case which would be better for Nexus. |
ea7309b
to
ed7db20
Compare
We have a different behavior for:
|
Please look at apache/maven-integration-testing#348 |
For me it is not a big problem, like connection exception is not common, more important is to have a list of not found artifact per repository |
ed7db20
to
0ff0d74
Compare
and similar for plugins:
|
0ff0d74
to
ca4c13a
Compare
@slawekjaranowski Should I try again? |
you can - but result for connection problems will still show last / one repo |
@cstamas Is this a huge issue to fix in Resolver? |
Unsure, did not look yet, but IMHO is certainly something that would NOT happen in 1.9.x resolver but 2.0.0... |
@michael-o is it any objections from you to merge without resolver fix ... |
Let me please review/test again today. |
As I looked into, this is a problem that probably looks like this: What happens, is that CO during collecting, issues series of requests to ADR to build the graph, but ADR "forwards" these to AR (to resolve POM), and it fails. If you "rollback", you will see that AR throws ArtifactResolutionException that carries a list (repo, cause). So far all nice and dandy. But ADR loses them. Instead, it will create ArtifactDescriptorException w/o cause (so AR thrown ArtifactResolutionEx is lost), instead it uses result, and gets 1st exception from result as "cause". Finally, CO receives this exception and sets it as one single CollectionResult ex and throws DependencyCollectionException w/ result. The problem stems from here: As ADR wants to reuse this message, but to do so, it simply leaves out cause (ArtifactResolutionEx), instead it does this: https://github.com/apache/maven-resolver/blob/master/maven-resolver-api/src/main/java/org/eclipse/aether/resolution/ArtifactDescriptorException.java#L76 |
Went thru resolver cobase, and we have 5 copies of this method: private static Throwable getCause(SomeResultType result) {
Throwable cause = null;
if (result != null && !result.getExceptions().isEmpty()) {
cause = result.getExceptions().get(0);
}
return cause;
} And these methods are bad, as they blindly choose 1st exception as "cause" in various scenarios. They are in classes:
|
My proposal: let's split the issues, have two: one for Maven 3.9 and one for master. Let's merge whatever improvement we have for 3.9 to make 3.9.8 rollin' and see about master branches of Resolver and Maven later down the road. |
if we want it in 3.9 it is not necessary to split - after merge to master I simply cherry-pick it to 3.9.x branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the issues in Resolver and good spirit in @cstamas knowning how to fix, I am fine with the change when my open question is resolved.
I am fine with the merge. |
The build result will be as: