[
https://issues.apache.org/jira/browse/HBASE-6145?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13287995#comment-13287995
]
Jesse Yates commented on HBASE-6145:
------------------------------------
A pretty in depth analysis, hopefully not too much:
Patch didn't apply cleanly with git, but got it go with basic patch command
(mostly - looks like you might be a little behind in the docbkx?).
I'm just going to step through per pom…
hbase-server/pom.xml
{quote}
+ <version>${avro.version}</version>
{quote}
This (and the rest of the dependency info) should be in the parent pom's
dependencyManagement section. Just as general style, if another module wants to
use avro, they should just declare the dependency, and not worry about making
sure they have the right version, excludes, etc (I know we are dropping avro
soon, but we should still do the right thing).
Also, looks like your spacing is off for the added dependencies - 2 spaces, not
tabs in xml.
hbase-common/pom.xml
{quote}
+ <plugins>
+ <plugin>
+ <artifactId>maven-surefire-plugin</artifactId>
{quote}
This can/should stay in the pluginManagement section. Surefire is part of the
default maven things to run, so it will just pick up the
configuration/executions from the management section - this also keeps all the
surefire stuff in the same sections in all poms.
Also, does anything actually happen if we remove:
{quote}
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-site-plugin</artifactId>
{quote}
from this pom? It may create the target/site (side effect of site being a core
part of maven, so all modules can respond to it), but is any actual work done?
Okay, onto the parent pom (/hbase/pom.xml):
Why the removal of the hbase-assembly module? In the official docs, it actually
says to use an assembly module. This is particularly poignant because the
alternative is to use the assembly:assembly descriptor, which is deprecated…
The docs say that you can use assembly:assembly from within the parent pom (but
doesn't say what that actually means) - any way to tie the assembly:single
phase to call the assembly:single/:assembly phase in the children poms?
In src/assembly/all.xml, this comment is no longer applicable.
{quote}
<!-- This is only necessary until maven fixes the intra-project dependency
bug
in maven 3.0. Until then, we have to include the test jars for
sub-projects. When
fixed, the below dependencySet stuff is sufficient for pulling in the
test jars as
well, as long as they are added as dependencies in this project. Right
now, we only
have 1 submodule to accumulate, but we can copy/paste as necessary until
maven is
fixed. -->
{quote}
Also, it would be awesome to move file set matching to a more general regex,
rather than tying it to the maven property (which is defined in the main pom).
General nit: I prefer having the properties above the build, since the
properties are used in the build section, but that's just style.
{quote}
<!--Pass -DskipJavadoc=true on command-line to skip javadoc building-->
{quote}
when building the site? Also, can't you just pass in -DskipJavadoc?
{quote}
<version>${maven.assembly.version}</version>
{quote}
and
{quote}
<version>${maven.site.version}</version>
{quote}
Would be nice to add:
{code}
<!--$NO-MVN-MAN-VER$-->
{code}
to the end of the lines to remove the eclipse warning
{quote}
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>xml-maven-plugin</artifactId>
{quote}
configuration formatting is off.
{quote}
<execution>
<id>copy-docbkx</id>
<goals>
<goal>copy-resources</goal>
</goals>
<phase>pre-site</phase>
<configuration>
<outputDirectory>target/site</outputDirectory>
<resources>
<resource>
<directory>${basedir}/target/docbkx</directory>
<includes>
<include>**/**</include>
</includes>
</resource>
</resources>
</configuration>
</execution>
</executions>
<configuration>
<escapeString>\</escapeString>
</configuration>
{quote}
Is the escape string new here? What property are you avoiding overriding? Also,
why are you moving them in the first place? You can just set the target
directory to be ${basedir}/target/docbkx in the docbkx plugin:
{code}
<groupId>com.agilejava.docbkx</groupId>
<artifactId>docbkx-maven-plugin</artifactId>
{code}
Here, you can also move the common traits to the 'top level' configuration for
the plugin, then just put the differences in each execution. For example:
{code}
<plugin>
<groupId>com.agilejava.docbkx</groupId>
<artifactId>docbkx-maven-plugin</artifactId>
<version>2.0.14</version>
<inherited>false</inherited>
<dependencies>
<!--… some stuff -->
</dependencies>
<configuration>
<!--common configuration stuff here -->
</configuration>
<executions>
<execution>
<id>multipage</id>
<goals>
<goal>generate-html</goal>
</goals>
<phase>pre-site</phase>
<configuration>
<!-- multi-page configuration stuff here -->
…
{code}
This also seems excessive:
{quote}
<plugin>
<artifactId>maven-resources-plugin</artifactId>
<version>${maven.resources.plugin.version}</version>
<inherited>false</inherited>
<executions>
<execution>
<id>copy-javadocs</id>
<goals>
<goal>copy-resources</goal>
</goals>
<phase>pre-site</phase>
{quote}
With the aggregate goal, you can just have all the javadocs copied into the
right directory in this module when you build them; at least that is what is
was doing before.
{quote}
<artifactId>maven-assembly-plugin</artifactId>
<version>${maven.assembly.version}</version>
<inherited>false</inherited>
<configuration>
{quote}
Whitespace is messed up for the configuration section (and actually the whole
section - spaces, not tabs!)
It would be sweet if we can get the eclipse natures/inclusions fixed for
{quote}
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-eclipse-plugin</artifactId>
<version>2.8</version>
</plugin>
{quote}
This way the hbase top level folder wouldn't have a java nature by default.
Also, for instance, we can add the assembly, docbkx, etc stuff for /hbase as
src folders, and for hbase-server finally fix having to fix the classpath when
you load it into eclipse (or do eclipse:eclipse). See:
http://maven.apache.org/plugins/maven-eclipse-plugin/examples/specifying-source-path-inclusions-and-exclusions.html
The latter might be a little out of scope for this ticket, but if you have the
time...
Other than that, looks good to me (didn't try building/testing yet). Good stuff
stack.
> Fix site target post modularization
> -----------------------------------
>
> Key: HBASE-6145
> URL: https://issues.apache.org/jira/browse/HBASE-6145
> Project: HBase
> Issue Type: Task
> Reporter: stack
> Assignee: stack
> Attachments: site.txt, site2.txt, sitev3.txt
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
