Export preferred_jvm_distributions that pants actually uses (and to deprecate jvm_distributions)

Review Request #3680 — Created April 11, 2016 and submitted

peiyu
pants
3175
pants-reviews
gmalmquist, patricklaw, stuhood, wisechengyi, zundel

This is so that Intellij plugin can auto config project sdk to be the same
that is used by pants. Note the current exported jvm_distributions are
static settings not necessarily the one used by pants.

Today people have to manually config IJ project Sdk, this

  • Is extra work
  • May be different from the jvm distribution pants uses, potentially causes
    surprises. For example, cmdline uses jdk 7, while plugin uses jdk 8.

Both strict and non_strict distributions are provided because user might
use test.junit.strict_jvm_version flag to restrict to use only the requested
version, i.e, jdk6 only for java6 (in the below example)

platform or test_platform is at target level, target level distributions
can be obtained by looking up preferred_jvm_distributions for the platform
that target actually is configured. For IJ to respect target level distribution
it will need to set sdk at module level. To get this integrated into plugin will
therefore be in two phases: 1) set the default project sdk 2) overwrite
the module level sdk.

A sample run:

tw-mbp-peiyu:pants peiyu$ ./pants export
{
    "preferred_jvm_distributions": {
        "java7": {
            "strict": "/Library/Java/JavaVirtualMachines/jdk1.7.0_72.jdk/Contents/Home",
            "non_strict": "/Library/Java/JavaVirtualMachines/jdk1.7.0_72.jdk/Contents/Home"
        },
        "java6": {
            "strict": "/System/Library/Java/JavaVirtualMachines/1.6.0.jdk/Contents/Home",
            "non_strict": "/Library/Java/JavaVirtualMachines/jdk1.7.0_72.jdk/Contents/Home"
        },
        "java8": {
            "strict": "/Library/Java/JavaVirtualMachines/jdk1.8.0_65.jdk/Contents/Home",
            "non_strict": "/Library/Java/JavaVirtualMachines/jdk1.8.0_65.jdk/Contents/Home"
        }
    },
    "libraries": {},
    "version": "1.0.7",
    "targets": {},
    "jvm_platforms": {
        "platforms": {
            "java7": {
                "source_level": "1.7",
                "args": [],
                "target_level": "1.7"
            },
            "java6": {
                "source_level": "1.6",
                "args": [],
                "target_level": "1.6"
            },
            "java8": {
                "source_level": "1.8",
                "args": [],
                "target_level": "1.8"
            }
        },
        "default_platform": "java6"
    }
}

https://travis-ci.org/peiyuwang/pants/builds/122164861 passed
https://travis-ci.org/peiyuwang/pants/builds/122392967 passed

  • 0
  • 0
  • 1
  • 1
  • 2
Description From Last Updated
ZU
  1. 
      
  2. There is already a section called 'jvm_distributions" which looks like:

        "jvm_distributions": {
            "darwin": [
                "/Library/Java/JavaVirtualMachines/jdk1.8.0_45.jdk/Contents/Home",
                "/Library/Java/JavaVirtualMachines/jdk1.7.0_80.jdk/Contents/Home"
            ],
            "linux": [
                "/usr/java/jdk1.8.0_45",
                "/usr/java/jdk1.7.0_80"
            ]
        },
    

    This new section seems to have more information, so maybe we should add comments that this new section is to be preferred and we can eventualy phase 'jvm_distributions' out?

    1. The difference is the existing section is static settings while the new section is the distributions that pants will actually use (static setting as part of the search path)

      Any use case we just need the static setting? if not then we can mark as deprecate, in favor of the new sectioin.

      Will add comment.

    2. in favor of removing the static settings, though (maybe) there needs to be changes on the intellij side to make sure the json parsing is correct.

    3. Added deprecated_conditional at 0.0.89 which is about 8 releases after, since this is very non-ergent.

      Renamed new section to preferred_jvm_distributions.

    4. ok, can you please wait on Garrett to weigh in on this? we use this field in a plugin.

  3. 
      
WI
  1. 
      
  2. a bit easier to reason this way:

    preferred_distributions = {}
    for jdk_type in ['strict', 'nonstrict']:
      dist = ... 
      if dist:
         preferred_distributions[jdk_type] = dist.home
    
    1. Good idea, improved.

  3. 
      
PE
WI
  1. 
      
  2. src/python/pants/backend/project_info/tasks/export.py (Diff revisions 1 - 2)
     
     
     
     
     
     

    based on this, the middle digit needs to increment as a field is removed.

    1. existing field is only marked as deprecated, won't be removed till 0.0.89, at that point we would make a minor version bump?

    2. i see. sounds good!

  3. 
      
GM
  1. This seems like an improvement. On our end I think it will be easy to update our plugin appropriately.

  2. 
      
ZU
  1. Ship It!
  2. 
      
PE
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as 46399f5554e540216a6bbdcff68d0d338cd700d7

ZU
  1. 
      
  2. We are seeing this warning. It uses the exported JSON for our square_idea task. Its not clear what is being deprecated here. Is it the jvm_distributions subsystem? Or is it just the jvm_distributions field in the export file?

    1. Yea, the message isn't super clear. It's the latter, just the export not exporting jvm_distributions, because preferred_jvm_distributions contains more information, there is no reason to have two fields in the json.

      See your first comment of this review for background how this was added.

  3. 
      
Loading...