Use the z.jar files on the zinc classpath instead of the destination directory of the class files.

Review Request #3982 — Created June 7, 2016 and submitted

gmalmquist
pants
gmalmquist/zundel-compile-with-zjars
3529, 3567
pants-reviews
benjyw, nhoward_tw, stuhood, zundel

Use the z.jar files on the zinc classpath instead of the destination directory of the class files.

This is zundel's patch from https://rbcommons.com/s/twitter/r/3955 with changes according to Stu's feedback. I made the changes since zundel is OOO.

CI went green: https://travis-ci.org/pantsbuild/pants/builds/135986552
CI went green: https://travis-ci.org/pantsbuild/pants/builds/136462391

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
  1. Title/description need edits.

  2. I think you'll need to add a guarantee to this method that it doesn't include the entry for the target being compiled... or that it always includes the output directory.

  3. 
      
  1. 
      
  2. Why add conditional code here when if you leave off the condition, the end results are the same?
    Also, I am afraid this will cause the logic to miss the last path on the classpath which must be omitted when you add classpath jars before the compile. This is probably mostly harmless, but not really desirable either.

    1. I guess I should have made this a 'task'. You'll be introducing an idiosyncracy here if you leave the 'if' test in place.

    2. If the end result is the same, why bother doing the O(n) step?

      I don't think this misses the last thing on the classpath. Why would it?

    3. Currently, when you get done compiling, I'm concerned that the runtime_classpath will look like:

      a/z.jar, b/z.jar, c/

      We want it to be

      a/z.jar, b/z.jar, c/z.jar

    4. It will be a/z.jar, b/z.jar, c/z.jar. Why wouldn't it be? All I did is change the mechanism that previously made it a/, b/, c/ to a/z.jar, b/z.jar, c/z.jar. If c/ was on the classpath as things were previously, c/z.jar will be there now.

    5. OK, I stand corrected. I patched in this change and added a breakpoint and saw that classpath_product doesn't have any loose classpath dirs in it.

      My concern was adding in the loose file dir to the classpath at line 622 might have introduced such a directory on the classpath, but I see now that isn't persisted into the compile_classpath product.

  3. 
      
  1. 
      
  2. Maybe name just --use-classpath-jars? It's already nested under compile, so the compile in the option name is a little redundant. Also, if the default is False, we could leave that arg off.

  3. This moves the current target's compile classpath entry from the first position in the list to the last.

    That could make things slower if zinc looks up incremental classes in the classpath instead of the output directory.

    Is the reordering necessary?

    1. (1) Incremental compile doesn't work with this anyway.
      (2) The reordering wasn't intentional; from what I can tell cp_entries.append(compile_context.classes_dir) isn't actually even required here, but Stu thought it might be necessary.

      1. This is a general code path, so incremental compile will use the classpath created here.
      2. I think you're probably right about that. We could test whether that's true or not if we wanted to be sure.
  4. Since incremental doesn't work when jars are enabled, maybe we should make setting both an error?

    1. Yeah, I can do that.

    2. I don't think its strictly true that incremental won't work with this. Incremental should still work if you just change code within a single target.

    3. That's a good point. I'd been under the impression that passing the analysis files of dependencies to zinc when the class files were in jars caused errors, which I think is clearly wrong.

    4. So... should I change it back so you can use both --incremental and --use-classpath-jars?

    5. Yes. That sounds good to me.

    6. That's correct. It's less effective, but will still work when there is only one changed target.

      Nonetheless, --use-classpath-jars definitely needs to be in the fingerprint.

  5. 
      
  1. Ship It!
  2. 
      
  1. Ship It!
  2. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

In 93894d4d59a5d80afb3dfc7be0a7d5a49d497b29.

Thanks Eric (for writing the original patch and reviewing my changes), and Benjy, Stu, Mateo, and Nick!

Loading...