stardog-union / pellet

Pellet is an OWL 2 reasoner in Java; open source (AGPL) and commercially licensed, commercial support available.
http://clarkparsia.com/pellet
Other
302 stars 153 forks source link

Convert build to use Maven #1

Closed ansell closed 10 years ago

ansell commented 11 years ago

The build could be converted to use Maven. All of the necessary dependencies are now in Maven Central. The main complication is that there are some core modules with circular code dependencies, so the code for those modules needs to be combined into a single common module before a Maven multi-module build will work easily.

Let me know if you want me to create a patch and open a Pull Request to fix this.

tobias-hammerschmidt commented 10 years ago

@evren Would upstream accept the build system conversion to maven if this would be provided as patches/pull request or are there any objections regarding maven usage? We're really interested in making this happen. I'm not sure whether this can be based on the fork of @ansell or if this has already diverged too much.

ansell commented 10 years ago

I think they have diverged too much for a direct conversion as it isn't as simple as just adding pom.xml files in this case.

Converting to Maven requires that the core modules that have interdependencies be compiled into a single module. Currently, the Ant build adds all of the core modules together when bootstrapping them (target=compile-root), but that isn't easy with Maven. https://github.com/clarkparsia/pellet/blob/master/build.xml#L232

I don't mind redoing the patch as the set of Git operations required are quite simple and I should be able to reuse most of the pom.xml files.

tobias-hammerschmidt commented 10 years ago

Since we're interested in commercial support/commercial license the licenses are also an important topic. I've seen that you split the aterm jar into a separate project (as well as jjtraveler) - are they only licensed under GPLv3? I tried to track down the source of aterm once but I'm not sure which license applied to it.

By the way I would also like to help with the maven conversion.

kendall commented 10 years ago

aterm is LGPL'd in the version we use. Then they switched to 3-clause BSD. Either way, we're fine in re: licensing.

Cheers, Kendall

On Wed, Jan 15, 2014 at 3:47 AM, Tobias Hammerschmidt < notifications@github.com> wrote:

Since we're interested in commercial support/commercial license the licenses are also an important topic. I've seen that you split the aterm jar into a separate project (as well as jtraveller) - are they only licensed under GPLv3? I tried to track down the source of aterm once but I'm not sure which license applied to it.

— Reply to this email directly or view it on GitHubhttps://github.com/clarkparsia/pellet/issues/1#issuecomment-32343580 .

ansell commented 10 years ago

I built aterms and shared-objects (same repository) and jjtraveller (separate repository) from the cwi-swat GitHub repositories, with the addition of maven scripts in my fork. My sources are:

https://github.com/ansell/aterms/tree/ansellpatches/aterm-java https://github.com/ansell/aterms/tree/ansellpatches/shared-objects https://github.com/ansell/jjtraveler/tree/ansellpatches

The original sources are:

https://github.com/cwi-swat/aterms/tree/master/aterm-java https://github.com/cwi-swat/aterms/tree/master/shared-objects https://github.com/cwi-swat/jjtraveler/tree/master

The licenses as Kendall says are now BSD-3-Clause:

https://github.com/cwi-swat/aterms/blob/master/aterm-java/COPYING https://github.com/cwi-swat/aterms/blob/master/shared-objects/COPYING https://github.com/cwi-swat/jjtraveler/blob/master/COPYING

ansell commented 10 years ago

My mavenised versions have also been deployed to Maven Central:

http://search.maven.org/#search|ga|1|g%3A%22com.github.ansell.aterms%22 http://search.maven.org/#search|ga|1|g%3A%22com.github.ansell.jjtraveler%22

tobias-hammerschmidt commented 10 years ago

Ok guess we're cool then from license perspective :-) Since no objections where raised against maven in this issue I assume that you'll merge a mavenized version once there is a open pull request?

evren commented 10 years ago

We don't use Maven internally but we are not against it either. One issue with @ansell's pellet fork is that it is based on a forked owlapi that has various changes. I'm sure that's useful in itself but we'd prefer the Pellet maven build to depend on the official owlapi release. If users can configure maven to select which version of owlapi they want to use (official version vs. fork), that'd be fine.

On Wed, Jan 15, 2014 at 3:01 PM, Tobias Hammerschmidt < notifications@github.com> wrote:

Ok guess we're cool then from license perspective :-) Since no objections where raised against maven in this issue I assume that you'll merge a mavenized version once there is a open pull request?

