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

Use non-deprecated Sink class #7

Merged
merged 1 commit into from
Jan 3, 2023
Merged

Use non-deprecated Sink class #7

merged 1 commit into from
Jan 3, 2023

Conversation

basil
Copy link
Member

@basil basil commented Aug 15, 2022

No description provided.

@basil basil added the internal label Aug 15, 2022
@basil basil marked this pull request as draft August 15, 2022 23:17
@basil basil changed the title Use non-deprecated Sink class Use non-deprecated Sink class Aug 16, 2022
@basil
Copy link
Member Author

basil commented Aug 16, 2022

My build fails with

[ERROR] src/jenkinsci/maven-jellydoc-plugin/maven-jellydoc-plugin/src/main/java/org/jvnet/maven/jellydoc/JellydocMojo.java:[72,7] error: JellydocMojo is not abstract and does not override abstract method generate(Sink,Locale) in MavenReport

Note that I am using

https://github.com/jenkinsci/maven-jellydoc-plugin/blob/master/maven-jellydoc-plugin/pom.xml#L137-L147

[INFO] +- org.apache.maven.reporting:maven-reporting-impl:jar:3.2.0:compile
[INFO] |  +- org.apache.maven.reporting:maven-reporting-api:jar:3.1.1:compile

@michael-osipov Is there a way for me to make Maven Jellydoc Plugin forward compatible with Maven Site Plugin 4, while continuing to have a primary target (and actual usage) of Maven Site Plugin 3?

@basil
Copy link
Member Author

basil commented Jan 2, 2023

Happy New Year @michael-o. Is there a way for me to make Maven Jellydoc Plugin forward compatible with Maven Site Plugin 4, while continuing to have a primary target (and actual usage) of Maven Site Plugin 3?

@michael-o
Copy link

Why do you use impl if you don't implement AbstractMavenReport? In your case you should implement similarly I did with Maven Javadoc Plugin, the you don't need impl since you have a hybrid plugin.

@basil
Copy link
Member Author

basil commented Jan 2, 2023

Why do you use impl

I did a case-insensitive search for the string impl in the two files that use the deprecated Sink class (JellydocMojo.java and ReferenceRenderer.java) and did not find any results. So as far as I can tell I am not using impl. So I do not understand what you are saying.

@michael-o
Copy link

Why do you use impl

I did a case-insensitive search for the string impl in the two files that use the deprecated Sink class (JellydocMojo.java and ReferenceRenderer.java) and did not find any results. So as far as I can tell I am not using impl. So I do not understand what you are saying.

But you depend on it. You can safely remove it first

@basil
Copy link
Member Author

basil commented Jan 2, 2023

But you depend on it.

Here "it" is a pronoun whose antecedent is "impl". I searched pom.xml for the string "impl" but did not find it. So as far as I can tell I do not depend on impl. So I do not understand what you are saying.

@basil
Copy link
Member Author

basil commented Jan 3, 2023

Thanks Michael, I had missed that in my search because I was only looking in the root directory. I tried doing what you suggested via

diff --git a/maven-jellydoc-plugin/pom.xml b/maven-jellydoc-plugin/pom.xml
index 9d65729..0613a26 100644
--- a/maven-jellydoc-plugin/pom.xml
+++ b/maven-jellydoc-plugin/pom.xml
@@ -136,14 +136,8 @@
     </dependency>
     <dependency>
       <groupId>org.apache.maven.reporting</groupId>
-      <artifactId>maven-reporting-impl</artifactId>
-      <version>3.2.0</version>
-      <exclusions>
-        <exclusion>
-          <groupId>dom4j</groupId>
-          <artifactId>dom4j</artifactId>
-        </exclusion>
-      </exclusions>
+      <artifactId>maven-reporting-api</artifactId>
+      <version>3.1.1</version>
     </dependency>

But it turns out that something in this plugin depends on maven-reporting-impl:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.10.1:compile (default-compile) on project maven-jellydoc-plugin: Compilation failure: Compilation failure: 
[ERROR] /home/basil/src/jenkinsci/maven-jellydoc-plugin/maven-jellydoc-plugin/src/main/java/org/jvnet/maven/jellydoc/ReferenceRenderer.java:[18,33] error: cannot find symbol
[ERROR]   symbol:   class AbstractMavenReportRenderer
[ERROR]   location: package org.apache.maven.reporting
[ERROR] /home/basil/src/jenkinsci/maven-jellydoc-plugin/maven-jellydoc-plugin/src/main/java/org/jvnet/maven/jellydoc/ReferenceRenderer.java:[44,39] error: cannot find symbol
[ERROR]   symbol: class AbstractMavenReportRenderer
[ERROR] -> [Help 1]

Since I didn't write this code and have no experience writing Maven plugins, I have no idea what ReferenceRenderer is doing or why it needs AbstractMavenReportRenderer from maven-reporting-impl.

Why do you use impl if you don't implement AbstractMavenReport?

Apparently because ReferenceRenderer needs it.

You can safely remove it first

Apparently not, as described above.

In any case I do not see how switching the dependency from maven-reporting-impl to maven-reporting-api has anything to do with my question about make Maven Jellydoc Plugin forward compatible with Maven Site Plugin 4, while continuing to have a primary target (and actual usage) of Maven Site Plugin 3.

@michael-o
Copy link

Let me check this later today and get back to you

@michael-o
Copy link

michael-o commented Jan 3, 2023

Looking into now. First of all, I would recommend to resolve this problem:

[INFO] --- maven-dependency-plugin:3.3.0:analyze (default-cli) @ maven-jellydoc-plugin ---
[WARNING] Used undeclared dependencies found:
[WARNING]    org.apache.maven.doxia:doxia-sink-api:jar:1.11.1:compile
[WARNING]    org.codehaus.plexus:plexus-utils:jar:3.5.0:compile
[WARNING]    com.sun.xml.txw2:txw2:jar:20110809:compile
[WARNING]    xml-apis:xml-apis:jar:1.3.03:compile
[WARNING]    org.apache.maven.reporting:maven-reporting-api:jar:3.1.1:compile
[WARNING] Unused declared dependencies found:
[WARNING]    jaxen:jaxen:jar:1.2.0:compile
[WARNING]    org.apache.maven:maven-model:jar:3.8.7:provided

@michael-o
Copy link

Here it is, incomplete because generate(3 arg) requires some polish, but it compiles:

osipovmi@deblndw011x:~/var/Projekte/maven-jellydoc-plugin (master *>)
$ git diff -U0
diff --git a/maven-jellydoc-plugin/src/main/java/org/jvnet/maven/jellydoc/JellydocMojo.java b/maven-jellydoc-plugin/src/main/java/org/jvnet/maven/jellydoc/JellydocMojo.java
index 12aa447..7ccd18f 100644
--- a/maven-jellydoc-plugin/src/main/java/org/jvnet/maven/jellydoc/JellydocMojo.java
+++ b/maven-jellydoc-plugin/src/main/java/org/jvnet/maven/jellydoc/JellydocMojo.java
@@ -32 +32 @@ import org.apache.maven.project.MavenProjectHelper;
-import org.apache.maven.reporting.MavenReport;
+import org.apache.maven.reporting.MavenMultiPageReport;
@@ -41 +41,4 @@ import org.apache.tools.ant.types.Path;
-import org.codehaus.doxia.sink.Sink;
+import org.apache.maven.doxia.sink.Sink;
+import org.apache.maven.doxia.sink.SinkFactory;
+import org.apache.maven.doxia.siterenderer.RenderingContext;
+import org.apache.maven.doxia.siterenderer.sink.SiteRendererSink;
@@ -70 +73 @@ import java.util.Locale;
-public class JellydocMojo extends AbstractMojo implements MavenReport {
+public class JellydocMojo extends AbstractMojo implements MavenMultiPageReport {
@@ -208,0 +212,4 @@ public class JellydocMojo extends AbstractMojo implements MavenReport {
+    public void generate(org.codehaus.doxia.sink.Sink sink, Locale locale) throws MavenReportException {
+        generate(sink, null, locale);
+    }
+
@@ -209,0 +217,5 @@ public class JellydocMojo extends AbstractMojo implements MavenReport {
+        generate(sink, null, locale);
+    }
+
+    @Override
+    public void generate(Sink sink, SinkFactory sinkFactory, Locale locale) throws MavenReportException {
diff --git a/maven-jellydoc-plugin/src/main/java/org/jvnet/maven/jellydoc/ReferenceRenderer.java b/maven-jellydoc-plugin/src/main/java/org/jvnet/maven/jellydoc/ReferenceRenderer.java
index 6a532f4..24262fd 100644
--- a/maven-jellydoc-plugin/src/main/java/org/jvnet/maven/jellydoc/ReferenceRenderer.java
+++ b/maven-jellydoc-plugin/src/main/java/org/jvnet/maven/jellydoc/ReferenceRenderer.java
@@ -23 +23 @@ import org.dom4j.io.SAXReader;
-import org.codehaus.doxia.sink.Sink;
+import org.apache.maven.doxia.sink.Sink;

@basil basil marked this pull request as ready for review January 3, 2023 19:20
@basil
Copy link
Member Author

basil commented Jan 3, 2023

Brilliant! Thanks Michael, the pointer to MavenMultiPageReport was all I needed. Now the interfaces and compatibility requirements all make sense.

@basil basil merged commit 7fa49d2 into jenkinsci:master Jan 3, 2023
@basil basil deleted the sink branch January 3, 2023 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants