-
Notifications
You must be signed in to change notification settings - Fork 6
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
[MSHARED-1327] Change default value of output directory in AbstractMavenReport #26
Conversation
I am expecting tests to fail after that change, if they assert on Another idea is to simply fold this change into #25, because it is contextually linked to MSHARED-1326. In that case, we could discard this PR. |
I will read your messages and this one. It is very likely that this change it correct. |
Please also report a JIRA issue. |
OK, if you wish to handle it separately from MSHARED-1326, I can do that. |
@michael-o: I have created MSHARED-1327 and force-pushed the amended commit with issue ID prefix and corrected |
src/main/java/org/apache/maven/reporting/AbstractMavenReport.java
Outdated
Show resolved
Hide resolved
@hboutemy, WDYT? Build directory or rather a subdir? I tend to a subdir for consistency reasons. |
@kriegaex I have rebased this PR.
all in target. Not ideal. @hboutemy What is your opinion? I still would prefer a subdir. @kriegaex, would you keep it in the build output dir directory? What is your opinion on the clutter? |
Good catch! My use case with AspectJ reporting is a multi-page report, for which always a subdirectory is created anyway. Honestly, when fixing the ITs here, I did not check whether there was additional clutter written into the build directory, because I think that each mojo writing more than a single file - say PDF, HTML or whatever - ought to take care of creating a subdir for itself anyway, because the same clutter would also end up in the reporting directory with the old default, dangerously mixing with the output of other plugins. So arguably, the demo plugin in the IT does not behave well. If the equation |
It is not that easy. Even if a plugin uses a subdir in the shared one, there are still assets from the skin which need to be present to properly render the report in the browser. They are irrelevant for an external one, but everything else it is not. Overriding the assets isn't a problem because they all use the same skin. This is harmless. |
Then, I think I am with you concerning the idea to have a dedicated common base directory for reports, because now you just gave me a good reason for it: common assets from the skin. I had not thought about that before. Of course, the current default I can adjust the PR, if we converge on a name for the report directory. I see you guys in the lead here, @michael-o and @hboutemy. Most directories in my build directory seem to prefer plural to singular, i.e. "reports" rather than "report", e.g. "its", "classes", "generated-sources" and notably "surefire-reports". The latter should probably default to be inside the new common base directory we are discussing here in the future, just like "apidocs" from Maven Javadoc. |
I still stand with |
1da470a
to
fd2fc4a
Compare
I rebased and force-pushed the PR with the updated directory.
I am not sure what that means, but it sounds like due diligence, which is a good thing. 🙂 |
By chain I mean all of our components and all reporting plugins. The path to other reports from one need to be calculated dynamically. |
The output directory now defaults to ${project.build.directory}/reports instead of ${project.reporting.outputDirectory}. This represents a clear separatation between standalone invocation compared to site generation. This closes apache#26
The output directory now defaults to ${project.build.directory}/reports instead of ${project.reporting.outputDirectory}. This represents a clear separatation between standalone invocation compared to site generation. This closes apache#26
I have pushed two more pending commits are the prerequisite to this one. Let's see how our plugins work now. |
Was the documentation updated to reflect this change? |
This hasn't been merged yet since I need to test this through all of our reporting plugins first. If this gets merged then it will appear in the release notes and here |
@kriegaex, look what I have found:
I think we need to make a proper design decision whether |
@michael-o, the way I understand it, the description applies to what I call the base directory, i.e. the The description IMO says nothing whatsoever about the plugin-specific report output directory, e.g. Of course, |
You completely misunderstood what I was trying to say. I was solely refering to the fact that this |
Hm, right. Maybe it should not be read-only.
Sorry, I read the sentence three times and still do not understand it. Can you elaborate, please?
I think, as much as possible should be configurable, because chances are that some users want to change it. Don't misunderstand me, as a user I probably would not change it in my own projects. I am all in for convention over configuration, simply because I am lazy and like good defaults. OTOH, there are always special situations, often due to the fact that developers changed one default already, indirectly forcing themselves to change another one, too, which in turn is actually a reason for more conventions and less configurability. KISS! But many aspects of Maven are configurable already, so where does that leave us? 😉 |
…venReport The output directory now defaults to ${project.build.directory}/reports instead of ${project.reporting.outputDirectory}. This represents a clear separatation between standalone invocation compared to site generation. This closes apache#26
…venReport The output directory now defaults to ${project.build.directory}/reports instead of ${project.reporting.outputDirectory}. This represents a clear separatation between standalone invocation compared to site generation. This closes apache#26
…venReport The output directory now defaults to ${project.build.directory}/reports instead of ${project.reporting.outputDirectory}. This represents a clear separatation between standalone invocation compared to site generation. This closes apache#26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested this through the entire chain: works for me and with dynamic calculation of paths this is really nice.
After my PR apache/maven-reporting-impl#26 for https://issues.apache.org/jira/browse/MSHARED-1327 was merged, the base directory for reports in AbstractMavenReport when calling mojos stand-alone, i.e. outside a site generation context, no longer defaults to 'target/site' but now to 'target/reports'. This is good, because reports for a Maven site can look different from reports for a single module. As AjcReportMojo extends AbstractMavenReport, we can respect the new default easily and even save lines of code and documentation, because we can just inherit the 'outputDirectory' property instead of overriding and documenting it.
The default superdirectory was changed by apache/maven-reporting-impl#26 and introduced in apache/maven-javadoc-plugin#204.
The default superdirectory was changed by apache/maven-reporting-impl#26 and introduced in apache/maven-javadoc-plugin#204.
The output directory now defaults to
${project.build.directory}
instead of${project.reporting.outputDirectory}
. Quoting my reasoning from apache/maven-jxr@ae81a34#commitcomment-130639906:The javadoc is, BTW, still correct and does not need to be changed: In a site generation context, the reporting base directory will be set here automatically without any further changes due to: