Change Distribution.cached to compare using Revision objects

Review Request #1653 - Created Jan. 23, 2015 and submitted

Information
Mateo Rodriguez
pants
b2769ff...
Reviewers
pants-reviews
jsirois, zundel

When spinning up distributions, Distribution.cached returns
previously instantiated jdk distros. The proper distro to return
was determined by comparing min/max arguments. The problem is that
cached() is generally passed strings . Those strings were being
compared with Revision objects. This should have raised an
exception b/c Revision.__cmp__ is overloaded so as to
compare _components.

This exception was never raised though, because Pants was almost
always just returning the Distribution that was cached during
setup. The _CACHE.key was just (minimum_version, jdk) which
is (None, True) or (None, False) for all situations I could find.

This conspired to cause pants to not respect any max_version
constraints. The solution is to cast the max/min version args
of Distribution.cached to full Revision objects for easy
apples-to-apples comparison. The _CACHE.key has been expanded to
(minimum_version, maximum_version, jdk) so as to honor all
constraints.

PANTS_DEV=1 ./pants test tests::
712 passed, 1 skipped, 5 xfailed, 3 xpassed, 1 warnings in 1201.53 seconds

20:24:06 21:25 [junit]
20:24:06 21:25 [specs]
SUCCESS

Travis passed: https://travis-ci.org/pantsbuild/pants/builds/48128985

John Sirois
Mateo Rodriguez
John Sirois
John Sirois
Mateo Rodriguez
Review request changed

Status: Closed (submitted)

Eric Ayers
Ship It!
Eric Ayers

   

I see that CI is passing, but I just had a local test failure on this line. I did a git clean -fdx and ran it a second time and still have a failure (I'm using macos)

PANTS_DEV=1 ./pants test tests/python/pants:all
...
==================== FAILURES ====================
                     ________ MockDistributionTest.test_cached ________

                     self = <pants_test.java.distribution.test_distribution.MockDistributionTest testMethod=test_cached>

                         def test_cached(self):
                           with self.distribution(executables=self.exe('java', '1.7.0_33')) as jdk:
                             with self.env(PATH=jdk):
                               Distribution.cached(minimum_version='1.7.0_25')
                             with self.env(PATH=jdk):
                               Distribution.cached(maximum_version='1.7.0_55')

                           with self.assertRaises(Distribution.Error):
                             with self.distribution(executables=self.exe('java', '1.7.0_33')) as jdk:
                               with self.env(PATH=jdk):
                                 Distribution.cached(maximum_version='1.6.0_20')

                           with self.assertRaises(Distribution.Error):
                             with self.distribution(executables=self.exe('java', '1.7.0_25')) as jdk:
                                 with self.env(PATH=jdk):
                     >             Distribution.cached(minimum_version='1.8.0_20')
                     E             AssertionError: Error not raised

                     tests/python/pants_test/java/distribution/test_distribution.py:174: AssertionError
                      1 failed, 593 passed, 1 skipped, 5 xfailed, 1 xpassed, 2 warnings in 126.55 seconds 
  1. That's unfortunate. It is passing on my OSX box and my Linux box. This passes for me on OSX whether I have 1.7 or 1.8 on the box's PATH. But investigating has brought up some weird behavior. This test passes even if I adjust the test to:

    with self.assertRaises(Distribution.Error):
      with self.distribution(executables=self.exe('java', '1.7.0_25')) as jdk:
          with self.env(PATH=jdk):
            Distribution.cached(minimum_version='1.7.0_35')
    

    But I can make it fail by further changing it to:

    with self.assertRaises(Distribution.Error):
      with self.distribution(executables=self.exe('java', '1.7.0_25')) as jdk:
          with self.env(PATH=jdk):
            Distribution.cached(minimum_version='1.7.0_30')
    

    Evn though there should be no difference in the result.
    Both of these tests were run with jdk 1.7.0_65 and 1.8.0_11 with identical output. The Revision objects are being compared with __cmp__ overloaded maybe a less naive comparison is needed. I will keep poking at it.

  2. Okay, I have found the bug. It is in the test implementation. The env is being carried over to each new test. I will work up a fix.

  3. I don't think so. Both env and environment_as are properly coded context managers that don't leak their changes outside the with context. The issue here is Distribution.cached which uses a class-level cache that is never cleared before the test methods run. Commented to this effect on https://rbcommons.com/s/twitter/r/1667/

Loading...