Re: [hibernate-dev] HSEARCH - Assorted fixes to the pom files

On Tue, Aug 13, 2013 at 10:58 AM, Sanne Grinovero <sanne(a)hibernate.org&gt; wrote: > On 13 August 2013 09:12, Guillaume Smet <guillaume.smet(a)gmail.com&gt; wrote: >> Hi, >> >> I'm planning to work on the pom files in the next few days. >> >> Here is more or less the plan I have in mind: >> - centralize all dependencies versions in the properties of the parent >> pom file. A couple of them are directly in the dependencyManagement >> block. It's easier to update them this way. > > +0 > It might no longer be the case, but there have been situations in > which we would test - for example - integration with JGroups 2.9x in > search-engine to make sure backwards compatibility would work, but > need to override this to 3.2.x when used in combination with > Infinispan. Basically that serves to verify against a broader range of > versions than what we can do with a single version. > End users use single versions (at least we hope so) but it's often > impossible for them to solve the dependency puzzle if we (and other > frameworks) don't allow compatibility with a somewhat more relaxed > range of versions, that brings to different definitions of "best > practices" depending if you're building a library or an application. You can do whatever you want by overriding the dependency management. It's the only way to really fix the problem of transitive dependencies. If you want a specific version for a child module, just define it in your local dependency management, it's going to override the one of the parent.

> Feel free to reorganize this but we're not going to enforce a hard > rule about it, we occasionally need to be able to achieve some > flexibility and the parent pom strategy in Maven isn't always good > enough. > >> - same for the Maven plugins; > > -1 for properties > I keep them up to date them via "mvn versions:[xyz]" and this doesn't > understand properties, I would need to track them all down manually. > > +1 to move all plugin versions in the <pluginManagement> section if > they aren't already (I see now that one is missing). I don't use the versions plugin but I just did some tests with our parent pom (which is *huge*) and it perfectly understand properties. It even has goals to update them.

My point is it's a little weird to have a mix of properties and hard coded versions. >> - I would like to enforce the dependency convergence rule via the >> enforcer plugin: a few additional dependency management declaration >> will be needed to get everything right. See >> http://maven.apache.org/enforcer/enforcer-rules/dependencyConvergence.html >> for more information. > > +1 OK. >> - remove the commons-logging API altogether and use slf4j and its >> wrapper for commons-logging > > Not sure I understand? We don't use neither commons-logging nor slf4j. > You're welcome to convince all our dependencies to move to JBoss > Logger :-D > I'd rather remove both if that where possible. I don't know jboss logging that well. Does it provide wrappers similar to what slf4j is doing?

My point in removing commons-logging is to have all logging dependencies satisfied with slf4j and its wrappers. commons-logging is a pain: the first time we had to completely remove it was because of problems with Weblogic IIRC. But we can keep it this way if you prefer.

>> - update a few core dependencies (especially Javassist to the version >> used in ORM, slf4j, log4j, commons-io which is a solr dependency and >> Solr is now using 2.1, not 1.4) > > +100 we need to stay in synch, that could be a bug > But pick the versions used by our dependencies, what is used by > "latest" Solr doesn't matter of course. Of course. I was talking about Solr 3.6.2.

> Ideally we should try to guarantee to stay in synch. Am I > understanding correctly that enforcer can do that via dependency > convergence ? Not exactly. What I usually recommend is the following: - define your dependencies versions in your dependencyManagement; - don't define the versions of the transitive dependencies: they will update themselves accordingly; - EXCEPT if you have a conflict: and it's where the dependency convergence rules helps you: it detects the conflicts and breaks the build. But it doesn't detect any conflict if your dependency is managed via dependency management. In the case of commons-io, if it's only coming from Solr, my advice is to remove it from the dependency management and let Solr choose the correct version. If one day, another dependency needs a different version, the enforcer plugin will warn us and we'll have to choose a common version which is working for both of them. From my experience of 5 years of maintaining a Maven based framework, base dependencies are usually very stable and we keep them in synch with upstream without too much pain.

>> - update the activemq dependency if it's not too much of a pain as >> it's only a test dependency; > > +1 very glad if you could do that > >> - remove outdated comments: I'm especially thinking about the one for >> the log4j dependency: the problem has been fixed a while ago; > > Are you sure it's fixed? It creates a hell of problems during the > release process. > Ideally I'd drop it, if only we could. I'm talking about this comment: <!-- Defining log4j here, but due to a bug in Shrinkwrap we still need to define the version explicitly in the sub modules. See HSEARCH-970 --> The issue is fixed so we can remove the comment I think (but not the dependency). >> - in latest JBoss EAP, Jackson is in a custom version of 1.9.9 (there >> is a "redhat" qualifier in the version string): maybe, I was thinking >> about upgrading the dependency to at least 1.9.9, instead of 1.9.2 >> (which is effectively the version used in AS 7.1); > > +1 > Also would be nice to depend on the "jackson module" provided by AS > rather than bringing our copy. Sorry, not a JBoss guy (yet) :). I'm not very aware of this module thing.

>> - remove the lucene-regex dependency management definition: this >> dependency isn't declared anywhere, except in the modules file. I >> think it's a leftover of something and we should get rid of it >> altogether; > > -1 > It's needed in the modules. It contains optional analyzers and helpers > which appear to be popular among users, since we package the module > for AS7 / EAP6 it's meant to be included in there. OK. >> - remove the luceneRegexJakartaVersion property: it's not used anywhere. > > +1 OK. > Thanks for the in-depth analysis! very nice Thanks for the feedback! -- Guillaume