Adding a special '$JAVA_HOME' symbol for use in jvm platforms args.

Review Request #3924 — Created May 23, 2016 and submitted

benjyw, jsirois, nhoward_tw, zundel
This addresses a long-standing TODO in pants.ini:

    # TODO(gmalmquist): Find a way to resolve the -Xbootclasspath
    # automatically, either by putting rt.jars up on the nexus or
    # using some kind of special variable name (see discussion on
    # RB 2494).

Now, at compile-time, the args from the jvm platform settings will
be pre-processed to replace all instances of `$JAVA_HOME` with the
java home determined by the platform's preferred jvm distribution.

The code will attempt to look up the distribution strictly, but if
that fails it will fallback on a non-strict lookup. "Strict" here
means a version X such that

    target_level <= X <= target_level.9999

Whereas unstrict is simply:

    target_level <= X

Manual testing + added a test to tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/

Travis went green:
Jenkins went green:

Travis went green again:

  • 0
  • 0
  • 3
  • 0
  • 3
Description From Last Updated
  1. This is fantastic Garrett.

  2. pants.ini (Diff revision 1)

    Update the .md documentation. pants.ini in the pants repo is only seen by pant devs

  2. I think the -C is a zinc specific option. Maybe we should let zinc supply the -C

    1. Maybe. This is already how we do things as-is though, so that'd be a breaking change.

    2. I think I'm going to vote for changing the -C thing in a separate patch if it's desired, because we'd have to bump the major version I think?

  2. Since the appropriate java distribution is unknown and it appears on inspection that appropriate -Xbootclasspaths vary from target distribution to target distribution - is this practical? Maybe suggesting a union of target -Xbootclasspaths works in practice since bogus classpath entries are ignored - I think?

    I'm going of this from my machine:

    JDK 6:

    find /usr/lib/jvm/java-6-jdk/jre/lib/ -name "*.jar"

    OpenJDK 8:

    $ find /usr/lib/jvm/java-8-openjdk/jre/lib/ -name "*.jar"

    I think to make this sane a user needs to be able to specify the Xbootclasspath per target, ie 6 has this one, 7 has this one, 8 has this one. Then, you'd say something like 'args': ["-C-Xbootclasspath:$TARGET_JVM_BOOT_CLASSPATH"]. In fact having the user spell this last part would make little sense, and instead code that launches java processes would use helpers to set the Xbootclasspath given the target. As such, at worst, the user just configures their N target boot classpaths (using your $JAVA_HOME idea). At best, later, we might pre-code these for known distributions like OpenJDK 7,8 and OracleJDK 6,7,8.

    1. The value of the $JAVA_HOME variable won't be the same for each target. It's calculated separately for each one, using the target_level to find the appropriate distribution.

      And by specifying the preferred distributions, per-operating system, you can effectively specify exactly what distribution gets used.

    2. OK - I may have target vm vs actual vm used confused, but, say the actual vm used is 7 and the target is 6, my assumption is the args for 7, if -C-Xbootclasspath:$JAVA_HOME/jre/lib/ext/nashorn.jar... are not correct args for the target of 6 even though the $JAVA_HOME correctly points to 6 because $JAVA_HOME/jre/lib/ext/nashorn.jar is only available in JDK 8.
    3. Sure. But the code will try to get a VM for Java 6 first to match the target, and will only fall back on something higher like Java 7 if it can't find anything for Java 6. In which case, if one of those jars isn't available, it will simply be ignored as you say.

    4. OK - So I think at the least the documented advice needs to note the -C-Xbootclasspath needs to be the union of classpaths of the possible target vms you've defined as this stands.
      On a larger scope I think this is indicative of structuring this configuration non-optimally.  The Xbootclasspath should be modelled 1st class as a property of a target vm and code that launches source vms should use the calculated target vm to look up the correct XBootclasspath in code.  You may not agree or you may agree but would like to proceed with a smaller step.  If the latter I'll just highlight the deprecation cycle and config disruption down the road that will be the price to pay for this currently simpler change.
    5. I was thinking of doing the latter. I'm not sure this change will negatively impact the deprecation cycle/config disruption; people can already specify the -Xbootclasspath this way, they just have to be system-specific.

      As things are at Square we currently generate an ini file containing jvm-platforms with the -Xbootclasspath set for the appropriate operating system.

    6. Sounds fine to me, I'd just like to see the union bit noted in the docs.
  2. It would be nice if this were structured as a helper method that could get its own unit test instead of needing an otherwise un-needed slow integration test.
  3. How about lift this into the if and directly zinc_args.extend(a.replace...) only when you actually have settings.args.

  2. src/python/pants/backend/jvm/tasks/jvm_compile/zinc/ (Diff revision 2)

    I feel like this should be lifted into a property / method of JvmPlatform itself rather than living here.

    There may be other places where we'd want to have the substitution happen, eg export. Also, it'd be nice to be able to cache the substituted values rather than recomputing them for each compile target.

    1. I'm pulling it into a classmethod in zinc_compile for now; I'm not sure it'd be great to add the subtle dependency on the DistributionLocator subsystem to a property of JvmPlatformSettings. Easy to move later; this is very non-public api.

  3. How about using a test that compiles against 1.6 with an import / usage from 1.7 that sees that it fails?

    It looks to me like this test might be able to pass without the accompanying patches since the code snippet has no std lib dependencies. I could be wrong though.

    1. I'm deleting this test anyway as per John's recommendation :-P

  1. This should use -bootclasspath, not -Xbootclasspath. -Xbootclasspath changes bootclasspath for javac itself rather than what it is compiling, which may cause problems if javac comes to depend on jdk signatures that don't exist in the target version. javac is pretty conservative about what language features / library APIs it uses, but it would be better to not change the bootclasspath for the compiler itself.

    1. You mean in the .md file? None of the actual code in this patch is specific to -Xbootclasspath vs -bootclasspath.

    2. Yes. I originally had that as an issue on the md text, but saw it in pants.ini as well and decided to make it a general comment.

  2. nit: arguments given in the jvm platform settings.

  3. nit: use settings_args instead of settings.args here, or you could just inline settings_args.

  4. This bit could stand to move into _get_zinc_arguments

    1. You're right, I didn't notice that.

  5. I'd like to see more cases here:
    - empty args
    - failed strict lookup + passing loose lookup
    - failed both lookups

    1. I can do the first one for sure, but the second two I'm not sure if I can do, because they depend on whatever jvm distributions are on the system running the tests.

    2. You could setup a dummy version of the distribution subsystem that blows up in the right way.

    3. +1 - and you want the subsystem_instance context manager - not create_subsystem. I've run across and fixed several tests that failed to reset their subsystems!

    4. Is there an example I can look up for setting up a dummy subsystem in a test that replaces calls to a normal subsystem? I don't think I've seen that before.

    5. It would be one thing if the test called the subsystem directly, but I'd need to spoof the calls made by JvmPlatform.

    6. You could use pants_test.subsystem.subsystem_instance to set one up in the env to monkey with. After thinking it through, I'd be ok without covering that portion if you want.

    7. I've used subsystem_instance() to set up a subsystem, but I'm not sure how I would replace an existing subsystem with a fake one using that? If I say subsystem_instance(FakeDistributionLocator), that doesn't affect calls to DistributionLocator.cached, I don't think?

    8. Is there an example ..

      Please grep subsystem_instance and profit.

    9. In perhaps the most obvious place:
    10. That isn't using a fake DistributionLocator subsystem. That's just instantiating the real one. That's why that test is has all those @skipIf(_get_two_distributions() is None, 'Could not find java 7 and java 8 jvms to test with.') decorators -- because they fail if the system doesn't have the right JVM installs.

    11. AFAICT none of the existing uses try to mock a subsystem, these all use the real ones I think:

      ~/Src/Pants gmalmquist/java-home-symbol-for-bootclasspath find tests -name '*.py' | xargs grep 'with subsystem_instance[(]' | sort
      tests/python/pants_test/backend/jvm/subsystems/    with subsystem_instance(ScalaPlatform, **options):
      tests/python/pants_test/backend/jvm/subsystems/    with subsystem_instance(JarDependencyManagement, **options) as manager:
      tests/python/pants_test/backend/jvm/subsystems/    with subsystem_instance(DistributionLocator):
      tests/python/pants_test/backend/jvm/subsystems/    with subsystem_instance(DistributionLocator):
      tests/python/pants_test/backend/jvm/tasks/  with subsystem_instance(DistributionLocator):
      tests/python/pants_test/backend/jvm/tasks/    with subsystem_instance(DistributionLocator):
      tests/python/pants_test/backend/jvm/tasks/      with subsystem_instance(JarDependencyManagement):
      tests/python/pants_test/backend/jvm/tasks/    with subsystem_instance(JarDependencyManagement, **scoped_options) as manager:
      tests/python/pants_test/backend/jvm/tasks/    with subsystem_instance(IvySubsystem) as ivy_subsystem:
      tests/python/pants_test/backend/jvm/tasks/    with subsystem_instance(ScalaPlatform):
      tests/python/pants_test/backend/jvm/tasks/    with subsystem_instance(ScalaPlatform):
      tests/python/pants_test/backend/project_info/tasks/        with subsystem_instance(DistributionLocator) as locator:
      tests/python/pants_test/backend/project_info/tasks/    with subsystem_instance(ScalaPlatform, **scala_options):
      tests/python/pants_test/backend/project_info/tasks/      with subsystem_instance(IvySubsystem) as ivy_subsystem:
      tests/python/pants_test/backend/project_info/tasks/      with subsystem_instance(IvySubsystem) as ivy_subsystem:
      tests/python/pants_test/backend/python/tasks/    with subsystem_instance(JVM):
      tests/python/pants_test/backend/python/      with subsystem_instance(SourceRootConfig):
      tests/python/pants_test/backend/python/      with subsystem_instance(ThriftBinary.Factory) as thrift_binary_factory:
      tests/python/pants_test/backend/python/    with subsystem_instance(IvySubsystem) as ivy_subsystem:
      tests/python/pants_test/backend/python/    with subsystem_instance(JVM):
      tests/python/pants_test/backend/python/    with subsystem_instance(PythonSetup) as python_setup:
      tests/python/pants_test/backend/python/    with subsystem_instance(SourceRootConfig):
      tests/python/pants_test/bin/            with subsystem_instance(Reproducer, **options) as repro_sub:
      tests/python/pants_test/build_graph/    with subsystem_instance(Target.UnknownArguments):
      tests/python/pants_test/build_graph/    with subsystem_instance(Target.UnknownArguments, **options):
      tests/python/pants_test/ivy/    with subsystem_instance(IvySubsystem) as ivy_subsystem:
      tests/python/pants_test/ivy/    with subsystem_instance(IvySubsystem):
      tests/python/pants_test/ivy/    with subsystem_instance(IvySubsystem):
      tests/python/pants_test/ivy/    with subsystem_instance(IvySubsystem) as ivy_subsystem:
      tests/python/pants_test/ivy/    with subsystem_instance(IvySubsystem) as ivy_subsystem:
      tests/python/pants_test/java/distribution/    with subsystem_instance(DistributionLocator) as locator:
      tests/python/pants_test/java/distribution/    with subsystem_instance(DistributionLocator) as locator:
      tests/python/pants_test/java/distribution/  with subsystem_instance(DistributionLocator, **options) as locator:
      tests/python/pants_test/pantsd/subsystem/    with subsystem_instance(PantsDaemonLauncher.Factory, **options) as factory:
      tests/python/pants_test/pantsd/subsystem/    with subsystem_instance(WatchmanLauncher.Factory) as factory:
      tests/python/pants_test/    with subsystem_instance(SourceRootConfig):
      tests/python/pants_test/    with subsystem_instance(SourceRootConfig):
    12. DistributionLocator.cached uses a discovery stack with env vars 1st in the list, comined with you've got a solution I think.
    13. Mmm, okay. I see. I didn't know existed, and I was confused because I thought you were suggesting the same solution as Nick (making a mock Distribution subsystem), but you were not.

    14. Granted, I was not exuberantly helpful spelling things out.  I expected you could dig a little and figure this out.  I've been disappointed in general in folks ability or willingness to invest effort in things in general as of late.  This is a reflection on me though; I do know there is a reality thing that intervenes and pays checks.  I took a break for a bit but may need to just step away a bit longer to regain tolerance.  I have no doubt folks are doing their best or what they think is their best.
  2. Since cls is not used you could just make this a @staticmethod.

  3. 2 lines when the param type is not a builtin:

    :param settings: The jvm platform settings from which to extract ...
    :type settings: :class:`JvmPlatformSettings`
  4. s/settings.args/settings_args/ or else don't bother extracting above on line 211 at all.
  2. src/python/pants/backend/jvm/tasks/jvm_compile/zinc/ (Diff revision 4)

    I'd prefer if this only happened if there were $JAVA_HOMEs to substitute. Otherwise, we're adding a new per target debug entry for a no-op operation.

    But, I'd be ok with coming up with a solution to that as a follow up patch.

  1. Ship It!
Review request changed

Status: Closed (submitted)

Change Summary:

In fd2316eac40bc58eac21fafbd54092367f68b7ae. Thanks John, Nick, & Eric.

  2. This should likely be memoized, or maybe computed once per platform in that Subsystem?