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

[MPLUGIN-437] Fixes to the plugin descriptor generation #167

Merged
merged 1 commit into from
Oct 28, 2022
Merged

[MPLUGIN-437] Fixes to the plugin descriptor generation #167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 19 additions & 10 deletions ...ache/maven/tools/plugin/extractor/annotations/JavaAnnotationsMojoDescriptorExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,21 +540,30 @@ protected void extendJavaProjectBuilderWithSourcesJar( JavaProjectBuilder builde
return;
}

// extract sources to target/maven-plugin-plugin-sources/${groupId}/${artifact}/sources
File extractDirectory = new File( request.getProject().getBuild().getDirectory(),
if ( sourcesArtifact.getFile().isFile() )
{
// extract sources to target/maven-plugin-plugin-sources/${groupId}/${artifact}/sources
File extractDirectory = new File( request.getProject().getBuild().getDirectory(),
"maven-plugin-plugin-sources/" + sourcesArtifact.getGroupId() + "/"
+ sourcesArtifact.getArtifactId() + "/" + sourcesArtifact.getVersion()
+ "/" + sourcesArtifact.getClassifier() );
extractDirectory.mkdirs();
extractDirectory.mkdirs();

UnArchiver unArchiver = archiverManager.getUnArchiver( "jar" );
unArchiver.setSourceFile( sourcesArtifact.getFile() );
unArchiver.setDestDirectory( extractDirectory );
unArchiver.extract();
UnArchiver unArchiver = archiverManager.getUnArchiver( "jar" );
unArchiver.setSourceFile( sourcesArtifact.getFile() );
unArchiver.setDestDirectory( extractDirectory );
unArchiver.extract();

extendJavaProjectBuilder( builder,
Arrays.asList( extractDirectory ),
request.getDependencies() );
extendJavaProjectBuilder( builder,
Arrays.asList( extractDirectory ),
request.getDependencies() );
}
else if ( sourcesArtifact.getFile().isDirectory() )
{
extendJavaProjectBuilder( builder,
Arrays.asList( sourcesArtifact.getFile() ),
request.getDependencies() );
}
}
catch ( ArchiverException | NoSuchArchiverException e )
{
Expand Down
8 changes: 0 additions & 8 deletions .../apache/maven/tools/plugin/extractor/annotations/converter/JavaClassConverterContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
import com.thoughtworks.qdox.JavaProjectBuilder;
import com.thoughtworks.qdox.builder.TypeAssembler;
import com.thoughtworks.qdox.library.ClassNameLibrary;
import com.thoughtworks.qdox.library.SourceLibrary;
import com.thoughtworks.qdox.model.JavaClass;
import com.thoughtworks.qdox.model.JavaField;
import com.thoughtworks.qdox.model.JavaModule;
Expand Down Expand Up @@ -81,13 +80,6 @@ public JavaClassConverterContext( JavaClass mojoClass, JavaClass declaringClass,
Map<String, MojoAnnotatedClass> mojoAnnotatedClasses,
JavadocLinkGenerator linkGenerator, int lineNumber )
{
if ( !( mojoClass.getJavaClassLibrary() instanceof SourceLibrary )
Copy link
Member

Choose a reason for hiding this comment

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

why has this been removed? Access to import statements is still crucial for resolving javadoc references.

Copy link
Contributor Author

@gnodet gnodet Oct 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I did remove the fact that the generation will throw an exception, not the fact that it will be used. Resolving javadoc reference is an order less important than having the plugin generate the descriptor. In my use case, it was throwing an exception and failed the whole build.

Copy link
Member

Choose a reason for hiding this comment

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

I understand this, but how can it work correctly? The exception was a good indicator to ease debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, in my case, the parent class was not part of the module being built, and the declaringClass.getJavaClassLibrary() is an instance of ClassLoaderLibrary.
The generation will still work, even if some links can not be resolved. But the plugin as a whole can not fail for this reason. And it's a valid use case, so even a warning may be too much, unless it can be disabled.

Copy link
Member

Choose a reason for hiding this comment

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

I think at least a warning is reasonable if javadoc links cannot be resolved.

Copy link
Member

Choose a reason for hiding this comment

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

Also how can meta information from the parent mojos be extracted which are not part of the binary (i.e. coming from javadoc like description, since or deprecated)? I think this requires evaluation of the parent's plugin descriptor....

Copy link
Contributor Author

@gnodet gnodet Oct 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Also how can meta information from the parent mojos be extracted which are not part of the binary (i.e. coming from javadoc like description, since or deprecated)? I think this requires evaluation of the parent's plugin descriptor....

I guess they don't. But in my case, the plugin is used internally during the build, so the documentation is not much of a problem.

Instead of warning upfront, I think it would be better to display a warning at the time the information can not be retrieved. That would avoid irrelevant warnings to be displayed. For example if the parent class does not contain any parameter definition, a warning would be irrelevant noise.

Copy link
Member

Choose a reason for hiding this comment

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

Agree, not sure how to accomplish this with QDox, though.

|| !( declaringClass.getJavaClassLibrary() instanceof SourceLibrary ) )
{
throw new IllegalStateException( "The given javaClasses must be loaded by SourceLibrary to have access to"
+ "e.g. the imports statements in it" );
}

this.mojoClass = mojoClass;
this.declaringClass = declaringClass;
this.javaProjectBuilder = javaProjectBuilder;
Expand Down