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

[Guillaume Smet]

On Tue, Aug 13, 2013 at 10:58 AM, Sanne Grinovero <sanne at hibernate.org> wrote:
> On 13 August 2013 09:12, Guillaume Smet <guillaume.smet at gmail.com> 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