Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 4.0-ALPHA
    • Fix Version/s: 3.5, 4.0-ALPHA
    • Component/s: Build
    • Labels:
      None

      Description

      I was looking at my maven based project's Solr-core dependencies (trunk), and observed some issues that I think should be fixed in Solr's maven poms. I ran mvn dependency:tree – the output is further below. There are two changes I see needed, related to logging:

      • slf4j-jdk14 should be runtime scope, and optional.
      • httpclient depends on commons-logging. Exclude this dependency from the httpclient dependency, and add a dependency on jcl-over-slf4j with compile scope.
      • Zookeeper depends on Log4j, unfortunately. There is an issue to change this to SLF4J: ZOOKEEPER-850. In the mean time we should exclude it and use log4j-over-slf4j with compile scope, at the solrj pom.

      As an aside, it's unfortunate to see all those velocity dependencies. It even depends on struts – seriously?! I hope solritas gets put back into a contrib sometime: SOLR-2588

      Steve, if you'd like to me to create the patch, I will.

      [INFO] +- org.apache.solr:solr-core:jar:4.0-SNAPSHOT:compile
      [INFO] |  +- org.apache.solr:solr-solrj:jar:4.0-SNAPSHOT:compile
      [INFO] |  |  \- org.apache.zookeeper:zookeeper:jar:3.3.3:compile
      [INFO] |  |     +- log4j:log4j:jar:1.2.15:compile
      [INFO] |  |     |  \- javax.mail:mail:jar:1.4:compile
      [INFO] |  |     |     \- javax.activation:activation:jar:1.1:compile
      [INFO] |  |     \- jline:jline:jar:0.9.94:compile
      [INFO] |  +- org.apache.solr:solr-noggit:jar:4.0-SNAPSHOT:compile
      [INFO] |  +- org.apache.lucene:lucene-analyzers-phonetic:jar:4.0-SNAPSHOT:compile
      [INFO] |  +- org.apache.lucene:lucene-highlighter:jar:4.0-SNAPSHOT:compile
      [INFO] |  +- org.apache.lucene:lucene-memory:jar:4.0-SNAPSHOT:compile
      [INFO] |  +- org.apache.lucene:lucene-misc:jar:4.0-SNAPSHOT:compile
      [INFO] |  +- org.apache.lucene:lucene-queryparser:jar:4.0-SNAPSHOT:compile
      [INFO] |  |  \- org.apache.lucene:lucene-sandbox:jar:4.0-SNAPSHOT:compile
      [INFO] |  |     \- jakarta-regexp:jakarta-regexp:jar:1.4:compile
      [INFO] |  +- org.apache.lucene:lucene-spatial:jar:4.0-SNAPSHOT:compile
      [INFO] |  +- org.apache.lucene:lucene-suggest:jar:4.0-SNAPSHOT:compile
      [INFO] |  +- org.apache.lucene:lucene-grouping:jar:4.0-SNAPSHOT:compile
      [INFO] |  +- org.apache.solr:solr-commons-csv:jar:4.0-SNAPSHOT:compile
      [INFO] |  +- commons-codec:commons-codec:jar:1.4:compile
      [INFO] |  +- commons-fileupload:commons-fileupload:jar:1.2.1:compile
      [INFO] |  +- commons-httpclient:commons-httpclient:jar:3.1:compile
      [INFO] |  |  \- commons-logging:commons-logging:jar:1.0.4:compile
      [INFO] |  +- commons-io:commons-io:jar:1.4:compile
      [INFO] |  +- org.apache.velocity:velocity:jar:1.6.4:compile
      [INFO] |  |  +- commons-collections:commons-collections:jar:3.2.1:compile
      [INFO] |  |  \- oro:oro:jar:2.0.8:compile
      [INFO] |  +- org.apache.velocity:velocity-tools:jar:2.0:compile
      [INFO] |  |  +- commons-beanutils:commons-beanutils:jar:1.7.0:compile
      [INFO] |  |  +- commons-digester:commons-digester:jar:1.8:compile
      [INFO] |  |  +- commons-chain:commons-chain:jar:1.1:compile
      [INFO] |  |  +- commons-validator:commons-validator:jar:1.3.1:compile
      [INFO] |  |  +- dom4j:dom4j:jar:1.1:compile
      [INFO] |  |  +- sslext:sslext:jar:1.2-0:compile
      [INFO] |  |  +- org.apache.struts:struts-core:jar:1.3.8:compile
      [INFO] |  |  |  \- antlr:antlr:jar:2.7.2:compile
      [INFO] |  |  +- org.apache.struts:struts-taglib:jar:1.3.8:compile
      [INFO] |  |  \- org.apache.struts:struts-tiles:jar:1.3.8:compile
      [INFO] |  +- org.slf4j:slf4j-jdk14:jar:1.6.1:compile
      [INFO] |  \- org.codehaus.woodstox:wstx-asl:jar:3.2.7:runtime
      
      1. SOLR-2849_maven_dependencies.patch
        44 kB
        Steve Rowe
      2. SOLR-2849_maven_dependencies.patch
        24 kB
        David Smiley
      3. SOLR-2849_maven_dependencies.patch
        25 kB
        David Smiley
      4. SOLR-2849_maven_dependencies.patch
        30 kB
        David Smiley
      5. SOLR-2849_maven_dependencies.patch
        9 kB
        David Smiley
      6. SOLR-2849_maven_dependencies.patch
        9 kB
        David Smiley

        Activity

        Hide
        Jason Rutherglen added a comment -

        As an aside, it's unfortunate to see all those velocity dependencies. It even depends on struts – seriously?! I hope solritas gets put back into a contrib sometime: SOLR-2588

        +1, move it out!

        Show
        Jason Rutherglen added a comment - As an aside, it's unfortunate to see all those velocity dependencies. It even depends on struts – seriously?! I hope solritas gets put back into a contrib sometime: SOLR-2588 +1, move it out!
        Hide
        Steve Rowe added a comment -

        Steve, if you'd like to me to create the patch, I will.

        Sure, please do.

        Show
        Steve Rowe added a comment - Steve, if you'd like to me to create the patch, I will. Sure, please do.
        Hide
        Erik Hatcher added a comment -

        As an aside, it's unfortunate to see all those velocity dependencies. It even depends on struts – seriously?! I hope solritas gets put back into a contrib sometime: SOLR-2588

        I hear ya loud and clear. I'll aim to move it out over the next week or so. There's some test hiccup in moving it out, IIRC, let me dust it off and get it relocated.

        As far as the Struts dependency, that's just some transitive POM listing, not some run (or compile)-time dependency. We certainly don't ship any Struts JARs from Solr and it all works fine.

        Show
        Erik Hatcher added a comment - As an aside, it's unfortunate to see all those velocity dependencies. It even depends on struts – seriously?! I hope solritas gets put back into a contrib sometime: SOLR-2588 I hear ya loud and clear. I'll aim to move it out over the next week or so. There's some test hiccup in moving it out, IIRC, let me dust it off and get it relocated. As far as the Struts dependency, that's just some transitive POM listing, not some run (or compile)-time dependency. We certainly don't ship any Struts JARs from Solr and it all works fine.
        Hide
        David Smiley added a comment -

        Attached is a patch making a fair amount of changes to dependencies in the maven poms. One of the biggest changes to the poms was shifting dependency exclusions at the solr war level down to the origins of where those dependencies were introduced. Below is a summary of the changes I made, including notes on changes I did not make.

        • /modules/queryparser
          • make lucene-sandbox an optional dependency. The dependency is isolated to the XML builder CorePlusExtensionsParser subclass, so it is quite optional.
        • /solr/core
          • added direct dependency on lucene-core, instead of leaving it to transitive resolution (a matter of taste)
          • exclude commons-logging from commons-httpclient dependency
          • depend on jcl-over-sfl4j
          • exclude many unused dependencies from velocity
          • remove slf4j-jdk14 (moved to war pom)
          • I noticed the ant build depends on guava, but we still don't actually use it, so I opted to not add this dependency.
        • /solr/solrj
          • exclude log4j and jline from zookeeper
          • depend on log4j-over-slf4j
          • exclude commons-logging from commons-httpclient dependency
          • depend on jcl-over-sfl4j
          • I noticed the ant distribution suggests woodstox is a solrj dependency. I opted to not include it here. The client doesn't actually depend on it, and SolrJ is usually going to use the efficient binary format any way.
        • /solr/webapp
          • removed numerous exclusions from the solr-core dependency, since these were effectively moved
          • depend on slf4j-jdk scope=runtime optional=true (actually moved it from solr-core)

        I reviewed the built war for consistency with the ant build, as well as the solrj dependencies.

        Show
        David Smiley added a comment - Attached is a patch making a fair amount of changes to dependencies in the maven poms. One of the biggest changes to the poms was shifting dependency exclusions at the solr war level down to the origins of where those dependencies were introduced. Below is a summary of the changes I made, including notes on changes I did not make. /modules/queryparser make lucene-sandbox an optional dependency. The dependency is isolated to the XML builder CorePlusExtensionsParser subclass, so it is quite optional. /solr/core added direct dependency on lucene-core, instead of leaving it to transitive resolution (a matter of taste) exclude commons-logging from commons-httpclient dependency depend on jcl-over-sfl4j exclude many unused dependencies from velocity remove slf4j-jdk14 (moved to war pom) I noticed the ant build depends on guava, but we still don't actually use it, so I opted to not add this dependency. /solr/solrj exclude log4j and jline from zookeeper depend on log4j-over-slf4j exclude commons-logging from commons-httpclient dependency depend on jcl-over-sfl4j I noticed the ant distribution suggests woodstox is a solrj dependency. I opted to not include it here. The client doesn't actually depend on it, and SolrJ is usually going to use the efficient binary format any way. /solr/webapp removed numerous exclusions from the solr-core dependency, since these were effectively moved depend on slf4j-jdk scope=runtime optional=true (actually moved it from solr-core) I reviewed the built war for consistency with the ant build, as well as the solrj dependencies.
        Hide
        David Smiley added a comment -

        I updated the patch to account for another problem I noticed. It turns out that a provided scoped dependency is not transitive. solr-core has a provided dependency on the servlet api. As a consequence, dependencies on solr-core (such as solr-test-framework) didn't pick up this dependency, and so my project using solr-test-framework otherwise had to add this explicit dependency. So I made it this dependency compile scope in solr-core, and then in the webapp I added the dependency to specify provided, so it doesn't end up in the war.

        Show
        David Smiley added a comment - I updated the patch to account for another problem I noticed. It turns out that a provided scoped dependency is not transitive. solr-core has a provided dependency on the servlet api. As a consequence, dependencies on solr-core (such as solr-test-framework) didn't pick up this dependency, and so my project using solr-test-framework otherwise had to add this explicit dependency. So I made it this dependency compile scope in solr-core, and then in the webapp I added the dependency to specify provided, so it doesn't end up in the war.
        Hide
        Steve Rowe added a comment -

        I noticed the ant distribution suggests woodstox is a solrj dependency. I opted to not include it here. The client doesn't actually depend on it, and SolrJ is usually going to use the efficient binary format any way.

        While it's true that Solr's dist target doesn't insist that woodstox is a solrj dependency (hence your "suggests"), there is no mechanism for insistence; all Solr modules' dependencies under the Ant build are "suggestions".

        David, as you may recall, we've previously had this discussion on SOLR-2756 and on #lucene-dev IRC.

        This February 2010 Apache CXF email message from Daniel Kulp that Yonik linked to on SOLR-2042 provides decent rationale for keeping woodstox as a runtime dependency: it's a lot faster than the JVM implementation. The CXF project's Common Utilities module still has this dependency (see the parent POM for the definition of the stax implementation groupId and artifactId).

        As I said in the previous version of this argument, getting rid of the woodstox dependency should be done project wide, if it is to happen, not just in the Maven build. My position is essentially that the Maven build should track the Ant build as closely as possible.

        As I'm sure you're aware, all you have to do to not use the woodstox dependency when you depend on solrj is add an exclusion. This is no great hardship.

        Show
        Steve Rowe added a comment - I noticed the ant distribution suggests woodstox is a solrj dependency. I opted to not include it here. The client doesn't actually depend on it, and SolrJ is usually going to use the efficient binary format any way. While it's true that Solr's dist target doesn't insist that woodstox is a solrj dependency (hence your "suggests"), there is no mechanism for insistence; all Solr modules' dependencies under the Ant build are "suggestions". David, as you may recall, we've previously had this discussion on SOLR-2756 and on #lucene-dev IRC . This February 2010 Apache CXF email message from Daniel Kulp that Yonik linked to on SOLR-2042 provides decent rationale for keeping woodstox as a runtime dependency: it's a lot faster than the JVM implementation. The CXF project's Common Utilities module still has this dependency (see the parent POM for the definition of the stax implementation groupId and artifactId). As I said in the previous version of this argument, getting rid of the woodstox dependency should be done project wide, if it is to happen, not just in the Maven build. My position is essentially that the Maven build should track the Ant build as closely as possible. As I'm sure you're aware, all you have to do to not use the woodstox dependency when you depend on solrj is add an exclusion. This is no great hardship.
        Hide
        David Smiley added a comment -

        Ok. FWIW, I didn't introduce the woodstox or guava discrepancies in the build between maven and ant; I just opted to let them be. I completely understand that it is better to be consistent. I will update the patch to fix these two discrepancies.

        Show
        David Smiley added a comment - Ok. FWIW, I didn't introduce the woodstox or guava discrepancies in the build between maven and ant; I just opted to let them be. I completely understand that it is better to be consistent. I will update the patch to fix these two discrepancies.
        Hide
        Steve Rowe added a comment -

        I didn't introduce the woodstox or guava discrepancies in the build between maven and ant; I just opted to let them be.

        Wow, I have been totally missing the fact that solrj's POM has never had a woodstox dependency.

        I will update the patch to fix these two discrepancies.

        Thanks.

        Show
        Steve Rowe added a comment - I didn't introduce the woodstox or guava discrepancies in the build between maven and ant; I just opted to let them be. Wow, I have been totally missing the fact that solrj's POM has never had a woodstox dependency. I will update the patch to fix these two discrepancies. Thanks.
        Hide
        Erik Hatcher added a comment -

        SOLR-2588 - Velocity moved back to contrib/velocity has been done on trunk. Is there a strong desire to do so on 3.x as well? (no motivation from me on that)

        Show
        Erik Hatcher added a comment - SOLR-2588 - Velocity moved back to contrib/velocity has been done on trunk. Is there a strong desire to do so on 3.x as well? (no motivation from me on that)
        Hide
        Steve Rowe added a comment -

        I'll handle the backport to branch_3x.

        Show
        Steve Rowe added a comment - I'll handle the backport to branch_3x.
        Hide
        Steve Rowe added a comment -

        The Velocity contrib retrograde advancement (SOLR-2588) has been backported to branch_3x.

        Show
        Steve Rowe added a comment - The Velocity contrib retrograde advancement ( SOLR-2588 ) has been backported to branch_3x.
        Hide
        David Smiley added a comment -

        I updated the patch with more extensive changes. These changes assume the latest trunk in which Velocity was moved back to a contrib.

        In order to make the dependencies more consistent with the ant build, I added woodstox to solrj, and guava to solr-core.

        One new problem that popped up since velocity moved is the fact that the ant build erroneously builds a WAR with commons-beanutils and commons-collections in it. These were velocity dependencies that aren't used by Solr otherwise. They should be removed from the ant build. I didn't add them to maven because I think the ant side is in error.

        Beyond that I found numerous places to consolidate and simplify certain dependencies:

        solr-parent

        solr-parent is the parent pom of all of Solr. It had no dependencies but this is a prime place to put several, which I did:

        • slf4j-api
        • lucene-test-framework (test)
        • junit (test)
        • slf4j-jdk14 (test)

        I was then able to remove various declarations of these in various other poms, since it became redundant.

        solr-contrib-aggregator

        solr-contrib-aggregator is a pom referring to all of Solr's contrib modules. However strangely, each of the contrib modules references solr-parent as their parent pom, not solr-contrib-aggregator. I changed them all to do this so that I could add dependencies common to all contribs:

        • solr-core
        • solr-test-framework (test)

        And then I was able to remove a good number of explicit dependency declarations from contrib poms, including transitive ones (e.g. the servlet api).

        I re-verified the solrj dependencies and solr war contents for accuracy.

        Show
        David Smiley added a comment - I updated the patch with more extensive changes. These changes assume the latest trunk in which Velocity was moved back to a contrib. In order to make the dependencies more consistent with the ant build, I added woodstox to solrj, and guava to solr-core. One new problem that popped up since velocity moved is the fact that the ant build erroneously builds a WAR with commons-beanutils and commons-collections in it. These were velocity dependencies that aren't used by Solr otherwise. They should be removed from the ant build. I didn't add them to maven because I think the ant side is in error. Beyond that I found numerous places to consolidate and simplify certain dependencies: solr-parent solr-parent is the parent pom of all of Solr. It had no dependencies but this is a prime place to put several, which I did: slf4j-api lucene-test-framework (test) junit (test) slf4j-jdk14 (test) I was then able to remove various declarations of these in various other poms, since it became redundant. solr-contrib-aggregator solr-contrib-aggregator is a pom referring to all of Solr's contrib modules. However strangely, each of the contrib modules references solr-parent as their parent pom, not solr-contrib-aggregator. I changed them all to do this so that I could add dependencies common to all contribs: solr-core solr-test-framework (test) And then I was able to remove a good number of explicit dependency declarations from contrib poms, including transitive ones (e.g. the servlet api). I re-verified the solrj dependencies and solr war contents for accuracy.
        Hide
        Steve Rowe added a comment -

        solr-contrib-aggregator is a pom referring to all of Solr's contrib modules. However strangely, each of the contrib modules references solr-parent as their parent pom, not solr-contrib-aggregator.

        As you must already know, aggregation and parent relationships are separate under Maven. There are standard Maven usage patterns that take advantage of this fact, e.g. parent POM as sibling module (see e.g. the Apache CXF project which uses this pattern) - in this pattern, aggregation is separate from parent relationship.

        I did it this way so that we didn't have to publish solr-contrib-aggregator POM (it is not now published), since it seemed like an unnecessary complication. Aggregation is important only for the purposes of the build, and not for purposes of artifact consumption.

        So if we change solr-contrib-aggregator POM to also be parent, then we have to publish it (this happens via 'ant generate-maven-artifacts'). Also, this POM will no longer simply be an aggregator, so it should be renamed to 'solr-contrib', removing the '-aggregator'.

        Show
        Steve Rowe added a comment - solr-contrib-aggregator is a pom referring to all of Solr's contrib modules. However strangely, each of the contrib modules references solr-parent as their parent pom, not solr-contrib-aggregator. As you must already know, aggregation and parent relationships are separate under Maven. There are standard Maven usage patterns that take advantage of this fact, e.g. parent POM as sibling module (see e.g. the Apache CXF project which uses this pattern) - in this pattern, aggregation is separate from parent relationship. I did it this way so that we didn't have to publish solr-contrib-aggregator POM (it is not now published), since it seemed like an unnecessary complication. Aggregation is important only for the purposes of the build, and not for purposes of artifact consumption. So if we change solr-contrib-aggregator POM to also be parent, then we have to publish it (this happens via 'ant generate-maven-artifacts'). Also, this POM will no longer simply be an aggregator, so it should be renamed to 'solr-contrib', removing the '-aggregator'.
        Hide
        David Smiley added a comment -

        Good point that the aggregator pom would end up getting published now. No harm; right?

        Any other comments or is this committable?

        Show
        David Smiley added a comment - Good point that the aggregator pom would end up getting published now. No harm; right? Any other comments or is this committable?
        Hide
        Erik Hatcher added a comment -

        One new problem that popped up since velocity moved is the fact that the ant build erroneously builds a WAR with commons-beanutils and commons-collections in it. These were velocity dependencies that aren't used by Solr otherwise. They should be removed from the ant build.

        Done. These have been moved to contrib/velocity/lib, and do not end up in solr.war.

        Show
        Erik Hatcher added a comment - One new problem that popped up since velocity moved is the fact that the ant build erroneously builds a WAR with commons-beanutils and commons-collections in it. These were velocity dependencies that aren't used by Solr otherwise. They should be removed from the ant build. Done. These have been moved to contrib/velocity/lib, and do not end up in solr.war.
        Hide
        David Smiley added a comment -

        Chris Male, can you please render your opinion on wether or not solr-contrib-aggregator should stay as-is pre-patch or with the patch's approach? Keeping as-is means that each contrib needs to declare solr-core & solr-test-framework as dependencies. The patch moves those dependencies down to a new parent in-between a contrib module and the solr-parent pom which means an extra pom to deploy and potentially understand by an end user. FYI mvn dependency:tree would be the same either way. See Steve's comment.

        Steve is barely +1, I am +1.

        Show
        David Smiley added a comment - Chris Male, can you please render your opinion on wether or not solr-contrib-aggregator should stay as-is pre-patch or with the patch's approach? Keeping as-is means that each contrib needs to declare solr-core & solr-test-framework as dependencies. The patch moves those dependencies down to a new parent in-between a contrib module and the solr-parent pom which means an extra pom to deploy and potentially understand by an end user. FYI mvn dependency:tree would be the same either way. See Steve's comment. Steve is barely +1, I am +1.
        Hide
        Steve Rowe added a comment -

        /modules/queryparser: make lucene-sandbox an optional dependency. The dependency is isolated to the XML builder CorePlusExtensionsParser subclass, so it is quite optional.

        I don't agree about it being "quite" optional. The lucene-sandbox dependency is there for FuzzyLikeThisQuery and DuplicateFilter. Making the lucene-sandbox dependency optional means Maven users will have to know that their own project has to depend on lucene-sandbox-X.X.jar if they want to use the builders for any of queries supported by CorePlusExtensionsParser (since a ClassNotFoundException will be thrown when the CorePlusExtensionsParser ctor is invoked):

        1. TermsFilter
        2. BooleanFilter
        3. DuplicateFilter
        4. MoreLikeThisQuery
        5. BoostingQuery
        6. FuzzyLikeThisQuery

        I think your motivation for making the dependency optional is that most people won't use any of these? But I don't think this is an appropriate distinction to make for a library. The queryparser library exposes certain functionality, and we should indicate the dependencies required to enable all of that functionality, not just the subset we think most people will use.

        I think we should only declare dependencies optional when their exclusion will not limit usage. Right now on trunk, only two deps in one POM (solr core) are declared optional, and the reason is that there really are no transitive dependencies on them.

        Show
        Steve Rowe added a comment - /modules/queryparser: make lucene-sandbox an optional dependency. The dependency is isolated to the XML builder CorePlusExtensionsParser subclass, so it is quite optional. I don't agree about it being "quite" optional. The lucene-sandbox dependency is there for FuzzyLikeThisQuery and DuplicateFilter . Making the lucene-sandbox dependency optional means Maven users will have to know that their own project has to depend on lucene-sandbox-X.X.jar if they want to use the builders for any of queries supported by CorePlusExtensionsParser (since a ClassNotFoundException will be thrown when the CorePlusExtensionsParser ctor is invoked): TermsFilter BooleanFilter DuplicateFilter MoreLikeThisQuery BoostingQuery FuzzyLikeThisQuery I think your motivation for making the dependency optional is that most people won't use any of these? But I don't think this is an appropriate distinction to make for a library. The queryparser library exposes certain functionality, and we should indicate the dependencies required to enable all of that functionality, not just the subset we think most people will use. I think we should only declare dependencies optional when their exclusion will not limit usage. Right now on trunk, only two deps in one POM (solr core) are declared optional, and the reason is that there really are no transitive dependencies on them.
        Hide
        Steve Rowe added a comment -

        David, I have a question about your use of provided scope.

        In your patch for the solr core POM, you remove a provided scope usage, saying that solr-core is a jar not a war, but then you leave in a provided scope usage and add a comment about its function; your message is decidedly mixed. Here are the patch excerpts I'm talking about:

        Index: dev-tools/maven/solr/core/pom.xml.template
        ===================================================================
        --- dev-tools/maven/solr/core/pom.xml.template  (revision 1189433)
        +++ dev-tools/maven/solr/core/pom.xml.template  (revision )
        @@ -138,17 +151,9 @@
             <dependency>
               <groupId>org.mortbay.jetty</groupId>
               <artifactId>jsp-2.1-jetty</artifactId>
        -      <scope>provided</scope>
        +      <scope>provided</scope><!-- FYI provided is non-transitive -->
             </dependency>
        @@ -162,14 +167,9 @@
             <dependency>
               <groupId>javax.servlet</groupId>
               <artifactId>servlet-api</artifactId>
        -      <scope>provided</scope>
        +      <!-- compile scope; solr-core is a jar not a war -->
             </dependency>
        

        Shouldn't the same rationale apply to both of these? If so, we need to pick one and be consistent, right?

        Show
        Steve Rowe added a comment - David, I have a question about your use of provided scope. In your patch for the solr core POM, you remove a provided scope usage, saying that solr-core is a jar not a war, but then you leave in a provided scope usage and add a comment about its function; your message is decidedly mixed. Here are the patch excerpts I'm talking about: Index: dev-tools/maven/solr/core/pom.xml.template =================================================================== --- dev-tools/maven/solr/core/pom.xml.template (revision 1189433) +++ dev-tools/maven/solr/core/pom.xml.template (revision ) @@ -138,17 +151,9 @@ <dependency> <groupId> org.mortbay.jetty </groupId> <artifactId> jsp-2.1-jetty </artifactId> - <scope> provided </scope> + <scope> provided </scope> <!-- FYI provided is non-transitive --> </dependency> @@ -162,14 +167,9 @@ <dependency> <groupId> javax.servlet </groupId> <artifactId> servlet-api </artifactId> - <scope> provided </scope> + <!-- compile scope; solr-core is a jar not a war --> </dependency> Shouldn't the same rationale apply to both of these? If so, we need to pick one and be consistent, right?
        Hide
        Steve Rowe added a comment - - edited

        Any other comments or is this committable?

        All tests pass under Maven for me with your patch.

        I don't have any other comments, so I'm +1 to commit, assuming we come to a resolution on the three open issues:

        1. moving shared solr contribs' dependency declarations to a renamed solr-contrib-aggregator POM, and publishing this POM
        2. making lucene-sandbox an optional dependency in the queryparser module
        3. use of provided scope in the solr core POM).
        Show
        Steve Rowe added a comment - - edited Any other comments or is this committable? All tests pass under Maven for me with your patch. I don't have any other comments, so I'm +1 to commit, assuming we come to a resolution on the three open issues: moving shared solr contribs' dependency declarations to a renamed solr-contrib-aggregator POM, and publishing this POM making lucene-sandbox an optional dependency in the queryparser module use of provided scope in the solr core POM).
        Hide
        David Smiley added a comment -

        Regarding lucene-sandbox; I tend to have a more minimalistic perspective but I don't feel strongly about lucene-sandbox being optional so I will put it back to a full-fledged dependency, and then add an exclusion at the Solr level, in solr-core.

        Regarding the servlet-api: Solr-core genuinely has a dependency on the servlet-api and someone using solr-core had better have it on its class path. Provided scope is incorrect because it is non-transitive (something new I learned). It needs to be a normal compile scope at solr-core so that if some project, (mine) uses solr-core then it gets pulled in. In order for it to not wind up in the web app, which is sort of a special rule about the servlet-api in particular, it is redeclared at that pom with provided scope.

        Regarding jetty-jsp: I find the jetty-jsp dependency curious as I'm not sure exactly why it is depended on; please enlighten me. I chose to add a comment to its existing declaration rather than change it since I'm not sure what's going on there. There are two other jetty dependencies; should they all be depended upon in the same way?

        Show
        David Smiley added a comment - Regarding lucene-sandbox; I tend to have a more minimalistic perspective but I don't feel strongly about lucene-sandbox being optional so I will put it back to a full-fledged dependency, and then add an exclusion at the Solr level, in solr-core. Regarding the servlet-api: Solr-core genuinely has a dependency on the servlet-api and someone using solr-core had better have it on its class path. Provided scope is incorrect because it is non-transitive (something new I learned). It needs to be a normal compile scope at solr-core so that if some project, (mine) uses solr-core then it gets pulled in. In order for it to not wind up in the web app, which is sort of a special rule about the servlet-api in particular, it is redeclared at that pom with provided scope. Regarding jetty-jsp: I find the jetty-jsp dependency curious as I'm not sure exactly why it is depended on; please enlighten me. I chose to add a comment to its existing declaration rather than change it since I'm not sure what's going on there. There are two other jetty dependencies; should they all be depended upon in the same way?
        Hide
        Steve Rowe added a comment -

        I don't feel strongly about lucene-sandbox being optional so I will put it back to a full-fledged dependency, and then add an exclusion at the Solr level, in solr-core.

        +1

        Regarding the servlet-api: Solr-core genuinely has a dependency on the servlet-api and someone using solr-core had better have it on its class path. Provided scope is incorrect because it is non-transitive (something new I learned). It needs to be a normal compile scope at solr-core so that if some project, (mine) uses solr-core then it gets pulled in. In order for it to not wind up in the web app, which is sort of a special rule about the servlet-api in particular, it is redeclared at that pom with provided scope.

        Sounds good.

        Regarding jetty-jsp: I find the jetty-jsp dependency curious as I'm not sure exactly why it is depended on; please enlighten me.

        The Jasper compiler from the jetty-jsp jar is used to validate Solr's JSP files in the Ant build in the Solr webapp module's test target. The Maven build doesn't perform this validation. The dependency can be removed from the solr core POM; when I removed the dependency, all tests passed under Maven.

        There are two other jetty dependencies; should they all be depended upon in the same way?

        I think the other two, which are required for tests to compile, should remain optional, so that they are not transitive.

        Show
        Steve Rowe added a comment - I don't feel strongly about lucene-sandbox being optional so I will put it back to a full-fledged dependency, and then add an exclusion at the Solr level, in solr-core. +1 Regarding the servlet-api: Solr-core genuinely has a dependency on the servlet-api and someone using solr-core had better have it on its class path. Provided scope is incorrect because it is non-transitive (something new I learned). It needs to be a normal compile scope at solr-core so that if some project, (mine) uses solr-core then it gets pulled in. In order for it to not wind up in the web app, which is sort of a special rule about the servlet-api in particular, it is redeclared at that pom with provided scope. Sounds good. Regarding jetty-jsp: I find the jetty-jsp dependency curious as I'm not sure exactly why it is depended on; please enlighten me. The Jasper compiler from the jetty-jsp jar is used to validate Solr's JSP files in the Ant build in the Solr webapp module's test target. The Maven build doesn't perform this validation. The dependency can be removed from the solr core POM; when I removed the dependency, all tests passed under Maven. There are two other jetty dependencies; should they all be depended upon in the same way? I think the other two, which are required for tests to compile, should remain optional , so that they are not transitive.
        Hide
        Chris Male added a comment -

        I didn't see my call back to comment on this issue till now.

        Chris Male, can you please render your opinion on wether or not solr-contrib-aggregator should stay as-is pre-patch or with the patch's approach? Keeping as-is means that each contrib needs to declare solr-core & solr-test-framework as dependencies. The patch moves those dependencies down to a new parent in-between a contrib module and the solr-parent pom which means an extra pom to deploy and potentially understand by an end user. FYI mvn dependency:tree would be the same either way. See Steve's comment.

        I'm really torn I have to say. Normally I'd be all for having a proper contrib pom but I understand Steven's comments that it seems like another complication. With that said, I feel the biggest issue is that we should be consistent. If we have a proper lucene contrib pom, then we should have one for Solr's contribs and for modules too. I'll happily go +1 if you're prepared to do all the leg work.

        Show
        Chris Male added a comment - I didn't see my call back to comment on this issue till now. Chris Male, can you please render your opinion on wether or not solr-contrib-aggregator should stay as-is pre-patch or with the patch's approach? Keeping as-is means that each contrib needs to declare solr-core & solr-test-framework as dependencies. The patch moves those dependencies down to a new parent in-between a contrib module and the solr-parent pom which means an extra pom to deploy and potentially understand by an end user. FYI mvn dependency:tree would be the same either way. See Steve's comment. I'm really torn I have to say. Normally I'd be all for having a proper contrib pom but I understand Steven's comments that it seems like another complication. With that said, I feel the biggest issue is that we should be consistent. If we have a proper lucene contrib pom, then we should have one for Solr's contribs and for modules too. I'll happily go +1 if you're prepared to do all the leg work.
        Hide
        David Smiley added a comment -

        I attached an updated patch. Changes:

        1. After some thought about Steve & Chris's comments, I backed out the intermediate parent contrib pom. So the solr-core and solr-test-framework deps went to back to the solr contribs.
        2. The jetty-jsp dependency in solr-core was changed to test.
        3. I backed-out making lucene-sandbox an optional dependency on lucene-queryparser. In solr-core I made an exclusion.

        Tests pass. It's a good idea to do a "mvn clean" first.

        Show
        David Smiley added a comment - I attached an updated patch. Changes: After some thought about Steve & Chris's comments, I backed out the intermediate parent contrib pom. So the solr-core and solr-test-framework deps went to back to the solr contribs. The jetty-jsp dependency in solr-core was changed to test. I backed-out making lucene-sandbox an optional dependency on lucene-queryparser. In solr-core I made an exclusion. Tests pass. It's a good idea to do a "mvn clean" first.
        Hide
        David Smiley added a comment -

        Attached is an updated patch with minor changes to use $

        {project.groupId}

        in some more places, and to remove some unneeded dependency declarations that were already transitively pulled in.

        Show
        David Smiley added a comment - Attached is an updated patch with minor changes to use $ {project.groupId} in some more places, and to remove some unneeded dependency declarations that were already transitively pulled in.
        Hide
        Steve Rowe added a comment -

        New version of the patch, based on David's most recent patch.

        The changes:

        1. Removed the junit dependency from all Lucene POMs that depend on the lucene-test-framework.
        2. Switched the solr-cell xercesImpl dependency to v2.8.1, the version used in the Ant build.
        3. Removed the DIH-extras xercesImpl dependency, since the Ant build doesn't include xercesImpl on the classpath.
        4. Removed the lucene-specific xercesImpl version from the grandfather POM's dependencyManagement section, for two reasons:
          • No other dependency with groupId org.apache.lucene is listed in the grandfather POM's dependencyManagement section; and
          • Only one module actually depends on this artifact: the benchmark module.

        Committing shortly.

        Show
        Steve Rowe added a comment - New version of the patch, based on David's most recent patch. The changes: Removed the junit dependency from all Lucene POMs that depend on the lucene-test-framework. Switched the solr-cell xercesImpl dependency to v2.8.1, the version used in the Ant build. Removed the DIH-extras xercesImpl dependency, since the Ant build doesn't include xercesImpl on the classpath. Removed the lucene-specific xercesImpl version from the grandfather POM's dependencyManagement section, for two reasons: No other dependency with groupId org.apache.lucene is listed in the grandfather POM's dependencyManagement section; and Only one module actually depends on this artifact: the benchmark module. Committing shortly.
        Hide
        Steve Rowe added a comment -

        Committed to trunk and branch_3x.

        Thanks David!

        Show
        Steve Rowe added a comment - Committed to trunk and branch_3x. Thanks David!
        Hide
        Uwe Schindler added a comment -

        Bulk close after 3.5 is released

        Show
        Uwe Schindler added a comment - Bulk close after 3.5 is released
        Hide
        David Smiley added a comment -

        This is an old issue that was resolved but I want to re-assess one thing:

        The slf4j-log4j12 (formerly slf4j-jdk14) is still not marked as optional. But if you're using maven to depend on solrj, wouldn't you want the consumer to pick the target logger? Maybe they like jdk14, maybe they like log4j, maybe logback. So rather than force them to exclude slf4j-jdk14 (which is annoying), it could be marked as optional.

        Show
        David Smiley added a comment - This is an old issue that was resolved but I want to re-assess one thing: The slf4j-log4j12 (formerly slf4j-jdk14) is still not marked as optional. But if you're using maven to depend on solrj, wouldn't you want the consumer to pick the target logger? Maybe they like jdk14, maybe they like log4j, maybe logback. So rather than force them to exclude slf4j-jdk14 (which is annoying), it could be marked as optional.
        Hide
        Steve Rowe added a comment -

        So rather than force them to exclude slf4j-jdk14 (which is annoying), it could be marked as optional.

        I see ant dist under solr/ still puts the slf4j-log4j12 jar into dist/solrj-lib/. As I've said previously on this issue, I think the Maven config should mirror the Ant config: until we stop distributing the slf4j-log4j12 jar as a solrj dependency, I don't think we should mark it as optional in the Maven config.

        Show
        Steve Rowe added a comment - So rather than force them to exclude slf4j-jdk14 (which is annoying), it could be marked as optional. I see ant dist under solr/ still puts the slf4j-log4j12 jar into dist/solrj-lib/ . As I've said previously on this issue, I think the Maven config should mirror the Ant config: until we stop distributing the slf4j-log4j12 jar as a solrj dependency, I don't think we should mark it as optional in the Maven config.

          People

          • Assignee:
            Steve Rowe
            Reporter:
            David Smiley
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development