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

IDEA-80032 Support classifiers with Maven sources #2826

Closed
wants to merge 1 commit into from

Conversation

nrayburn-tech
Copy link

This resolves the issue when using a Maven dependency with a classifier. Previously, the sources without the classifier would be downloaded. This modifies the behavior to download the sources with the classifier.

I didn't find any tests to edit for WorkspaceModuleImporter.kt, but if there's something existing I can update it.

A few notes before merging.

  1. I noticed that may be related to my changes is that when viewing the source files, the breadcrumbs at the bottom right show a different source jar depending on where you click within the source file on screen. Sometimes it shows as primefaces-14.0.4-jakarta.jar and sometimes it shows as primefaces-14.0.4-jakarta-sources.jar.
  2. This only adds the prefix for sources, it does not do it for javadocs. I'm not sure if it's the right approach or not for javadocs.
  3. If the sources don't exist with the artifact classifier prefix, then there is not any fallback to the sources without the classifier prefix.

Manual Test Steps

  1. Create a new Java -> Maven project within IntelliJ.
  2. Modify the existing pom.xml with the below dependency.
<dependency>
  <groupId>org.primefaces</groupId>
  <artifactId>primefaces</artifactId>
  <version>14.0.4</version>
  <classifier>jakarta</classifier>
</dependency>
  1. Reload the Maven project and download sources.
  2. Expand the "External Libraries" in the file explorer.
  3. Navigate to Maven: org.primefaces:primefaces:jakarta:14.0.4 and expand the library.
  4. Open the org.primefaces.application.exceptionhandler.PrimeExceptionHandler class and review the imports at the top of the file, confirm they are the jakarta imports.
  5. Remove the classifier from the dependency.
  6. Reload the Maven project and download sources.
  7. Repeat the same general steps to confirm that the javax imports are used now.
  8. Navigate to Maven: org.primefaces:primefaces:14.0.4 and expand the library.
  9. Open the org.primefaces.application.exceptionhandler.PrimeExceptionHandler class and review the imports at the top of the file, confirm they are the javax imports.

Adds classifier as a prefix when automatically attaching or downloading sources
@zulkar
Copy link
Contributor

zulkar commented Aug 26, 2024

Thanks for the patch, unfortunately it cannot be accepted
Classifierm by definition is some optional and arbitrary string that - if present - is appended to the artifact name just after the version number. The classifier distinguishes artifacts that were built from the same POM but differ in content.

So, one cannot simply calculate the source classifier as classifier + "sources".
For your dependency, org.primefaces:primefaces:14.0.4 and your classifier jakarta the source jakarta-sources is correct, indeed. But this is not true for the common case, since it is an arbitrary string, I don't know about such convention and can't find any in official maven docs, please let me know if I'm wrong.

For example, oauth.signpost:signpost-commonshttp4:2.1.1 has an artifact with test classifier, but does not have with test-sources.

One more example: Almost every other maven package has an artifact with javadoc classifier. You can even depend on it, but this does not mean that there should be an artifact withjavadoc-sources classifier. Again, it is an arbitrary string, maven-source-plugin do not generate source artifact automatically for you project with classifier, there is no such convention

@zulkar
Copy link
Contributor

zulkar commented Aug 26, 2024

To address the issue, we should probably test both artifacts, with sources classifier and with classifier+"sources", but it is pure heuristic thing.

Also, regarding for ther code. DependenciesImportingTest#testDependencyWithClassifier is a test for this.

Also, MavenAttachSourcesProvider should be fixed as well probably.

@zulkar zulkar closed this Aug 26, 2024
@nrayburn-tech
Copy link
Author

@zulkar thanks for reviewing.

I think that javadoc and sources are unique classifiers. These classifiers are the default values for maven-javadoc-plugin and maven-sources-plugin so they will be very common.

Given that this only solves a single possible way of naming additional sources, maybe a different approach would be better. Otherwise, there may end up being multiple naming styles requested (${classifier}-sources and sources-${classifier} for example).

Any improvement to using alternative sources would be helpful. Would it be better to discuss ideas within the youtrack issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants