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

Jump to bottom

Create parent POM module #874

Closed
wants to merge 4 commits into from
Closed

Create parent POM module #874

wants to merge 4 commits into from

Conversation

acoburn
Copy link
Contributor

@acoburn acoburn commented Aug 8, 2015

https://jira.duraspace.org/browse/FCREPO-1684

This is a first stab at cleaning up the maven configuration. The idea is that this will eliminate a lot of the redundant maven configuration across modules that may not inherit directly from the fcrepo maven POM. As a guiding principle, I created the fcrepo-parent to be the sort of POM artifact that I wish existed when configuring fcrepo-camel and fcrepo-camel-toolbox. I have verified separately that fcrepo-camel builds correctly when using fcrepo-parent as a parent POM (i.e. after eliminating most of the existing plugin configuration in fcrepo-camel/pom.xml).

I have some open questions related to this PR:

  • I removed a number of dependencies that were not used (directly or transitively), including: javax.mail, enunciate(?), commons-compress, jaxb, javassist. I also removed joda-time as a direct dependency: it is not used directly and is only imported transitively via modeshape-jcr.
  • I left the enunciate and jmeter plugins in the fcrepo pom file, though I'm not sure they are being used. I also didn't touch the lifecycle-mapping plugin: I don't use eclipse, but others do, so I assume it is relevant for them.
  • Plugin configuration, for the most part, was moved to the fcrepo-parent POM, but anything that didn't seem truly applicable to a wide range of projects stayed in the fcrepo POM. I left the enforcer plugin configuration in the fcrepo POM, since those rules may not apply to other projects.
  • I am not particularly familiar with the mvn site and the release/deployment-related phases, so I would appreciate it if @awoods could verify that this configuration actually does what I intend it to do: that is, to make deployment, site generation and releases easier and more consistent across projects.

@acoburn acoburn mentioned this pull request Aug 9, 2015
Merged
ajs6f
ajs6f reviewed Aug 10, 2015
View reviewed changes
fcrepo-kernel-modeshape/pom.xml
@@ -8,7 +8,7 @@
</parent>

<artifactId>fcrepo-kernel-modeshape</artifactId>
<name>Fedora Repository Kernel Implementation based on Modeshape</name>
<name>Fedora Repository Kernel Implementation (Modeshape)</name>
<description>The Fedora Commons repository kernel: Provides the basic abstractions at the heart of the Fedora
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this description say something about Modeshape?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should.

@ajs6f
Copy link
Contributor

ajs6f commented Aug 10, 2015

I think the jmeter stuff is toast-worthy, at this point. The enunciate plugin was pretty cool, although we never got it working really well. It might be worth taking another stab at that. The lifecycle-mapping stuff shouldn't be there. People who use Eclipse (like me) should do that stuff in their own preferences and not in the pom.xmls.

@awoods
Copy link

awoods commented Aug 11, 2015

Please rebase on master.

@acoburn
Copy link
Contributor Author

acoburn commented Aug 11, 2015

@awoods this branch is up to date with regard to master

awoods
awoods reviewed Aug 14, 2015
View reviewed changes
pom.xml
@@ -162,47 +143,34 @@
<version>${guava.version}</version>
</dependency>
<dependency>
<groupId>org.apache.abdera</groupId>
<artifactId>abdera-parser</artifactId>
Copy link

Choose a reason for hiding this comment

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

Who did you determine that abdera-parser is not needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Abdera had something to do with RSS, right?

Copy link

Choose a reason for hiding this comment

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

Yes. https://abdera.apache.org/
I am not concerned about it going away, but more interested in the technique for determining it is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My basic technique was this: what packages does the artifact export? And are they used in the code anywhere?

Sometimes, there is a transitive dependency conflict and it is necessary to explicitly load a particular version so that the class loader can resolve dependencies properly, but this did not appear to be the case with abdera.

awoods
awoods reviewed Aug 25, 2015
View reviewed changes
pom.xml
<execution>
<id>default-integration-test</id>
<goals>
<goal>integration-test</goal>
Copy link

Choose a reason for hiding this comment

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

The recommendation from the Maven docs seems to suggest our original configuration of the plugin, i.e. having both integration-test and verify.
https://maven.apache.org/surefire/maven-failsafe-plugin/usage.html

@awoods
Copy link

awoods commented Aug 25, 2015

The updates look good. In doing some testing I have run into issues with the following:

cd fcrepo-message-consumer
mvn javadoc:test-aggregate
cd fcrepo-module-auth-xacml
mvn javadoc:jar
cd fcrepo4-swordserver
mvn javadoc:test-aggregate

@acoburn
Copy link
Contributor Author

acoburn commented Aug 26, 2015

@awoods: I have added some additional PRs for the projects listed, related to this change. See the Jira ticket for details.

@awoods
Copy link

awoods commented Aug 26, 2015

Resolved with: 99397f6

@awoods awoods closed this Aug 26, 2015
@acoburn acoburn deleted the fcrepo-1684 branch September 9, 2015 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@acoburn @ajs6f @awoods