Lucene - Core
  1. Lucene - Core
  2. LUCENE-5257

Lock down centralized versioning of ivy dependencies

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.6, 6.0
    • Component/s: general/build
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      LUCENE-5249 introduced centralized versioning of 3rd party dependencies and converted all ivy.xml files across Lucene/Solr to use this scheme. But there is nothing preventing people from ignoring this setup and (intentionally or not) introducing non-centralized dependency versions.

      SOLR-3664 discusses the problem of out-of-sync 3rd party dependencies between Lucene/Solr modules. Centralized versioning makes synchronization problems less likely but not impossible.

      One fairly simple way to ensure that all modules use the same version of 3rd party deps would be to require that all deps in ivy.xml would have to use the rev="${/org/name}" syntax, via a validation script.

      The problem remains that there may eventually be a requirement to use different 3rd party libs in different modules. Any form of lockdown here should allow for this possibility. Hoss's suggestion from a conversation on #lucene IRC earlier today:

        <hoss> perhaps exceptions could be by naming convetion
      <sarowe> can you give an example?
        <hoss> ie: variables must match either ${group}/${artifact} or they must match
               /VERSION_MISTMATCH_EXCEPTION/${group}/${artifact}
      <sarowe> nice idea
               no external config required
        <hoss> right
               and it has to be real obvious when you are bucking convention
        <hoss> or better yet: ${group}/${artifact}/VERSION_MISTMATCH_EXCEPTION
               ... and there is another check that the version file is in ascii order
               so you are garuntted that it has to be right there in the versions file
               one line after ${group}/${artifact}
      <sarowe> i like it
        <hoss> no change someone updating ${group}/${artifact} won't notice it
               i suppose really it should be
               ${group}/${artifact}/VERSION_MISTMATCH_EXCEPTION/${reason}
               ... since you might have more then one exception per ${group}/${artifact}
               but now i'm just making things up w/o evn really understanding
               the conventions you've alreay put in place
      <sarowe> :)
        <hoss> you get the idea
      <sarowe> yes
      
      1. LUCENE-5257.patch
        27 kB
        Steve Rowe
      2. LUCENE-5257.patch
        27 kB
        Steve Rowe
      3. LUCENE-5257.patch
        19 kB
        Steve Rowe

        Issue Links

          Activity

          Hide
          Robert Muir added a comment -

          The problem remains that there may eventually be a requirement to use different 3rd party libs in different modules. Any form of lockdown here should allow for this possibility.

          Not a chance. -1 to jar hell.

          Show
          Robert Muir added a comment - The problem remains that there may eventually be a requirement to use different 3rd party libs in different modules. Any form of lockdown here should allow for this possibility. Not a chance. -1 to jar hell.
          Hide
          Steve Rowe added a comment -

          Not a chance. -1 to jar hell.

          Cool, makes this issue easier.

          Show
          Steve Rowe added a comment - Not a chance. -1 to jar hell. Cool, makes this issue easier.
          Hide
          Hoss Man added a comment -

          FWIW: the idea that we might down the road want to allow moduleA to depends on third-party-2.0.jar while unrelated moduleB depends on third-party-3.0.jar came from a mental exercise of "if we lock this down with a build failure now, how will we deal with it if we decide we want to allow it in some special case later?"

          but i agree: even if moduleA and moduleB have no dependencies on eachother, and no other moduleC exists that depends on both moduleA and moduleB it's still a bad idea to let moduleA and moduleB depend on diff versions of third-party.jar since some downstream client of lucene/solr might want to use both moduleA and moduleB.

          more importantly: we can lock this down with a build failure now, and if we do ever change out mind, we have an idea here of how to tweak the rules to allow special exceptions in a way that's non-sneaky and really obvious ... but unless someone is clamoring for it, i say we help our users avoid classpath hell just as much as we want to.

          Show
          Hoss Man added a comment - FWIW: the idea that we might down the road want to allow moduleA to depends on third-party-2.0.jar while unrelated moduleB depends on third-party-3.0.jar came from a mental exercise of "if we lock this down with a build failure now, how will we deal with it if we decide we want to allow it in some special case later?" but i agree: even if moduleA and moduleB have no dependencies on eachother, and no other moduleC exists that depends on both moduleA and moduleB it's still a bad idea to let moduleA and moduleB depend on diff versions of third-party.jar since some downstream client of lucene/solr might want to use both moduleA and moduleB. more importantly: we can lock this down with a build failure now, and if we do ever change out mind, we have an idea here of how to tweak the rules to allow special exceptions in a way that's non-sneaky and really obvious ... but unless someone is clamoring for it, i say we help our users avoid classpath hell just as much as we want to.
          Hide
          Robert Muir added a comment -

          Let me clarify what i mean by jar hell:

          if we want to add exceptions because someone jacked up their maven coordinates or some other reason, when in fact its not "jar hell", to me thats different than declaring we will allow exceptions for jar hell.

          For example: some stuff depends on jetty6 (org.mortbay.jetty) and other stuff depends on jetty8 (org.eclipse.jetty). This isn't jar hell, because the packages are totally different in the jar files so they don't conflict. They also renamed the coordinates to org.eclipse.jetty to match.

          But i think we shouldnt allow true jar hell (where the stuff inside the jars conflict). People should be able to pick and choose what stuff they want to use and use it, and we shouldnt ship a release with jar hell.

          I would also like to explore adding a "jar hell detector" in the future to our smoketester, that unzips all jar files and fails if e.g. any .class (maybe even resources outside of META-INF) conflict. I think we should try to improve our build with checks like this, but if we allow for "jar hell exceptions" it makes such a check extremely difficult or impossible.

          Show
          Robert Muir added a comment - Let me clarify what i mean by jar hell: if we want to add exceptions because someone jacked up their maven coordinates or some other reason, when in fact its not "jar hell", to me thats different than declaring we will allow exceptions for jar hell. For example: some stuff depends on jetty6 (org.mortbay.jetty) and other stuff depends on jetty8 (org.eclipse.jetty). This isn't jar hell, because the packages are totally different in the jar files so they don't conflict. They also renamed the coordinates to org.eclipse.jetty to match. But i think we shouldnt allow true jar hell (where the stuff inside the jars conflict). People should be able to pick and choose what stuff they want to use and use it, and we shouldnt ship a release with jar hell. I would also like to explore adding a "jar hell detector" in the future to our smoketester, that unzips all jar files and fails if e.g. any .class (maybe even resources outside of META-INF) conflict. I think we should try to improve our build with checks like this, but if we allow for "jar hell exceptions" it makes such a check extremely difficult or impossible.
          Hide
          Steve Rowe added a comment -

          Patch implementing the idea as a custom Ant task: LibVersionCheckTask.

          I had originally planned a scripted implementation, but I figured that could fail for valid ivy-version.properties syntax, or valid ivy.xml syntax, since a script would likely make assumptions about the format, so I went with full parsing for both, though I had to write the Properties file parser, since java.util.Properties doesn't provide file-order access to the parsed contents.

          The new task checks the /org/name keys in the centralized versions file for lexical sort order and for duplicates, then verifies that the rev attributes for all <dependency>-s in all ivy.xml files in the current directory (lucene/ and solr/) and below are formatted in ${/org/name} format, where org matches the value of the "org" attribute, and name matches the value of the "name" attribute.

          I modeled it on (and stole a lot of it from) the custom LicenseCheckTask.

          The patch includes a fix to the ivy-versions.properties file that the new task found: there are currently duplicate property keys for two of the httpcomponents jars.

          Like LicenseCheckTask, I made it run with the validate target, so it'll be included with ant precommit.

          Run alone, it took less than a tenth of second in each of lucene/ and solr/ on my machine.

          I think it's ready to go.

          Show
          Steve Rowe added a comment - Patch implementing the idea as a custom Ant task: LibVersionCheckTask . I had originally planned a scripted implementation, but I figured that could fail for valid ivy-version.properties syntax, or valid ivy.xml syntax, since a script would likely make assumptions about the format, so I went with full parsing for both, though I had to write the Properties file parser, since java.util.Properties doesn't provide file-order access to the parsed contents. The new task checks the /org/name keys in the centralized versions file for lexical sort order and for duplicates, then verifies that the rev attributes for all <dependency> -s in all ivy.xml files in the current directory ( lucene/ and solr/ ) and below are formatted in ${/org/name } format, where org matches the value of the "org" attribute, and name matches the value of the "name" attribute. I modeled it on (and stole a lot of it from) the custom LicenseCheckTask . The patch includes a fix to the ivy-versions.properties file that the new task found: there are currently duplicate property keys for two of the httpcomponents jars. Like LicenseCheckTask , I made it run with the validate target, so it'll be included with ant precommit . Run alone, it took less than a tenth of second in each of lucene/ and solr/ on my machine. I think it's ready to go.
          Hide
          Steve Rowe added a comment -

          New patch with a few changes:

          • Added orphan checking for /org/name coordinate keys in ivy-versions.properties file that are not referred to in any ivy.xml file.
          • In order to add orphan checking, I had to make the task run only at the top level, so that it sees all the ivy.xml files across the whole project. The simplest way to do that with the current build was to place the check-lib-versions target in lucene/build.xml, and then invoke it from the top-level validate target, recursively checking the ivy.xml files in the parent of the lucene/ directory. (This is a little clunky - really it should live at the top level - but I can live with it.)
          • Cleaned up the ivy-versions.properties file, so that versions shared by two or more libs are represented as a property named for their org/groupId (and sometimes part of their name/artifactId if the org/groupId didn't seem to be sufficiently differentiating).

          I'll commit later today if there are no objections.

          Show
          Steve Rowe added a comment - New patch with a few changes: Added orphan checking for /org/name coordinate keys in ivy-versions.properties file that are not referred to in any ivy.xml file. In order to add orphan checking, I had to make the task run only at the top level, so that it sees all the ivy.xml files across the whole project. The simplest way to do that with the current build was to place the check-lib-versions target in lucene/build.xml , and then invoke it from the top-level validate target, recursively checking the ivy.xml files in the parent of the lucene/ directory. (This is a little clunky - really it should live at the top level - but I can live with it.) Cleaned up the ivy-versions.properties file, so that versions shared by two or more libs are represented as a property named for their org/groupId (and sometimes part of their name/artifactId if the org/groupId didn't seem to be sufficiently differentiating). I'll commit later today if there are no objections.
          Hide
          Steve Rowe added a comment -

          Patch with cosmetic cleanups and a CHANGES.txt entry.

          Show
          Steve Rowe added a comment - Patch with cosmetic cleanups and a CHANGES.txt entry.
          Hide
          ASF subversion and git services added a comment -

          Commit 1529243 from Steve Rowe in branch 'dev/trunk'
          [ https://svn.apache.org/r1529243 ]

          LUCENE-5257: Lock down centralized versioning of ivy dependencies

          Show
          ASF subversion and git services added a comment - Commit 1529243 from Steve Rowe in branch 'dev/trunk' [ https://svn.apache.org/r1529243 ] LUCENE-5257 : Lock down centralized versioning of ivy dependencies
          Hide
          ASF subversion and git services added a comment -

          Commit 1529246 from Steve Rowe in branch 'dev/trunk'
          [ https://svn.apache.org/r1529246 ]

          LUCENE-5257: Fix top-level validate target subant invocation syntax

          Show
          ASF subversion and git services added a comment - Commit 1529246 from Steve Rowe in branch 'dev/trunk' [ https://svn.apache.org/r1529246 ] LUCENE-5257 : Fix top-level validate target subant invocation syntax
          Hide
          ASF subversion and git services added a comment -

          Commit 1529248 from Steve Rowe in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1529248 ]

          LUCENE-5257: Lock down centralized versioning of ivy dependencies (merged trunk r1529243 and r1529246)

          Show
          ASF subversion and git services added a comment - Commit 1529248 from Steve Rowe in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1529248 ] LUCENE-5257 : Lock down centralized versioning of ivy dependencies (merged trunk r1529243 and r1529246)
          Hide
          ASF subversion and git services added a comment -

          Commit 1529249 from Steve Rowe in branch 'dev/trunk'
          [ https://svn.apache.org/r1529249 ]

          LUCENE-5257: merge CHANGES.txt entry with LUCENE-5249's entry

          Show
          ASF subversion and git services added a comment - Commit 1529249 from Steve Rowe in branch 'dev/trunk' [ https://svn.apache.org/r1529249 ] LUCENE-5257 : merge CHANGES.txt entry with LUCENE-5249 's entry
          Hide
          ASF subversion and git services added a comment -

          Commit 1529250 from Steve Rowe in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1529250 ]

          LUCENE-5257: merge CHANGES.txt entry with LUCENE-5249's entry (merged trunk r1529249)

          Show
          ASF subversion and git services added a comment - Commit 1529250 from Steve Rowe in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1529250 ] LUCENE-5257 : merge CHANGES.txt entry with LUCENE-5249 's entry (merged trunk r1529249)
          Hide
          Steve Rowe added a comment -

          Committed to trunk and branch_4x.

          Show
          Steve Rowe added a comment - Committed to trunk and branch_4x.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development