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.

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

Fix issues with querying Maven artifacts from different repositories #6489

Merged
merged 5 commits into from
Feb 21, 2023
Merged

Fix issues with querying Maven artifacts from different repositories #6489

merged 5 commits into from
Feb 21, 2023

Conversation

sschuberth
Copy link
Member

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

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

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

@mnonnenmacher
Copy link
Member

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

Yes, tests pass locally.

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

@mnonnenmacher
Copy link
Member

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

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
analyzer/src/main/kotlin/managers/utils/MavenSupport.kt Outdated Show resolved Hide resolved
@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
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 enabled auto-merge (rebase) February 21, 2023 12:02
@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
@sschuberth sschuberth added the release notes Changes that should be mentioned in release notes label Feb 23, 2023
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
release notes Changes that should be mentioned in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sschuberth @mnonnenmacher @fviernau