Solr
  1. Solr
  2. SOLR-3918

Change the way -excl-slf4j targets work

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 3.6.1, 4.0-BETA
    • Fix Version/s: 4.1, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      If you want to create an entire dist target but leave out slf4j bindings, you must currently use this:

      ant dist-solrj, dist-core, dist-test-framework, dist-contrib dist-war-excl-slf4j

      It would be better to have a single target. Attaching a patch against branch_4x for this.

      1. SOLR-3918.patch
        2 kB
        Hoss Man
      2. SOLR-3918.patch
        1 kB
        Shawn Heisey
      3. SOLR-3918.patch
        0.9 kB
        Shawn Heisey

        Issue Links

          Activity

          Hide
          Shawn Heisey added a comment -

          Time for a judgement call. I believe that all slfj4j jars, even slf4j-api, should be removed from the war. Currently slf4j-api is left in.

          The wiki needs to be updated with instructions on how to use the -excl-slf4j war to run jetty with an alternate binding, such as log4j. As I have just figured out how to do this, I can handle the first draft of the wiki additions.

          Show
          Shawn Heisey added a comment - Time for a judgement call. I believe that all slfj4j jars, even slf4j-api, should be removed from the war. Currently slf4j-api is left in. The wiki needs to be updated with instructions on how to use the -excl-slf4j war to run jetty with an alternate binding, such as log4j. As I have just figured out how to do this, I can handle the first draft of the wiki additions.
          Hide
          Shawn Heisey added a comment -

          An updated patch that also changes the exclusion list to remove all slf4j jars.

          Show
          Shawn Heisey added a comment - An updated patch that also changes the exclusion list to remove all slf4j jars.
          Hide
          Shawn Heisey added a comment -

          I've changed the issue title, because the latest version of the patch changes how dist-war-excl-slf4j works, in addition to creating a new dist-excl-slf4j target. The current build.xml leaves slf4j-api in the war, forcing you to stick with that specific slf4j version. The patch removes all slf4j jars from the war.

          With my patch, someone who wants to change the slf4j binding can go to slf4j.org and download the newest version. By putting the appropriate jars into the proper location (lib/ext for the included jetty8), they can use the -excl-slf4j war and have everything work. The required jars are slf4j-api, jcl-over-slf4j, log4j-over-slf4j, the required binding jar. In the case of log4j, you have to include the log4j jar itself as well.

          Show
          Shawn Heisey added a comment - I've changed the issue title, because the latest version of the patch changes how dist-war-excl-slf4j works, in addition to creating a new dist-excl-slf4j target. The current build.xml leaves slf4j-api in the war, forcing you to stick with that specific slf4j version. The patch removes all slf4j jars from the war. With my patch, someone who wants to change the slf4j binding can go to slf4j.org and download the newest version. By putting the appropriate jars into the proper location (lib/ext for the included jetty8), they can use the -excl-slf4j war and have everything work. The required jars are slf4j-api, jcl-over-slf4j, log4j-over-slf4j, the required binding jar. In the case of log4j, you have to include the log4j jar itself as well.
          Hide
          Shawn Heisey added a comment -

          Discussion on SOLR-4129 touched on this issue. I think my patch for this issue needs to be committed. My patch does two things: 1) adds a new target so you can exclude slf4j but still get everything else that "dist" gives you now. 2) Removes the sl4j api jar from the war in addition to the other slf4j jars. Restating my reasoning for number 2:

          When you choose to exclude slf4j and use your own binding, you're already going to have to go track down jars from an external source and add them to your install. The path of least resistance will lead you to download a newer version of slf4j (1.7.2 right now) than is found in Solr (1.6.4 right now). Because of the api jar sitting in the war, this won't work. If a bug or performance problem is found that affects Solr, it's a fair amount of manual work to get operational with a new slf4j version. My patch eliminates that problem, with the additional requirement that you include the api jar yourself.

          Show
          Shawn Heisey added a comment - Discussion on SOLR-4129 touched on this issue. I think my patch for this issue needs to be committed. My patch does two things: 1) adds a new target so you can exclude slf4j but still get everything else that "dist" gives you now. 2) Removes the sl4j api jar from the war in addition to the other slf4j jars. Restating my reasoning for number 2: When you choose to exclude slf4j and use your own binding, you're already going to have to go track down jars from an external source and add them to your install. The path of least resistance will lead you to download a newer version of slf4j (1.7.2 right now) than is found in Solr (1.6.4 right now). Because of the api jar sitting in the war, this won't work. If a bug or performance problem is found that affects Solr, it's a fair amount of manual work to get operational with a new slf4j version. My patch eliminates that problem, with the additional requirement that you include the api jar yourself.
          Hide
          Hoss Man added a comment -

          Context...

          http://mail-archives.apache.org/mod_mbox/lucene-solr-user/201210.mbox/%3C507937C9.50105@elyograg.org%3E

          I was hoping Jan would chime in on this patch since he did the original work on SOLR-2487, but he did chime in on the mail that spawned this issue suggesting that he thought the approach made sense...

          http://mail-archives.apache.org/mod_mbox/lucene-solr-user/201210.mbox/%3C9F528A23-9A3F-4E4E-9D4F-E1B817A96750@cominvent.com%3E

          Show
          Hoss Man added a comment - Context... http://mail-archives.apache.org/mod_mbox/lucene-solr-user/201210.mbox/%3C507937C9.50105@elyograg.org%3E I was hoping Jan would chime in on this patch since he did the original work on SOLR-2487 , but he did chime in on the mail that spawned this issue suggesting that he thought the approach made sense... http://mail-archives.apache.org/mod_mbox/lucene-solr-user/201210.mbox/%3C9F528A23-9A3F-4E4E-9D4F-E1B817A96750@cominvent.com%3E
          Hide
          Hoss Man added a comment -

          I don't personally see the need for dist-excl-slf4j (is it so terrible to use "ant dist dist-war-excl-slf4j" and ignore the regular war?) but i don't object to it.

          I do however object to having "dist-common" be an advertised target – if it's not something we want people to ever run from the command line, it shouldn't show up in "ant -p" (we're trying to make that list smaller, not bigger)

          The attached patch fixes this, as well as fixes the other descriptions to be clear that now it excludes everything rleated to SLF4J.

          Shawn: if this looks good to you, and if you edit https://wiki.apache.org/solr/SolrLogging with instructions on how to use it, i'll happily commit it.

          (and if i forget, or don't notice you added the instructions to the wiki: send me an email every hour until i do)

          Show
          Hoss Man added a comment - I don't personally see the need for dist-excl-slf4j (is it so terrible to use "ant dist dist-war-excl-slf4j" and ignore the regular war?) but i don't object to it. I do however object to having "dist-common" be an advertised target – if it's not something we want people to ever run from the command line, it shouldn't show up in "ant -p" (we're trying to make that list smaller, not bigger) The attached patch fixes this, as well as fixes the other descriptions to be clear that now it excludes everything rleated to SLF4J. Shawn: if this looks good to you, and if you edit https://wiki.apache.org/solr/SolrLogging with instructions on how to use it, i'll happily commit it. (and if i forget, or don't notice you added the instructions to the wiki: send me an email every hour until i do)
          Hide
          Shawn Heisey added a comment -

          Shawn: if this looks good to you, and if you edit https://wiki.apache.org/solr/SolrLogging with instructions on how to use it, i'll happily commit it.

          I have made a start on the wiki mods. It's not very good yet. I'll take another look at it tomorrow.

          Show
          Shawn Heisey added a comment - Shawn: if this looks good to you, and if you edit https://wiki.apache.org/solr/SolrLogging with instructions on how to use it, i'll happily commit it. I have made a start on the wiki mods. It's not very good yet. I'll take another look at it tomorrow.
          Hide
          Commit Tag Bot added a comment -

          [trunk commit] Chris M. Hostetter
          http://svn.apache.org/viewvc?view=revision&revision=1421411

          SOLR-3918: Fixed the 'dist-war-excl-slf4j' ant target to exclude all slf4j jars, so that the resulting war is usable as is provided the servlet container includes the correct slf4j api and impl jars.

          Show
          Commit Tag Bot added a comment - [trunk commit] Chris M. Hostetter http://svn.apache.org/viewvc?view=revision&revision=1421411 SOLR-3918 : Fixed the 'dist-war-excl-slf4j' ant target to exclude all slf4j jars, so that the resulting war is usable as is provided the servlet container includes the correct slf4j api and impl jars.
          Hide
          Hoss Man added a comment -

          It's not very good yet. I'll take another look at it tomorrow.

          don't knock it – it's still better then the docs we had about excl-slf4j before.

          Committed revision 1421411.
          Committed revision 1421412.

          Show
          Hoss Man added a comment - It's not very good yet. I'll take another look at it tomorrow. don't knock it – it's still better then the docs we had about excl-slf4j before. Committed revision 1421411. Committed revision 1421412.
          Hide
          Commit Tag Bot added a comment -

          [branch_4x commit] Chris M. Hostetter
          http://svn.apache.org/viewvc?view=revision&revision=1421412

          SOLR-3918: Fixed the 'dist-war-excl-slf4j' ant target to exclude all slf4j jars, so that the resulting war is usable as is provided the servlet container includes the correct slf4j api and impl jars (merge r1421411)

          Show
          Commit Tag Bot added a comment - [branch_4x commit] Chris M. Hostetter http://svn.apache.org/viewvc?view=revision&revision=1421412 SOLR-3918 : Fixed the 'dist-war-excl-slf4j' ant target to exclude all slf4j jars, so that the resulting war is usable as is provided the servlet container includes the correct slf4j api and impl jars (merge r1421411)

            People

            • Assignee:
              Shawn Heisey
              Reporter:
              Shawn Heisey
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development