Lucene - Core
  1. Lucene - Core
  2. LUCENE-930

fail build if contrib tests fail to compile

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.2
    • Component/s: general/build
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      spinoff of LUCENE-885, from Steven's comments...

      Looking at the current build (r545324) it looks like the some contrib failures are getting swallowed. Things like lucli are throwing errors along the lines of

      [subant] /home/barronpark/smparkes/work/lucene/trunk/common-build.xml:366: srcdir "/home/barronpark/smparkes/work/lucene/trunk/contrib/lucli/src/test" does not exist!

      but these don't make it back up to the top level status.

      It looks like the current state will bubble up junit failures, but maybe not build failures?

      ...

      It's "test-compile-contrib" (if you will) that fails and rather being contrib-crawled, that's only done as the target of "test" in each contrib directory, at which point, it's running in the protected contrib-crawl.

      Easy enough to lift this loop into another target, e.g., build-contrib-test. And that will start surfacing errors, which I can work through.

      1. LUCENE-930.patch
        6 kB
        Hoss Man
      2. LUCENE-930.patch
        6 kB
        Hoss Man
      3. LUCENE-930.patch
        5 kB
        Hoss Man
      4. LUCENE-930.patch
        2 kB
        Hoss Man

        Issue Links

          Activity

          Hide
          Hoss Man added a comment -

          patch orriginally contributed by Steven Parkes in LUCENE-885 using the name "LUCENE-885-pt2.patch"

          Show
          Hoss Man added a comment - patch orriginally contributed by Steven Parkes in LUCENE-885 using the name " LUCENE-885 -pt2.patch"
          Hide
          Hoss Man added a comment -

          Damn, i forgot to include these comments from Steven regarding the patch...

          This patch file removes the swallowed failures by doing more of the build steps out of the guarded loop (that only detects junit failures, not others).

          In addition the patch, need to create and "svn add"
          A contrib/wordnet/src/test
          A contrib/similarity/src/test
          A contrib/lucli/src/test

          This makes it possible to actually run the gdata-server tests, which were not running before. I have seen one case where it hung, but maybe that was a fluke.

          Show
          Hoss Man added a comment - Damn, i forgot to include these comments from Steven regarding the patch... This patch file removes the swallowed failures by doing more of the build steps out of the guarded loop (that only detects junit failures, not others). In addition the patch, need to create and "svn add" A contrib/wordnet/src/test A contrib/similarity/src/test A contrib/lucli/src/test This makes it possible to actually run the gdata-server tests, which were not running before. I have seen one case where it hung, but maybe that was a fluke.
          Hide
          Steven Parkes added a comment -

          Thanks, Chris. Sorry for the extra steps.

          Show
          Steven Parkes added a comment - Thanks, Chris. Sorry for the extra steps.
          Hide
          Hoss Man added a comment -

          No worries Steven.

          Looking at the patch now, i'm wondering if it's really worth adding the top level "compile-test-contrib" target, or if the top level "build-contrib" target should just take care of calling the compile-test target (and making test-contrib depend on build-contrib) ... it would be one less time we have to crawl the contribs, and the only "downside" is that there is no longer a way to build all contirbs without compiling the tests for each contrib as well ... is that really that bad? is anyone building binary distributions without running the tests anyway? would those people really care if the build took a little longer because it compile the tests even though they weren't run?

          Show
          Hoss Man added a comment - No worries Steven. Looking at the patch now, i'm wondering if it's really worth adding the top level "compile-test-contrib" target, or if the top level "build-contrib" target should just take care of calling the compile-test target (and making test-contrib depend on build-contrib) ... it would be one less time we have to crawl the contribs, and the only "downside" is that there is no longer a way to build all contirbs without compiling the tests for each contrib as well ... is that really that bad? is anyone building binary distributions without running the tests anyway? would those people really care if the build took a little longer because it compile the tests even though they weren't run?
          Hide
          Michael Busch added a comment -

          > would those people really care if the build took a little longer
          > because it compile the tests even though they weren't run?

          IMO this is fine.

          > and since i'm not certain these additional changes will make it
          > into the 2.2 branch (not my call to make)

          Since this is contrib stuff I would say it is ok to get it into
          2.2 as long as we also fix the failing tests with this patch.

          Show
          Michael Busch added a comment - > would those people really care if the build took a little longer > because it compile the tests even though they weren't run? IMO this is fine. > and since i'm not certain these additional changes will make it > into the 2.2 branch (not my call to make) Since this is contrib stuff I would say it is ok to get it into 2.2 as long as we also fix the failing tests with this patch.
          Hide
          Hoss Man added a comment -

          1) rather then add empty src/test directories to contribs (which might confuse people: they might assume tests exist by seeing the dir, they might assume they didn't get the tests in their release since the dir is empty, they might svn remove the dirs not realizing it will break the build, etc...) i made the contrib-build.xml skip the compile-test and test targets if there are no tests.

          2) Encountered a problem while testing the patch: tests in some contribs rely on the core tests files (spellchecker depends on the English class for example) and "ant clean test-contrib" doesn't ensure that the core tests are compiled (because build-contrib no longer depends on compile-test). I view this is really a problem with the way contrib dependencies are built and not the main build.xml ... on a clean checkout you should be able to do "cd contrib/foo; ant test" and have it automatically build all your dependencies (this already works for the core lucene jar because of the "build-lucene" task). so I made some additions to contrib-build.xml to support this (a "build-lucene-test" task), and the spellchecker contrib that needed it.

          3) with Michael's encouragement, i went ahead and removed the "compile-test-contrib" target and just made "build-contrib" take care of it ... this involved adding a new "build-jar-and-tests" since contrib-crawl/subant only support a single target name ... this is much cleaner in my opinion then the old way where build-contrib would just run whatever the 'default' target was for each contrib (which could be named anything, and could do anything) .. now the expected semantics are clearer (although i'm open torenaming the target)
          ...but i ran into a slight snag because of the "javacc-uptodate-check" init depends on which doesn't work for "meta-contribs" like gdata and db that don't have a src dir ... the task even has a TODO that it really only needs to be done for a few contribs, so that looks like a good thing to fix to ... but i've got to run now. i'll try to update the patch a little later tonight (any feedback in the meantime would be appreciated)

          Show
          Hoss Man added a comment - 1) rather then add empty src/test directories to contribs (which might confuse people: they might assume tests exist by seeing the dir, they might assume they didn't get the tests in their release since the dir is empty, they might svn remove the dirs not realizing it will break the build, etc...) i made the contrib-build.xml skip the compile-test and test targets if there are no tests. 2) Encountered a problem while testing the patch: tests in some contribs rely on the core tests files (spellchecker depends on the English class for example) and "ant clean test-contrib" doesn't ensure that the core tests are compiled (because build-contrib no longer depends on compile-test). I view this is really a problem with the way contrib dependencies are built and not the main build.xml ... on a clean checkout you should be able to do "cd contrib/foo; ant test" and have it automatically build all your dependencies (this already works for the core lucene jar because of the "build-lucene" task). so I made some additions to contrib-build.xml to support this (a "build-lucene-test" task), and the spellchecker contrib that needed it. 3) with Michael's encouragement, i went ahead and removed the "compile-test-contrib" target and just made "build-contrib" take care of it ... this involved adding a new "build-jar-and-tests" since contrib-crawl/subant only support a single target name ... this is much cleaner in my opinion then the old way where build-contrib would just run whatever the 'default' target was for each contrib (which could be named anything, and could do anything) .. now the expected semantics are clearer (although i'm open torenaming the target) ...but i ran into a slight snag because of the "javacc-uptodate-check" init depends on which doesn't work for "meta-contribs" like gdata and db that don't have a src dir ... the task even has a TODO that it really only needs to be done for a few contribs, so that looks like a good thing to fix to ... but i've got to run now. i'll try to update the patch a little later tonight (any feedback in the meantime would be appreciated)
          Hide
          Hoss Man added a comment -

          updated version of the patch .. still not quite perfect and i need to do a bunch more testing, but i'm having some issues with my home machine which may result in a complete rebuild so i wanted to upload this version of the patch incase that happens.

          Show
          Hoss Man added a comment - updated version of the patch .. still not quite perfect and i need to do a bunch more testing, but i'm having some issues with my home machine which may result in a complete rebuild so i wanted to upload this version of the patch incase that happens.
          Hide
          Hoss Man added a comment -

          (FYI: the javacc thing was ared herring)

          ok, i am 99% sure this patch is GOLDEN ... it should take care of everything Steven pointed out, and more.

          What this this patch does...

          High Level...

          • "build-contrib" now calls a new "build-artifacts-and-tests" target
            on every contrib (previously it just called the default target)
            ... any failures in any contrib fail the build.
          • "test-contrib" now depends on "build-contrib" ... along with the
            previous point, this solves addresses the crux of the issue Steven
            noticed.
          • By default, the compile-test and test targets of contribs without
            tests do nothing (previously they failed because of a missing directory)
          • cleanup of the build files for various contribs so that various
            dependencies work better.

          Details...

          • changes to contrib-build.xml...
          • add a build-artifacts-and-tests target
          • better support for "build-lucene" (executed in proper directory
            now)
          • new support for "build-lucene-tests"
          • test and compile-test now do nothing if the contrib has no tests
          • changes to gdata-server build.xml...
          • test now depends on compile-test
          • added build-artifacts-and-tests (because it can't import
            contrib-build.xml)
          • adds explicit compile-core, compile-test, and
            build-artifacts-and-tests targets to the db build.xml (meta-contrib
            can't use the ones inherited from contrib-build.xml)
          • clean up the bdb, bdb-je, and benchmark build.xml files so their
            init targets depend on the contrib-build init target (and doesn't
            skip up to common.init)
          • change spellchecker build.xml so it's compile-test depends on
            build-lucene-test (it compiles against English.java)
          • main build.xml ...
          • build-contrib now contrib-crawls build-artifacts-and-tests
          • test-contrib depends on build-contrib

          I'll try to do some more testing of various build target permutations tomorrow – particularly under java 1.4 – but i'd appreciate it if other people could try beating on it as well

          Show
          Hoss Man added a comment - (FYI: the javacc thing was ared herring) ok, i am 99% sure this patch is GOLDEN ... it should take care of everything Steven pointed out, and more. What this this patch does... High Level... "build-contrib" now calls a new "build-artifacts-and-tests" target on every contrib (previously it just called the default target) ... any failures in any contrib fail the build. "test-contrib" now depends on "build-contrib" ... along with the previous point, this solves addresses the crux of the issue Steven noticed. By default, the compile-test and test targets of contribs without tests do nothing (previously they failed because of a missing directory) cleanup of the build files for various contribs so that various dependencies work better. Details... changes to contrib-build.xml... add a build-artifacts-and-tests target better support for "build-lucene" (executed in proper directory now) new support for "build-lucene-tests" test and compile-test now do nothing if the contrib has no tests changes to gdata-server build.xml... test now depends on compile-test added build-artifacts-and-tests (because it can't import contrib-build.xml) adds explicit compile-core, compile-test, and build-artifacts-and-tests targets to the db build.xml (meta-contrib can't use the ones inherited from contrib-build.xml) clean up the bdb, bdb-je, and benchmark build.xml files so their init targets depend on the contrib-build init target (and doesn't skip up to common.init) change spellchecker build.xml so it's compile-test depends on build-lucene-test (it compiles against English.java) main build.xml ... build-contrib now contrib-crawls build-artifacts-and-tests test-contrib depends on build-contrib I'll try to do some more testing of various build target permutations tomorrow – particularly under java 1.4 – but i'd appreciate it if other people could try beating on it as well
          Hide
          Michael Busch added a comment -

          Hoss,

          I tried the new patch out on Windows and Linux, JDK 1.5. I ran different target combinations - it always finished successful. Will review the patch itself tomorrow (am a bit too tired now...). Thank you for taking care of all these build issues, Hoss!

          Show
          Michael Busch added a comment - Hoss, I tried the new patch out on Windows and Linux, JDK 1.5. I ran different target combinations - it always finished successful. Will review the patch itself tomorrow (am a bit too tired now...). Thank you for taking care of all these build issues, Hoss!
          Hide
          Hoss Man added a comment -

          yeah, i've done testing with jdk1.5/ant1.6.5, jdk1.4/ant1.6.5 and jdk1.5/ant1.7.0 (all Linux) everything looks good .. i'll let it stew for a few more hours before committing.

          Show
          Hoss Man added a comment - yeah, i've done testing with jdk1.5/ant1.6.5, jdk1.4/ant1.6.5 and jdk1.5/ant1.7.0 (all Linux) everything looks good .. i'll let it stew for a few more hours before committing.
          Hide
          Michael Busch added a comment -

          Alright, I think this patch looks good. Maybe Steven could take a look as well? Then I think you can go ahead and also commit it to the 2.2 branch.

          Show
          Michael Busch added a comment - Alright, I think this patch looks good. Maybe Steven could take a look as well? Then I think you can go ahead and also commit it to the 2.2 branch.
          Hide
          Hoss Man added a comment -

          Committed revision 546226. (trunk)
          Committed revision 546235. (2.2 branch)

          Show
          Hoss Man added a comment - Committed revision 546226. (trunk) Committed revision 546235. (2.2 branch)

            People

            • Assignee:
              Hoss Man
              Reporter:
              Hoss Man
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development