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.

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

Fix for Dependencies with classifier and SNAPSHOT #3787

Merged
merged 1 commit into from Nov 11, 2021

Conversation

fmarot
Copy link
Contributor

@fmarot fmarot commented Nov 9, 2021

Fixes issue #3786
Dependencies with a classifier and SNAPSHOT version cannot be resolved bug

Fixes Issue #3786

Description of Change

Since recent changes to workaround MSHARED-998 the use of the class ArtifactResult appeared in the code. While this class looks like ArtifactCoordinate, we have to take care of the getVersion() method because for SNAPSHOT versions it provides the timestamped result, not the 'real' version.

So I only changed the getVersion() by getBaseVersion() call for class ArtifactResult.

I have tested the change manually with a fake project and it solves the DependencyNotFoundException as expected.

Have test cases been added to cover the new functionality?

no, not yet, sorry...

Fixes issue jeremylong#3786
Dependencies with a classifier and SNAPSHOT version cannot be resolved bug
@boring-cyborg boring-cyborg bot added the maven changes to the maven plugin label Nov 9, 2021
@fmarot
Copy link
Contributor Author

fmarot commented Nov 9, 2021

ping @aikebah as you may want to review my (minor) change on the method sameArtifact(...) you recently added to work around MSHARED-998

@fmarot
Copy link
Contributor Author

fmarot commented Nov 9, 2021

For an explanation of getVersion() VS getBaseVersion() here is a comment by Robert Scholte of Maven: https://stackoverflow.com/questions/19196836/use-case-of-artifact-getbaseversion#comment28405796_19196934

@aikebah
Copy link
Collaborator

aikebah commented Nov 9, 2021

LGTM

@aikebah
Copy link
Collaborator

aikebah commented Nov 9, 2021

Thanks for the PR @fmarot overlooked this scenario, which has bitten me in other cases in the past. When I read your ping and the summary it was immediately obvious to me what the change was about

@aikebah aikebah added this to the 6.5.1 milestone Nov 11, 2021
@aikebah aikebah merged commit 078ab0f into jeremylong:main Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maven changes to the maven plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants