You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "cstamas (via GitHub)" <gi...@apache.org> on 2023/06/29 21:49:02 UTC
[GitHub] [maven-resolver] cstamas opened a new pull request, #309: [MRESOLVER-376] Bugfix for relocated artifacts
cstamas opened a new pull request, #309:
URL: https://github.com/apache/maven-resolver/pull/309
The code uses Artifact to key (forever during collection) the read artifact descriptors, but, this is not always what we want.
Example: ArtifactDescriptorReader when reads a relocated artifact, it will use RelocatedArtifact (in maven-resolver-provider) as result.artifact, and BF use this artifact as key. But, this artifact will be key of relocation target, while the descriptor is the descriptor of original (relocated) artifact. As it gets into (inner) async resolver cache, it will stay there forever. But alas, we next ask for relocation target decriptor, that as can be seen, use same (key-wise, or GAV wise) key as the relocated artifact, and nothing happens, as computeIfAbsent does not compute (as it is present), and we fall into endless loop.
---
https://issues.apache.org/jira/browse/MRESOLVER-376
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] caiwei-ebay2 commented on pull request #309: [MRESOLVER-376] Bugfix for relocated artifacts
Posted by "caiwei-ebay2 (via GitHub)" <gi...@apache.org>.
caiwei-ebay2 commented on PR #309:
URL: https://github.com/apache/maven-resolver/pull/309#issuecomment-1614413759
@cstamas
With the details you shared and combined with debugging, I fully understand the case. This case is happened with version range + relocation, this is why I cannot reproduce the issue with a relocation artifact like
`<dependency>
<groupId>mysql</groupId>
<artifactId>mysql-connector-java</artifactId>
<version>8.0.33</version>
</dependency>`
Both fixes are good. And I prefder the fix in 311, I have tried that and it worked like a charm.
https://github.com/apache/maven-resolver/pull/311
Thanks again for your quick triaging and fix!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #309: [MRESOLVER-376] Bugfix for relocated artifacts
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #309:
URL: https://github.com/apache/maven-resolver/pull/309#issuecomment-1614311710
What patch actually does:
* introduces `replace` boolean flag, that is `false` always,
* EXCEPT when it is about relocation, as then it assumes
* that A1 descriptor is cached under A2 key
* so when it goes to get "real" A2 descriptor, it simply "forces" and replaces it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #309: [MRESOLVER-376] Bugfix for relocated artifacts
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #309:
URL: https://github.com/apache/maven-resolver/pull/309#issuecomment-1614435311
Created bug to examing range+redirect case https://issues.apache.org/jira/browse/MRESOLVER-379
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #309: [MRESOLVER-376] Bugfix for relocated artifacts
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #309:
URL: https://github.com/apache/maven-resolver/pull/309#issuecomment-1614328597
The more i think, I believe this PR -- while fixes the issue as @snjeza reported -- is wrong and artifact mixup used for key needs to be resolved :smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #309: [MRESOLVER-376] Bugfix for relocated artifacts
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #309:
URL: https://github.com/apache/maven-resolver/pull/309#issuecomment-1614309073
Yes, please reproduce, it is really consistent (in JIRA there is a commit hash to use, if they changed anything in git)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #309: [MRESOLVER-376] Bugfix for relocated artifacts
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #309:
URL: https://github.com/apache/maven-resolver/pull/309#issuecomment-1614346729
I think this PR is to be superseded by this "proper fix": https://github.com/apache/maven-resolver/pull/311
@snjeza @caiwei-ebay2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] snjeza commented on pull request #309: [MRESOLVER-376] Bugfix for relocated artifacts
Posted by "snjeza (via GitHub)" <gi...@apache.org>.
snjeza commented on PR #309:
URL: https://github.com/apache/maven-resolver/pull/309#issuecomment-1613928729
This PR fixes https://github.com/redhat-developer/vscode-java/issues/3085 and https://issues.apache.org/jira/browse/MRESOLVER-376
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] caiwei-ebay2 commented on pull request #309: [MRESOLVER-376] Bugfix for relocated artifacts
Posted by "caiwei-ebay2 (via GitHub)" <gi...@apache.org>.
caiwei-ebay2 commented on PR #309:
URL: https://github.com/apache/maven-resolver/pull/309#issuecomment-1613935345
@cstamas @snjeza
Looks good to me, thanks for the fix!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] caiwei-ebay2 commented on pull request #309: [MRESOLVER-376] Bugfix for relocated artifacts
Posted by "caiwei-ebay2 (via GitHub)" <gi...@apache.org>.
caiwei-ebay2 commented on PR #309:
URL: https://github.com/apache/maven-resolver/pull/309#issuecomment-1614097696
@cstamas
A few questions.
BF patch do considers relocated artifact but does not work for this case, May I know what's the special thing of this relocation artifact?
From my original implementation, it is like this:
A1 will be relocates to A2, so first we resolve A1, we cache A2's descriptor with A2's key, next resolve A2, A2 is already there.
This is different with the behavior as you observed.
"it will use RelocatedArtifact (in maven-resolver-provider) as result.artifact, and BF use this artifact as key. But, this artifact will be key of relocation target, while the descriptor is the descriptor of original (relocated) artifact"
Let me try reproduce the issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas closed pull request #309: [MRESOLVER-376] Bugfix for relocated artifacts
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas closed pull request #309: [MRESOLVER-376] Bugfix for relocated artifacts
URL: https://github.com/apache/maven-resolver/pull/309
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #309: [MRESOLVER-376] Bugfix for relocated artifacts
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #309:
URL: https://github.com/apache/maven-resolver/pull/309#issuecomment-1614300205
Things to be aware:
* RelocatedArtifact (implemented in Maven) https://github.com/apache/maven/blob/maven-3.9.x/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/RelocatedArtifact.java
* ArtifactDescriptorReader (also implemented in Maven): https://github.com/apache/maven/blob/maven-3.9.x/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultArtifactDescriptorReader.java#L299-L308
* async resolver has a Map<Artifact, ArtifactDescriptor> that uses computeIfAbsent, hence if key once added, will not be re-added during resolution
Steps happening:
* BF asks for `junit:junit-dep:4.11` descriptor async resolution
* descriptor reader returns `ArtifactDescriptorResult` that carries `RelocatedArtifact` as d.artifact (that contains GAVs for relocation target `junit:junit:4.11`) while descriptor contains `junit:junit-dep:4.11` data, so even redirects are there
* this descriptor is being put into map (computeIfAbsent) as `junit:junit:4.11` -> `ArtifactDescriptorResult` (that of `junit:junit-dep:4.11`
* next, relocation target `junit:junit:4.11` descriptor is asked, but the entry above is returned (that is actually descriptor of `junit;junit-dep:4.11` that AGAIN contains relocation
* and so on, relocation target is being asked for over and over again, but the resolver map returns relocated artifact descriptor over and over again, so "constant relocation loop happens".
@caiwei-ebay2 yes, very simple to reproduce, and this above is what I saw during debug (and locally added some logging). Problem is that `RelocatedArtifact` is completely opaque in resolver (as it is implemented in maven-resolver-provider), as IMHO simplest would be something like this (but there is no such method either):
```
Artifact key = artifactDescriptor.artifact
if (key instance of RelocatedArtifact) {
key = ((RelocatedArtifact) key).getOriginalArtifact();
}
```
Ultimately, problem is this "mixup" that in the map, the relocated artifact descriptor is stored under relocation target key, and here a loop starts, as relocation target mapping will again return relocated artifact descriptor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #309: [MRESOLVER-376] Bugfix for relocated artifacts
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #309:
URL: https://github.com/apache/maven-resolver/pull/309#issuecomment-1614327105
So, IMHO the problem is somewhere around here: https://github.com/apache/maven-resolver/blob/maven-resolver-1.9.13/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/bf/BfDependencyCollector.java#L425
You do `args.resolver.cacheVersionRangeDescriptor(dr.artifact, dr))` BUT:
* `dr` is A1 descriptor
* `dr.artifact` is A2 (RelocatedArtifact)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #309: [MRESOLVER-376] Bugfix for relocated artifacts
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #309:
URL: https://github.com/apache/maven-resolver/pull/309#issuecomment-1613855291
@caiwei-ebay ping
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #309: [MRESOLVER-376] Bugfix for relocated artifacts
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #309:
URL: https://github.com/apache/maven-resolver/pull/309#issuecomment-1614307240
> From my original implementation, it is like this: A1 will be relocates to A2, so first we resolve A1, we cache A2's descriptor with A2's key, next resolve A2, A2 is already there.
So problem is:
* "we first resolve A1" -> but DescriptorReader will return A2 as descriptor.artifact (RelocatedArtifact implementation)
* you stuff this into a map (and use key descriptor.artifact, that is A2)
* "next resolver A2" -> and you get A1 descriptor (that relocates to A2...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #309: [MRESOLVER-376] Bugfix for relocated artifacts
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #309:
URL: https://github.com/apache/maven-resolver/pull/309#issuecomment-1614428957
Superseded by #311
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] snjeza commented on pull request #309: [MRESOLVER-376] Bugfix for relocated artifacts
Posted by "snjeza (via GitHub)" <gi...@apache.org>.
snjeza commented on PR #309:
URL: https://github.com/apache/maven-resolver/pull/309#issuecomment-1614550741
> I think this PR is to be superseded by this "proper fix": https://github.com/apache/maven-resolver/pull/311
@snjeza @caiwei-ebay2
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org