Pick up classpath based on target id; Set up infrastructure to pass down full targets' info to external builder

Review Request #3300 — Created Jan. 5, 2016 and submitted

wisechengyi
intellij-pants-plugin
95
pants-reviews
patricklaw, stuhood, tejal, zundel

Earlier https://rbcommons.com/s/twitter/r/3211/ was merged to safely output classpath with target id.

So this change:
1. Allows the plugin to pick up classpath given the exported target id (depending on https://rbcommons.com/s/twitter/r/3291/).
2. Setup the infratrusture to pass Set<TargetAddressInfo> down to the external builder via gson serialization, so further external builder logic correction can be done. For example, to unblock the temp fix (https://rbcommons.com/s/twitter/r/3237/) by telling whether to compile a target by its property is_synthetic in TargetAddressInfo rather than its name.

In order to reassemble Set<TargetAddressInfo> in the external builder, the following two files are currently 'duplcated' via symblinks, but ideally we want to isolate them as a separate target and have others depends on it, but the plugin BUILD relations is a bit of tricky(full of python code), so we might have to come back to this.

jps-plugin/com/twitter/intellij/pants/jps/incremental/serialization/Globs.java
jps-plugin/com/twitter/intellij/pants/jps/incremental/serialization/TargetAddressInfo.java

https://travis-ci.org/pantsbuild/intellij-pants-plugin/builds/102470851

  • 0
  • 0
  • 2
  • 1
  • 3
Description From Last Updated
WI
WI
WI
WI
WI
WI
WI
WI
WI
  1. 
      
  2. jps-plugin/jps-plugin.iml (Diff revision 4)
     
     
     
     
     
     

    Have jps-plugin module include lib in its content, and export gson-2.3.1.jar in its final product

  3. resources/META-INF/plugin.xml (Diff revision 4)
     
     

    include 15.0.2 in the build range

  4. resources/META-INF/plugin.xml (Diff revision 4)
     
     

    include gson-2.3.1.jar in the classpath for the jps-plugin (the external builder)

  5. scripts/prepare-ci-environment.sh (Diff revision 4)
     
     

    include gson in the classpath during ci

  6. testFramework/com/twitter/intellij/pants/testFramework/PantsIntegrationTestCase.java (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    abandon the old way to look for classfile directly under .pants.d/compile/

  7. 
      
ST
  1. 
      
  2. Can the version come in as a variable?

    1. Does not seem doable. This is a IDEA file and $USER_HOME is not set in this project.

  3. Generic util classes are generally a bad idea... since these additions involved adding imports for packages that you didn't need before (jps.incremental), it's worth considering whether the new methods should live in another class.

    1. Accidental inclusion, it is not needed and now removed. Agreed, PantsUtil tends to keep very generic and commonly used functions.

  4. This is a bit of an unusual code style (mistmatches Twitter's at least)... is this the new default for the project?

    1. Been going with the IDEA default style for java

  5. I see warnings floating around that Gson is not threadsafe, so you might consider not keeping this object statically.

  6. Is subclassesing TypeToken here intentional (via the brackets)? If so, can you add a comment above explaining why you're doing this?

    1. It is to help deserialize the Set<TargetAddressInfo>. Comment added and moved into the method.

  7. next on an empty iterator will throw... can you be certain that there will be a value here, or do you need to account for that somewhere?

    1. size checked now as well

  8. Since this method will only be called with one argument or the other, it should probably be split into separate methods for each approach.

    Alternatively, could split into two implementation Strategy classes and then mark the old one '@deprecated'

    1. Split into the two methods.

  9. 
      
WI
WI
WI
WI
WI
ST
  1. 
      
  2. jps-plugin/BUILD (Diff revision 7)
     
     
     
     
     
     
     
     
     

    Hm... is it necessary to check this in rather than get it via ivy? Let's make another pass to fix this.

  3. 
      
WI
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as d4a62e86d1000256a9e39d1ef34950fc9c84da91

Loading...