add --jvm-distributions-{min,max}imum-version options

Review Request #3310 — Created Jan. 7, 2016 and submitted

mlandis
pants
landism:mlandis/optionize_distribution_version
2776
pants-reviews
stuhood, zundel

Users building with JDK 8 with -target 7 can hit various problems due to pants' lack of -bootclasspath (https://github.com/pantsbuild/pants/issues/2396). Until that exists, it's useful to be able to force a specific JDK version.

https://travis-ci.org/pantsbuild/pants/builds/101124263

  • 0
  • 0
  • 5
  • 0
  • 5
Description From Last Updated
ZU
  1. Ship It!
  2. 
      
ST
WI
  1. 
      
  2. Should it be JDK instead of JRE? same for the other test case

    1. I don't see a particular reason it should be. This is the error message already emitted by pants when it fails to find a distribution matching the constraints. If the question is about the inconsistency between "jre" here and "jdk" in the test names, that's fair. Changed the test names to "jvm" (which is still inconsistent with "jre", but is consistent with "--jvm-distributions").

  3. 
      
ML
GM
  1. 
      
  2. Looks like the global minimum_version and maximum_version options are only used if not specified as parameters; I.E. a jvm_platform will override these for running java, unit tests, etc. Which is fine, but that should be documented in the help text for these options.

    1. good point!
      I don't think this is a reasonable behavior, so instead of updating the documentation, I've changed it to honor both the option and the argument.

  3. 
      
ML
GM
  1. 
      
  2. Due to how the minimum_version and maximum_version are now computed, they could now very easily come from different sources, which could yield unintended impossible results.

    For example, if a caller to cached() asks for (min=1.6, max=1.7), but in pants.ini the minimum_version and maximum_version options are set to (min=1.8, max=1.9)

    Here, the min will resolve to 1.8 (which is the stricter minimum), and the max will resolve to 1.7, because that's the stricter maximum, so the computed range will be an impossible combination of the two of them: (min=1.8, max=1.7)!

    I think we should probably through an error here if the maximum version is less than the minimum version, because that means the user's configuration went bad somewhere.

    1. It's currently throwing an error saying, e.g. "Failed to locate a JDK distribution with minimum version 1.8, maximum version 1.7", but yeah, probably good to make clear that this is due to the pants config/options rather than just a pants bug.
      (it'd be even nicer to say where those constraints came from, because even if we say it's a configuration issue, it'll probably require reading pants code to figure out that, e.g., the other constraints are coming from --test-junit-strict-jvm-version, but that is hard and was already a problem before this change)

  3. Why the lambda indirection? You can just pass max here.

    1. because I'm bad at python
      thanks

  4. same comment as above

  5. 
      
ML
GM
  1. Thanks for the error messaging! Please put the name of your branch in the 'branch' field for this review.

  2. 
      
ML
ML
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as 9ece216a3019db4c9b0771fa188554bdffd24fc8

Loading...