— Reply to this email directly or view it on GitHubhttps://github.com/clarkparsia/pellet/issues/1#issuecomment-32408996 .

ansell commented 10 years ago

I am fine with reworking the patches to rely on the official upstream, as long as the META-INF/services file for the OWLReasonerFactory is maintained (which shouldn't affect anyone else).

evren commented 10 years ago

Yes, sure, that'd be fine.

On Wed, Jan 15, 2014 at 3:21 PM, Peter Ansell notifications@github.comwrote:

I am fine with reworking the patches to rely on the official upstream, as long as the META-INF/services file for the OWLReasonerFactory is maintained (which shouldn't affect anyone else).

— Reply to this email directly or view it on GitHubhttps://github.com/clarkparsia/pellet/issues/1#issuecomment-32410840 .

tobias-hammerschmidt commented 10 years ago

:+1:

ansell commented 10 years ago

The other main question before reworking the patches is, what is the preference for the core module structure. In my fork I have maintained the core modules as empty shells and added a single pellet-common dependency for them all.

Should I keep that structure or just compress down to a single pellet-common (or pellet-core?) module?

tobias-hammerschmidt commented 10 years ago

A clear dependency analysis would be good for sure (guess you also invested some time on that). I can't comment on the pellet owlapi bindings but at least for the jena part the version 2.3.1 doesn't seem to depend on xsdlib and relaxngDatatype anymore. This is right now in your pellet common artifact but doesn't seem to be so 'common' at all (correct me if I'm wrong).

evren commented 10 years ago

I don't have a strong preference either way. Single pellet-core module sounds good.

On Wed, Jan 15, 2014 at 3:26 PM, Peter Ansell notifications@github.comwrote:

The other main question before reworking the patches is, what is the preference for the core module structure. In my fork I have maintained the core modules as empty shells and added a single pellet-common dependency for them all.

Should I keep that structure or just compress down to a single pellet-common (or pellet-core?) module?

— Reply to this email directly or view it on GitHubhttps://github.com/clarkparsia/pellet/issues/1#issuecomment-32411303 .

ansell commented 10 years ago

I will look into removing the xsdlib and relaxngDatatype dependencies, but they may still be necessary as from memory there were compile errors before I found Maven Central matching versions for them.

Single pellet-core it will be.

I should be able to get onto it when I get to work this morning.

evren commented 10 years ago

The libraries xsdlib and relaxNg were required by pellet core in the past but those dependencies have been removed in 8dbff90.

tobias-hammerschmidt commented 10 years ago

The failure cause for the JenaExplanationTest in the maven branch seems to be related to the used aterm library. If the aterm version which is part of the ant build is used the test runs (finishes) fine - it is only failing (not really failing but rather stuck) with the aterm version of @ansell (https://github.com/ansell/aterms). @ansell - any idea whats causing this? Is there any particular reason for depending on the updated (concluding from the version number) aterm library?

ansell commented 10 years ago

I just depended on the latest version as it worked for me. I don't however use the Jena module so there may be some subtle conflict with Jena.

If someone can identify the matching good version in their Git repository, then I will publish the older aterms version to Maven Central and we can use that instead until we figure out why the more recent version conflicts. May also need to publish matching good versions of shared-objects and jjtraveler ( https://github.com/ansell/jjtraveler )

tobias-hammerschmidt commented 10 years ago

In this case we might simply publish the aterm jar from pellet master. This also includes jjtraveler and shared-objects so we don't need to publish those.

ansell commented 10 years ago

There is not much of a difference in publishing three artifacts to Maven Central compared to one, so I don't mind doing it.

I will look at the 1.6 aterms release in their Git repository and match it datewise up to shared-objects and jjtraveler and release them this week. Then if it is still broken we can work on finetuning its provenance from there.

evren commented 10 years ago

I don't think the problem is in the ATerm library. These tests pass for me when run inside Eclipse but fail when run through maven command line. I can't tell what is going yet but I'll investigate this further.

On Tue, May 6, 2014 at 9:24 PM, Peter Ansell notifications@github.comwrote:

There is not much of a difference in publishing three artifacts to Maven Central compared to one, so I don't mind doing it.

I will look at the 1.6 aterms release in their Git repository and match it datewise up to shared-objects and jjtraveler and release them this week. Then if it is still broken we can work on finetuning its provenance from there.

— Reply to this email directly or view it on GitHubhttps://github.com/clarkparsia/pellet/issues/1#issuecomment-42379545 .

tobias-hammerschmidt commented 10 years ago

Just to make sure - the mechanicalEngineeringTest in class AbstractClassificationTest has currently an Ignore annotation on it. Do you have removed that before testing? This is also failing in eclipse for me not only when run as maven build. Again the test will succeed both in eclipse as well as in maven when using another aterm library so I still think this should be the issue. I even profiled this test case and it seems to be stuck (at least from perception) somewhere in tableau codeline.

As a reference the aterm library I used is http://dev.biordf.net/maven/aterm/aterm-java/1.6/aterm-java-1.6.jar.

tobias-hammerschmidt commented 10 years ago

@ansell any update regarding the publishing of the aterm lib? If you don't have time to look into the aterm git repo maybe you can simply publish the aterm-java from the biordf maven repo to maven central?

ansell commented 10 years ago

Evren said that he didn't think the issue was in the aterm library so I didn't follow on with the release. I don't want to publish the combined jar file to central, as it doesn't fit with the way the original software was published and the way the authors structure their source repositories.

If Evren thinks that trying 1.6 is worthwhile I will put it back on my todo list.

evren commented 10 years ago

I disabled that one test case related to MechanicalEngineering ontology which is an extreme case of a hard reasoning problem mostly due to bad modeling. I fixed all the other failing test cases which were due to unrelated issues. For example, JenaExplanationTests were failing due to a mismatch in Jena locator configuration. I think we can continue for now with that test disabled and I'll take a look at it later. But there is still a question about what is different in com.github.ansell.aterms compared to the original aterm library if any.

Other than that if we can get the assembly setup sorted out I think we can merge this branch to master. I looked at the assembly descriptor briefly and realized we need to exclude the profiler. It is only used during the development process and should not be distributed. We also need to include couple additional things (executable scripts, examples, etc.) in the distribution but that's it.

On Mon, May 12, 2014 at 4:59 PM, Peter Ansell notifications@github.comwrote:

Evren said that he didn't think the issue was in the aterm library so I didn't follow on with the release. I don't want to publish the combined jar file to central, as it doesn't fit with the way the original software was published and the way the authors structure their source repositories.

If Evren thinks that trying 1.6 is worthwhile I will put it back on my todo list.

— Reply to this email directly or view it on GitHubhttps://github.com/clarkparsia/pellet/issues/1#issuecomment-42887390 .

ansell commented 10 years ago

My patches are minimal, mostly adding the pom.xml files:

https://github.com/ansell/aterms/compare/ansellpatches

https://github.com/ansell/jjtraveler/compare/ansellpatches

The biggest difference is actually the base version that is used, as aterms-java was last released 2 years ago at version 1.8.2 where Pellet was previously using 1.6 from 10 years ago. It is not easy to compare all the way back to 1.6, mostly due to the way they used subtree merge to pull repositories (except jjtraveler for some reason) into a single Git repository.

If you are interested in doing forensics the following is the 1.6 release commit:

https://github.com/cwi-swat/aterms/commit/e52ab10f010df9bacd5c32ca38932906a4d10745

tobias-hammerschmidt commented 10 years ago

Since master is now converted to maven can this issue be closed? Maybe a follow-up issue should be created to investigate the failing MechanicalEngineering test caused by the aterm update. Another question is release management - now that pellet is mavenized will clarkparsia also take care of uploading releases to sonatype oss repo? In addition with the recent changes in master (classification, rete) wouldn't a new release (for instance 2.3.2) be justified?

evren commented 10 years ago

Yes, this issue can be closed (I might create a new issue for assembly improvements). But I want to migrate the old Pellet tickets we had in a Trac installation before creating any new issues. You are right that we have enough changes for a new release and we'll be wrapping up that soon after some more testing. We are looking into deployment of maven releases as well.

On Mon, Jun 2, 2014 at 2:45 PM, Tobias Hammerschmidt < notifications@github.com> wrote:

Since master is now converted to maven can this issue be closed? Maybe a follow-up issue should be created to investigate the failing MechanicalEngineering test caused by the aterm update. Another question is release management - now that pellet is mavenized will clarkparsia also take care of uploading releases to sonatype oss repo? In addition with the recent changes in master (classification, rete) wouldn't a new release (for instance 2.3.2) be justified?

— Reply to this email directly or view it on GitHub https://github.com/clarkparsia/pellet/issues/1#issuecomment-44875523.