Skip to content

fix: add groupId before version number #29437

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

Closed
wants to merge 4 commits into from

Conversation

dratler
Copy link

@dratler dratler commented Jan 16, 2022

No description provided.

@u3r
Copy link

u3r commented Jan 16, 2022

Hi @dratler, this always adds the groupid. I didn't provide this solution as it changes the status quo.

Adding a configuration option (as mentioned in the issue) would involve patching through the option from the plugin into the ArtifactsLibararies or even configure a naming strategy.

For me personally this solution is perfectly ok - but I didn't spend time to think of implications of very long group/artifact-name combinations or other possible clashes.

@dratler
Copy link
Author

dratler commented Jan 16, 2022

Hi @u3r ,
I have added it and also on tests add ref to getClassifier() it's also part of the solution

assertThat(this.libraryCaptor.getAllValues().get(1).getName()).isEqualTo("g2-artifact-1.0.jar");
assertThat(this.libraryCaptor.getAllValues().get(0).getName()).isEqualTo(this.libraryCaptor.getAllValues().get(1).getName());
assertThat(this.libraryCaptor.getAllValues().get(0).getName()).isEqualTo("g1-artifact-g1-1.0-cl-1.jar");
assertThat(this.libraryCaptor.getAllValues().get(1).getName()).isEqualTo("g1-artifact-g1-1.0-cl-1.jar");
Copy link

Choose a reason for hiding this comment

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

Isn't this the thing to avoid? According to the test name?
So shouldn't
a) the input to these tests have at least one differing field so the resulting filenames are different or
b) throw an error as packaging two artifacts with identical coordinates seem fishy or
c) (least likely, considering (b) ) Add something to discern the files?

Can anyone official and familiar with the specific semantic of the plugin chime in, what expected output for Artifacts with identical coordinates should be or where this is filtered/addressed?

Copy link
Member

Choose a reason for hiding this comment

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

Maven doesn't allow multiple dependencies with identical coordinates.

Copy link
Author

Choose a reason for hiding this comment

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

I will be working on :

  1. Duplicate Entry
  2. Entry with diffrent Art id
  3. Entry with diffrent Group Ip
  4. Entry with diffrent Version Number
    So all scenarios needs to be checked

@@ -164,12 +164,15 @@ private boolean isLocal(Artifact artifact) {

private String getFileName(Artifact artifact) {
StringBuilder sb = new StringBuilder();
sb.append(artifact.getArtifactId()).append("-").append(artifact.getBaseVersion());
sb.append(artifact.getArtifactId()).append("-").append(artifact.getGroupId()).append("-")
Copy link
Member

@wilkinsona wilkinsona Jan 18, 2022

Choose a reason for hiding this comment

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

Rather than doing this here, I think it would be better to reuse the existing logic that prepends the group ID when a duplicate is detected. A configuration option should be added so that, when enabled, the group ID is always prepended irrespective of whether or not a duplicate has been detected.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @wilkinsona ,
method doWithLibraries it is using method getDuplicates and it's using getFileName and on loop for every file \ Artifacts we are using that method this is why I thought we can fix only at method getFileName
how can I inject that opinion ?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @wilkinsona ,
do you think that getDuplicates should return Artifices and then it can be better compare ?

Copy link
Member

@wilkinsona wilkinsona Jan 24, 2022

Choose a reason for hiding this comment

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

I don't think there's anything to compare. The file name already contains the artifact ID, version, and any classifier. Maven guarantees that within a group, each combination of artifact ID, version, and classifier will be unique. Therefore, prepending the group ID is sufficient to avoid a duplicate entry name.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Jan 18, 2022
@wilkinsona
Copy link
Member

How's it going, @dratler?

@wilkinsona
Copy link
Member

Thanks for you efforts here, @dratler. Given the lack of progress in the last few weeks and my comment on #29390, I think we need to take a step back and think some more about exactly what we want to do here. Thanks anyway.

@wilkinsona wilkinsona closed this Feb 14, 2022
@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants