Re: How to set default directories when developing a reporting plugin?
On 2023/10/22 02:38:07 Alexander Kriegisch wrote: > Hello Michael, hello community. > > I have decided to actually change the plugin's behaviour the way I > described: > > -- I overrode the default value of AbstractMavenReport.outputDirectory > [1] by defining a setter method and adding a @Parameter annotation, > specifying ${project.build.directory} as the base direcory for the > report, because this is what I want to use when the mojo is > executed directly. The abstract super class defaults to > ${project.reporting.outputDirectory}, which is IMO suboptimal. In a > site generation context, the report directory is set automatically > anyway, because AbstractMavenReport.setReportOutputDirectory, as > mentioned in my original message, sets both reportOutputDirectory > and outputDirectory. BTW, is this "trick" to override an inherited > property correct? Is there a canonical way to do that? > > -- Then, I introduced a new reportDirectory [2] property, whis is just > a String, not a File. It specifies the subdirectory to be appended > to outputDirectory [3]. This works in both use cases, stand-alone > mojo execution and site generation. > > -- Hence, it is now possible to configure the base directory and the > subdirectory name separately: The base directory is set in the > identically named property outputDirectory, either in the plugin or > in Maven Site, depending on the use case. The subdirectory is set > in the new reportDirectory property. I.e., reports will end up in > the following directories: >** without configuration, stand-alone: > target/aspectj-report >** without configuration, site generation: > target/site/aspectj-report >** custom base directory, stand-alone: e.g. > my-target/aspectj-report >** custom base directory, site generation: e.g. > target/my-site/aspectj-report >** custom subdirectory, stand-alone: e.g. > target/my-aspectj-report >** custom subdirectory, site generation: e.g. > target/site/my-aspectj-report >** custom base + subdirectory, stand-alone: e.g. > my-target/my-aspectj-report >** custom base + subdirectory, site generation: e.g. > target/my-site/my-aspectj-report > Notably, the custom base directory for site generation should work > for all plugins playing nicely, e.g. Javadoc or (now after my > change) AspectJ. See the integration tests [4] for examples, namely > CreateReport* test cases for stand-alone and CreateSite* ones for > site generation use cases. For each test case, compare the > differences in Maven Site and AspexctJ Maven settings and the > corresponding checks in verify.groovy. > > [1] > https://github.com/dev-aspectj/aspectj-maven-plugin/blob/9f2d0e97ed963c2b69a7fdf731a4c919a6bb56ef/src/main/java/org/codehaus/mojo/aspectj/AjcReportMojo.java#L90-L106 > [2] > https://github.com/dev-aspectj/aspectj-maven-plugin/blob/9f2d0e97ed963c2b69a7fdf731a4c919a6bb56ef/src/main/java/org/codehaus/mojo/aspectj/AjcReportMojo.java#L57-L67 > [3] > https://github.com/dev-aspectj/aspectj-maven-plugin/blob/9f2d0e97ed963c2b69a7fdf731a4c919a6bb56ef/src/main/java/org/codehaus/mojo/aspectj/AjcReportMojo.java#L255 > [4] https://github.com/dev-aspectj/aspectj-maven-plugin/tree/main/src/it Just checked these. They look reasonable. I need to check MJAVADOC whether it is reasonable or not. Let's continue with the open PRs. - To unsubscribe, e-mail: users-unsubscr...@maven.apache.org For additional commands, e-mail: users-h...@maven.apache.org
Re: How to set default directories when developing a reporting plugin?
Michael, first of all thanks for your conceptual thoughts and efforts to improve both code and documentation. > Let me know what you think! I fully agree that the notion that both the output directory and the report output directory are meant to be *base* directories. I disagree, however, with that they should default to the same base directory. Hence https://github.com/apache/maven-reporting-impl/pull/26. I am more in the Maven Javadoc "fan club" in this case. Regards -- Alexander Kriegisch https://scrum-master.de Michael Osipov schrieb am 23.10.2023 00:53 (GMT +07:00): > Alexander, > > I have now read the code of all our reporting plugins, > maven-reporting-impl and maven-site-plugin. > > Here is my understanding how I expect to work which it is not at the > moment and there is a flaw in the system were both MJXR and MJAVADOC > apply a hack and I will explain why they do. > > We have > > standalone execution: >> @Parameter(defaultValue = "${project.reporting.outputDirectory}", >> readonly >> = true, required = true) >> protected File outputDirectory; > > and > > site execution: >> @Parameter(property = "siteOutputDirectory", defaultValue = >> "${project.reporting.outputDirectory}") >> protected File outputDirectory; > > both point to root directory where all reports put there files in, from > the simplest single page one, to multipage to those which are external > and operate in subdirectories. Everything below that output directory is > fixed and completely subject to the plugin. > > Both o.a.m.reporting.AbstractMavenReport.getReportOutputDirectory() and > o.a.m.reporting.AbstractMavenReport.setReportOutputDirectory(File) exist > for the sole purpose to accomondate both execution styles. One value is > exected through DI (standalone) the other one set (through site). > Therefore, in both cases #getReportOutputDirectory() should be used to > get the right value to write to. > In any case #setReportOutputDirectory(File) does *not* contain a > directory for the reporting plugin only, but for the entire generated > tree of plugins, regardless of 'mvn site' /or/ 'mvn pmd:pmd > checkstyle:checkstyle'. > > Now, o.a.m.reporting.MavenReport.getOutputName() has bad documentation > because it is actually a relative output *path* to the generated item > with the base/root path given by > o.a.m.reporting.MavenReport.getReportOutputDirectory(). Again, here is > the name completely deceiving. Needs doc update as well. > > You might wonder, what to do now with: >> @Override >> public String getOutputName() { >> return "xref/index"; >> } > > and > >> @Override >> public void setReportOutputDirectory(File reportOutputDirectory) { >> if ((reportOutputDirectory != null) >> && >> (!reportOutputDirectory.getAbsolutePath().endsWith("xref"))) >> { >> this.destDir = new File(reportOutputDirectory, >> "xref").getAbsolutePath(); >> } else { >> this.destDir = reportOutputDirectory.getAbsolutePath(); >> } >> } > ? > > Well, while I consider the former is correct, the latter is not: > * Does one expect to pass *null* as reportOutputDirectory? > * Why is *this.reportOutputDirectory* never updated? > * It expects the report this live under *.../xref*, but ignores >> /** >> * Folder where the Xref files will be copied to. >> */ >> @Parameter(defaultValue = "${project.reporting.outputDirectory}/xref") >> private String destDir; > * It mixes the the notion of a *pluginReportOutputDirectory* which is > passed to the external report generator as a root/base or a multi sink > if you want to generate multiple reports under one dir, but have the > starting point at *foo/index* (output name) and all resides under *foo/*. > MJAVADOC does the same trick although it has replaces the literal 'xref' > with a field, but the issue remains the same. > Whether the path component(s) between the root and *index* should be > flexible, is upto the plugin (e.g, xref, xref-test, apidocs, etc.), but > from my personal PoV changing it does not make: > * In site execution this has to be carried along > * If you need to change it otherwise you can rename/move it with an ant > task or a subprocessor. > > What I will do not is to create a few issues and PRs to improve the > situation and create a PR for JXR on top of 'doxia-2.0.0' how I consider > it should be. Some of the method names are unfortunately, but I am > afraid it is too hard to change them given they have too widespread use. > > I guess after all, I will need to perform another round of releases with > those PRs before I can start new reporting plugin releases. I think what > you have raised was long overdue and absoltuely legit. > > Let me know what you think! > > @Hervé, I would also appreciate to hear your opinion on this! > > Michael > > ---
RE: How to set default directories when developing a reporting plugin?
Alexander, I have now read the code of all our reporting plugins, maven-reporting-impl and maven-site-plugin. Here is my understanding how I expect to work which it is not at the moment and there is a flaw in the system were both MJXR and MJAVADOC apply a hack and I will explain why they do. We have standalone execution: @Parameter(defaultValue = "${project.reporting.outputDirectory}", readonly = true, required = true) protected File outputDirectory; and site execution: @Parameter(property = "siteOutputDirectory", defaultValue = "${project.reporting.outputDirectory}") protected File outputDirectory; both point to root directory where all reports put there files in, from the simplest single page one, to multipage to those which are external and operate in subdirectories. Everything below that output directory is fixed and completely subject to the plugin. Both o.a.m.reporting.AbstractMavenReport.getReportOutputDirectory() and o.a.m.reporting.AbstractMavenReport.setReportOutputDirectory(File) exist for the sole purpose to accomondate both execution styles. One value is exected through DI (standalone) the other one set (through site). Therefore, in both cases #getReportOutputDirectory() should be used to get the right value to write to. In any case #setReportOutputDirectory(File) does *not* contain a directory for the reporting plugin only, but for the entire generated tree of plugins, regardless of 'mvn site' /or/ 'mvn pmd:pmd checkstyle:checkstyle'. Now, o.a.m.reporting.MavenReport.getOutputName() has bad documentation because it is actually a relative output *path* to the generated item with the base/root path given by o.a.m.reporting.MavenReport.getReportOutputDirectory(). Again, here is the name completely deceiving. Needs doc update as well. You might wonder, what to do now with: @Override public String getOutputName() { return "xref/index"; } and @Override public void setReportOutputDirectory(File reportOutputDirectory) { if ((reportOutputDirectory != null) && (!reportOutputDirectory.getAbsolutePath().endsWith("xref"))) { this.destDir = new File(reportOutputDirectory, "xref").getAbsolutePath(); } else { this.destDir = reportOutputDirectory.getAbsolutePath(); } } ? Well, while I consider the former is correct, the latter is not: * Does one expect to pass *null* as reportOutputDirectory? * Why is *this.reportOutputDirectory* never updated? * It expects the report this live under *.../xref*, but ignores /** * Folder where the Xref files will be copied to. */ @Parameter(defaultValue = "${project.reporting.outputDirectory}/xref") private String destDir; * It mixes the the notion of a *pluginReportOutputDirectory* which is passed to the external report generator as a root/base or a multi sink if you want to generate multiple reports under one dir, but have the starting point at *foo/index* (output name) and all resides under *foo/*. MJAVADOC does the same trick although it has replaces the literal 'xref' with a field, but the issue remains the same. Whether the path component(s) between the root and *index* should be flexible, is upto the plugin (e.g, xref, xref-test, apidocs, etc.), but from my personal PoV changing it does not make: * In site execution this has to be carried along * If you need to change it otherwise you can rename/move it with an ant task or a subprocessor. What I will do not is to create a few issues and PRs to improve the situation and create a PR for JXR on top of 'doxia-2.0.0' how I consider it should be. Some of the method names are unfortunately, but I am afraid it is too hard to change them given they have too widespread use. I guess after all, I will need to perform another round of releases with those PRs before I can start new reporting plugin releases. I think what you have raised was long overdue and absoltuely legit. Let me know what you think! @Hervé, I would also appreciate to hear your opinion on this! Michael - To unsubscribe, e-mail: users-unsubscr...@maven.apache.org For additional commands, e-mail: users-h...@maven.apache.org
Re: How to set default directories when developing a reporting plugin?
Hello Michael, hello community. I have decided to actually change the plugin's behaviour the way I described: -- I overrode the default value of AbstractMavenReport.outputDirectory [1] by defining a setter method and adding a @Parameter annotation, specifying ${project.build.directory} as the base direcory for the report, because this is what I want to use when the mojo is executed directly. The abstract super class defaults to ${project.reporting.outputDirectory}, which is IMO suboptimal. In a site generation context, the report directory is set automatically anyway, because AbstractMavenReport.setReportOutputDirectory, as mentioned in my original message, sets both reportOutputDirectory and outputDirectory. BTW, is this "trick" to override an inherited property correct? Is there a canonical way to do that? -- Then, I introduced a new reportDirectory [2] property, whis is just a String, not a File. It specifies the subdirectory to be appended to outputDirectory [3]. This works in both use cases, stand-alone mojo execution and site generation. -- Hence, it is now possible to configure the base directory and the subdirectory name separately: The base directory is set in the identically named property outputDirectory, either in the plugin or in Maven Site, depending on the use case. The subdirectory is set in the new reportDirectory property. I.e., reports will end up in the following directories: ** without configuration, stand-alone: target/aspectj-report ** without configuration, site generation: target/site/aspectj-report ** custom base directory, stand-alone: e.g. my-target/aspectj-report ** custom base directory, site generation: e.g. target/my-site/aspectj-report ** custom subdirectory, stand-alone: e.g. target/my-aspectj-report ** custom subdirectory, site generation: e.g. target/site/my-aspectj-report ** custom base + subdirectory, stand-alone: e.g. my-target/my-aspectj-report ** custom base + subdirectory, site generation: e.g. target/my-site/my-aspectj-report Notably, the custom base directory for site generation should work for all plugins playing nicely, e.g. Javadoc or (now after my change) AspectJ. See the integration tests [4] for examples, namely CreateReport* test cases for stand-alone and CreateSite* ones for site generation use cases. For each test case, compare the differences in Maven Site and AspexctJ Maven settings and the corresponding checks in verify.groovy. [1] https://github.com/dev-aspectj/aspectj-maven-plugin/blob/9f2d0e97ed963c2b69a7fdf731a4c919a6bb56ef/src/main/java/org/codehaus/mojo/aspectj/AjcReportMojo.java#L90-L106 [2] https://github.com/dev-aspectj/aspectj-maven-plugin/blob/9f2d0e97ed963c2b69a7fdf731a4c919a6bb56ef/src/main/java/org/codehaus/mojo/aspectj/AjcReportMojo.java#L57-L67 [3] https://github.com/dev-aspectj/aspectj-maven-plugin/blob/9f2d0e97ed963c2b69a7fdf731a4c919a6bb56ef/src/main/java/org/codehaus/mojo/aspectj/AjcReportMojo.java#L255 [4] https://github.com/dev-aspectj/aspectj-maven-plugin/tree/main/src/it -- Alexander Kriegisch https://scrum-master.de Michael Osipov schrieb am 22.10.2023 01:17 (GMT +07:00): > > > On 2023/10/20 02:08:25 Alexander Kriegisch wrote: >> Hello. >> >> I am trying to improve a reporting mojo in a Maven plugin. The mojo >> extends org.apache.maven.reporting.AbstractMavenReport. I recently >> upgraded it to Doxia 2.0 - thanks again to Hervé B. and Michael O. for >> their support - and cleaned out some cruft, removing redundant super >> class fields and methods) in the mojo code. So far, so good. >> >> My question is about the different behaviour for configuring the report >> output directory when starting the mojo directly as a Maven goal >> compared to it being started during Maven site generation. The Javadoc >> is helpful and explains: >> >> >> >> public abstract class AbstractMavenReport extends AbstractMojo >> implements MavenMultiPageReport >> { >> /** >>* The output directory for the report. Note that this parameter is >>* only evaluated if the goal is run directly from the command line. >>* If the goal is run indirectly as part of a site generation, the >>* output directory configured in the Maven Site Plugin is used >>* instead. >>*/ >> @Parameter(defaultValue = "${project.reporting.outputDirectory}", >> readonly = true, required = true) >> protected File outputDirectory; >> >> >> >> I.e., if the user sets the directory as a plugin option, she is fine >> when using the plugin directly. If she uses it as a reporting plugin for >> site generation, she needs to set 'outpu
RE: How to set default directories when developing a reporting plugin?
On 2023/10/20 02:08:25 Alexander Kriegisch wrote: Hello. I am trying to improve a reporting mojo in a Maven plugin. The mojo extends org.apache.maven.reporting.AbstractMavenReport. I recently upgraded it to Doxia 2.0 - thanks again to Hervé B. and Michael O. for their support - and cleaned out some cruft, removing redundant super class fields and methods) in the mojo code. So far, so good. My question is about the different behaviour for configuring the report output directory when starting the mojo directly as a Maven goal compared to it being started during Maven site generation. The Javadoc is helpful and explains: public abstract class AbstractMavenReport extends AbstractMojo implements MavenMultiPageReport { /** * The output directory for the report. Note that this parameter is * only evaluated if the goal is run directly from the command line. * If the goal is run indirectly as part of a site generation, the * output directory configured in the Maven Site Plugin is used * instead. */ @Parameter(defaultValue = "${project.reporting.outputDirectory}", readonly = true, required = true) protected File outputDirectory; I.e., if the user sets the directory as a plugin option, she is fine when using the plugin directly. If she uses it as a reporting plugin for site generation, she needs to set 'outputDirectory' in Maven Site Plugin. But actually, that is not the right approach, because my understanding is that 'outputDirectory' should rather set a base directory for *all* reporting plugins, if the default target/site is not OK. In that case, any plugin-specific output directory ought to be interpreted as a subdirectory of the general reporting 'outputDirectory', not simply be ignored. I saw that Maven Javadoc Plugin has some logic that seems to handle that, using a separate 'destDir' property to recalculate the output directory in a reporting context. From class JavadocReport: /** * @param theDestDir the destination directory */ public void setDestDir(String theDestDir) { this.destDir = theDestDir; updateReportOutputDirectory(reportOutputDirectory, theDestDir); } private void updateReportOutputDirectory(File reportOutputDirectory, String destDir) { if (reportOutputDirectory != null && destDir != null && !reportOutputDirectory.getAbsolutePath().endsWith(destDir)) { this.reportOutputDirectory = new File(reportOutputDirectory, destDir); } else { this.reportOutputDirectory = reportOutputDirectory; } } Different Maven plugins seem to handle this situation differently. Is this the suggested, canonical approach? What do you think is a user's expectation for multi-page report mojos extending AbstractMavenReport? Certainly not that three conflicting reporting plugins have to argue about which one can use Maven Site's 'outputDirectory' proprty for its private purposes, leaving the other reporting plugins falsely configured. Do all reporting plugins need to provide a separate 'destDir' or 'reportingDir' property for that use case? WDYT? Sorry for asking a long-winded question, but I think it is important for me to provide some context for the benefit of everyone trying to answer. This is, indeed, a good question. I have stumbled about similar during Doxia 2.0.0 work, but didn't dare to touch it because I didn't want to open too many construction sites. Let me go through to get a better understanding. Regarding Javadoc Plugin: I don't know whether is the perfect external reporting plugin because it does not inherit from AbstractMavenReport. M
Re: How to set default directories when developing a reporting plugin?
One more thing: In class AbstractMavenReport, field 'reportOutputDirectory' is private (not protected), i.e. it cannot be updated independently by extending mojos. Using the setter method also updates 'outputDirectory' simultaneously. The latter is a protected field, BTW. Of course, the setter method could be overridden or the overriding class could somehow cater to the sacred knowledge of the side effect in 'setReportOutputDirectory'. Interestingly, Maven Javadoc Plugin's class JavadocReport simply defines its own private 'reportOutputDirectory' field, not extending AbstractMavenReport at all. I would, however, rather stick with the approach to use the abstract base class and extend it the right way, based on expert suggestions here. -- Alexander Kriegisch https://scrum-master.de Alexander Kriegisch schrieb am 20.10.2023 09:08 (GMT +07:00): > Hello. > > I am trying to improve a reporting mojo in a Maven plugin. The mojo > extends org.apache.maven.reporting.AbstractMavenReport. I recently > upgraded it to Doxia 2.0 - thanks again to Hervé B. and Michael O. for > their support - and cleaned out some cruft, removing redundant super > class fields and methods) in the mojo code. So far, so good. > > My question is about the different behaviour for configuring the report > output directory when starting the mojo directly as a Maven goal > compared to it being started during Maven site generation. The Javadoc > is helpful and explains: > > > > public abstract class AbstractMavenReport extends AbstractMojo > implements MavenMultiPageReport > { > /** >* The output directory for the report. Note that this parameter is >* only evaluated if the goal is run directly from the command line. >* If the goal is run indirectly as part of a site generation, the >* output directory configured in the Maven Site Plugin is used >* instead. >*/ > @Parameter(defaultValue = "${project.reporting.outputDirectory}", > readonly = true, required = true) > protected File outputDirectory; > > > > I.e., if the user sets the directory as a plugin option, she is fine > when using the plugin directly. If she uses it as a reporting plugin for > site generation, she needs to set 'outputDirectory' in Maven Site > Plugin. But actually, that is not the right approach, because my > understanding is that 'outputDirectory' should rather set a base > directory for *all* reporting plugins, if the default target/site is not > OK. In that case, any plugin-specific output directory ought to be > interpreted as a subdirectory of the general reporting > 'outputDirectory', not simply be ignored. I saw that Maven Javadoc > Plugin has some logic that seems to handle that, using a separate > 'destDir' property to recalculate the output directory in a reporting > context. From class JavadocReport: > > > > /** > * @param theDestDir the destination directory > */ > public void setDestDir(String theDestDir) { > this.destDir = theDestDir; > updateReportOutputDirectory(reportOutputDirectory, theDestDir); > } > > private void updateReportOutputDirectory(File reportOutputDirectory, > String destDir) > { > if (reportOutputDirectory != null >&& destDir != null >&& !reportOutputDirectory.getAbsolutePath().endsWith(destDir)) > { > this.reportOutputDirectory = > new File(reportOutputDirectory, destDir); > } else { > this.reportOutputDirectory = > reportOutputDirectory; > } > } > > > > Different Maven plugins seem to handle this situation differently. Is > this the suggested, canonical approach? What do you think is a user's > expectation for multi-page report mojos extending AbstractMavenReport? > Certainly not that three conflicting reporting plugins have to argue > about which one can use Maven Site's 'outputDirectory' proprty for its > private purposes, leaving the other reporting plugins falsely > configured. Do all reporting plugins need to provide a separate > 'destDir' or 'reportingDir' property for that use case? WDYT? > > Sorry for asking a long-winded question, but I think it is important for > me to provide some context for the benefit of everyone trying to answer. > > Thanks in advance. Cheers - To unsubscribe, e-mail: users-unsubscr...@maven.apache.org For additional commands, e-mail: users-h...@maven.apache.org