Use maven-metadata-local.xml for file:// paths
#4512
Conversation
|
|
||
| if ("file".equals(scheme)) { | ||
| // A maven repository can be expressed as a URI with a file scheme | ||
| Path path = Paths.get(URI.create(uri)); | ||
| Path path = Paths.get(URI.create(baseUri + "maven-metadata-local.xml")); |
There was a problem hiding this comment.
In theory it should be maven-metadata-<repo-id>.xml, and the <localRepository>’s id is local.
I don’t know if it’s very common to have another local repository though. Maybe when people are running CI on the same server as the Maven repo?
There was a problem hiding this comment.
Yes my main goal here was to have a better chance of matching an existing file, while not yet perfect. I'd say this is better than falling through to deriving that information needlessly further down.
| @@ -256,23 +256,22 @@ public MavenMetadata downloadMetadata(GroupArtifactVersion gav, @Nullable Resolv | |||
| boolean cacheEmptyResult = false; | |||
| try { | |||
| String scheme = URI.create(repo.getUri()).getScheme(); | |||
| String uri = repo.getUri() + (repo.getUri().endsWith("/") ? "" : "/") + | |||
| String baseUri = repo.getUri() + (repo.getUri().endsWith("/") ? "" : "/") + | |||
| requireNonNull(gav.getGroupId()).replace('.', '/') + '/' + | |||
| gav.getArtifactId() + '/' + | |||
There was a problem hiding this comment.
This bit is duplicated at lines 259-261 and 538-540. I wonder if the URI building logic (and possibly more) wouldn’t be better suited in the MavenRepository class itself.
There was a problem hiding this comment.
I understand & appreciate the suggestion; in general we try to keep the classes under .tree. free of logic; they are intended mostly as a representation of the underlying source, with anything operating on those classes typically extracted elsewhere, like traits or in helper classes like this one. Since we only use this pattern twice (so far) I'd opt to for now keep that bit as-is. But definitely something to keep in mind if it repeats more often or elsewhere, thanks!
There was a problem hiding this comment.
I wanted to mean that it was duplicated 3 times. The third time is actually at lines 356-358.
I didn’t check the whole thing, but the metadata is per-repository. The local repository keeps track of some remote repository metadata which might be outdated indeed, but I think the code doesn’t use it since it always fetches the metadata from the remote repository. Since it downloads the metadata from all repositories and then merges it, I guess it’s fine? (assuming the merge is correct) |
|
Thanks for helping think through the options here! Indeed we merge the metadata as also seen on this PR With that I think we should be safe to merge this as an improvement for a commonly expected case. |
What's changed?
Use
maven-metadata-local.xmlwhen accessingfile://URIs, as documented onWhat's your motivation?
Improve odds of picking up an existing local file, as opposed to downloading metadata remotely.
Anything in particular you'd like reviewers to focus on?
Presumably the local metadata might be outdated; should we guard against that somehow?
Anyone you would like to review specifically?
@DidierLoiseau
Have you considered any alternatives or workarounds?
Locally I also see
maven-metadata-central.xmlas well as a few other repo-ids; Should we account for those?Any additional context