Skip to content

Fix issues with querying Maven artifacts from different repositories #6489

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
Merged
sschuberth merged 5 commits into from
Feb 21, 2023
Merged

Fix issues with querying Maven artifacts from different repositories #6489

sschuberth merged 5 commits into from
Feb 21, 2023

Conversation

@sschuberth
Copy link
Member

@sschuberth sschuberth commented Feb 15, 2023

Please have a look at the individual commit messages for the details.

@sschuberth sschuberth mentioned this pull request Feb 15, 2023
Closed
@sschuberth sschuberth force-pushed the maven-artifact-repo-query-fix branch 3 times, most recently from acd6347 to f0ddcea Compare February 15, 2023 16:20
@sschuberth sschuberth marked this pull request as ready for review February 15, 2023 16:25
@sschuberth sschuberth requested a review from a team as a code owner February 15, 2023 16:25
sschuberth
sschuberth commented Feb 15, 2023
View reviewed changes
sschuberth
sschuberth commented Feb 15, 2023
View reviewed changes
@sschuberth sschuberth force-pushed the maven-artifact-repo-query-fix branch 3 times, most recently from 66068db to af583bf Compare February 16, 2023 15:21
@sschuberth sschuberth requested a review from a team as a code owner February 16, 2023 15:21
mnonnenmacher
mnonnenmacher previously approved these changes Feb 16, 2023
View reviewed changes
Copy link
Member

@mnonnenmacher mnonnenmacher left a comment

Choose a reason for hiding this comment

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

Approved, but it looks like there are more expected test results you need to update.

@sschuberth
Copy link
Member Author

sschuberth commented Feb 16, 2023

Approved, but it looks like there are more expected test results you need to update.

The failing test pass for me locally (also see my message on Slack). Would you mind checking whether GradleKotlinScriptFunTest and SbtFunTest pass locally for you, too?

@sschuberth
Copy link
Member Author

sschuberth commented Feb 16, 2023

On CI, I for example see

https://repo.maven.apache.org/maven2/org/jetbrains/annotations/13.0/annotations-13.0.jar

being used instead of

https://jcenter.bintray.com/org/jetbrains/annotations/13.0/annotations-13.0.jar

@sschuberth sschuberth force-pushed the maven-artifact-repo-query-fix branch from 8cddd27 to 30a6dd0 Compare February 16, 2023 18:10
@mnonnenmacher
Copy link
Member

mnonnenmacher commented Feb 17, 2023

The failing test pass for me locally (also see my message on Slack). Would you mind checking whether GradleKotlinScriptFunTest and SbtFunTest pass locally for you, too?

Yes, tests pass locally.

@sschuberth sschuberth mentioned this pull request Feb 17, 2023
Closed
5 tasks
@sschuberth
Copy link
Member Author

sschuberth commented Feb 17, 2023

Yes, tests pass locally.

As I'm totally clueless by now, I've filed actions/setup-java#454.

@mnonnenmacher
Copy link
Member

mnonnenmacher commented Feb 17, 2023

As I'm totally clueless by now, I've filed actions/setup-java#454.

Did you already try to clear the settings.xml after the setup-java task?

@sschuberth
Copy link
Member Author

sschuberth commented Feb 17, 2023

Did you already try to clear the settings.xml after the setup-java task?

Not after setup-java, but I instructed setup-java to not overwrite it (after I created an empty one).

@sschuberth sschuberth marked this pull request as draft February 20, 2023 16:25
@sschuberth sschuberth force-pushed the maven-artifact-repo-query-fix branch 5 times, most recently from 719717d to eb918e8 Compare February 20, 2023 20:58
fviernau
fviernau previously requested changes Feb 21, 2023
View reviewed changes
@sschuberth sschuberth force-pushed the maven-artifact-repo-query-fix branch from 1c6b117 to 56802f8 Compare February 21, 2023 08:19
@sschuberth sschuberth changed the title fix(MavenSupport): Correct querying of artifact repos declared in POMs Fix issues with querying Maven artifacts from different repositories Feb 21, 2023
@sschuberth sschuberth marked this pull request as ready for review February 21, 2023 09:07
@sschuberth sschuberth requested review from a team, fviernau and mnonnenmacher February 21, 2023 09:08
mnonnenmacher
mnonnenmacher requested changes Feb 21, 2023
View reviewed changes
mnonnenmacher
mnonnenmacher requested changes Feb 21, 2023
View reviewed changes
@sschuberth
refactor(MavenDependencyHandler): Replace a let to reduce nesting
0ad46ef 
Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
@sschuberth
fix(MavenSupport): Correct querying of artifact repos declared in POMs
ed4c21a 
Consider artifact repositories declared in POMs with lowest priority for
Maven (see [1]) and not at all for Gradle (see [2]).

Adjust the expected results for affected tests accordingly. E.g.
`multi-kotlin-project` indeed only declares JCenter as a repository [3].

Fixes #6488.

[1]: https://maven.apache.org/guides/mini/guide-multiple-repositories.html#repository-order
[2]: https://docs.gradle.org/current/userguide/declaring_repositories.html#strict_limitation_to_declared_repositories
[3]: https://github.com/oss-review-toolkit/ort/blob/f6c9386/analyzer/src/funTest/assets/projects/external/multi-kotlin-project/build.gradle.kts#L13

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
@sschuberth
refactor(MavenSupport): Move some variables closer to their use
ae43b4a 
Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
@sschuberth
fix(MavenSupport): Also use the list of repos for the artifact cache key
65be79a 
Previously, the cache for the remote artifacts only used the artifact
coordinates as the cache key. But if the list of repos to query changes,
there may be another match for the same artifact in another repo. Thus,
also take the list of repos into account for the cache key.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
@sschuberth
style(MavenSupport): Simplify logging of local repositories
4879ca5 
The comment was no in-line with the code anyway, so address this by
avoiding the outer condition for the log level completely.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
@sschuberth sschuberth force-pushed the maven-artifact-repo-query-fix branch from 56802f8 to 4879ca5 Compare February 21, 2023 12:02
@sschuberth sschuberth enabled auto-merge (rebase) February 21, 2023 12:02
@sschuberth sschuberth dismissed fviernau’s stale review February 21, 2023 12:02

Comment addressed.

@sschuberth sschuberth merged commit acfd617 into main Feb 21, 2023
@sschuberth sschuberth deleted the maven-artifact-repo-query-fix branch February 21, 2023 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mnonnenmacher mnonnenmacher mnonnenmacher approved these changes

@fviernau fviernau Awaiting requested review from fviernau

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sschuberth @mnonnenmacher @fviernau