Migrating more data to payload fields in jvm_app and jvm_binary targets

Review Request #2011 — Created March 31, 2015 and submitted

zundel
pants
zundel/update_jvm_binary_payload
1349
007af85...
pants-reviews
jsirois, nhoward_tw, patricklaw
  • All fields custom to jvm_binary and jvm_app should now be reflected in the payload.
  • Added more tests to test_jvm_binary and test_jvm_app
  • Made tests for exceptions more explicit in test_jvm_app to make sure errors
    of type TargetDefinintionExeption have an address specified
  • New custom PayloadField added for JarRules

Added unit tests. CI passed at: https://travis-ci.org/pantsbuild/pants/builds/56576452

  • 0
  • 0
  • 9
  • 0
  • 9
Description From Last Updated
ZU
  1. 
      
  2. I could deprecate this if you think folks are using this in their own plugins. There are no references inside of OSS pants.

  3. 
      
ZU
ZU
  1. ping. This fixes a bug in cache validation if you make changes to main, basename, jar_rules or other fields in the BUILD file for jvm_app and jvm_binary targets.

  2. 
      
PA
  1. 
      
  2. I'm a bit iffy of relying on repr for this (I see it's used for fingerprinting below). Maybe this should just have a fingerprint method? I'm concerned about code drifting and new encapsulated params not being rolled into the repr.

    1. There is precedent for using repr in payload_field (Excludes). Maybe I could use something like: for name in dir(self) and iterate through all of the fields in the object and combine the hashes of the field values? That could be a generic type of payload field we could re-use in other places.

    2. I believe the way to do that would be to maintain a list of attrs as a class attribute that should contribute to the hashing, and then iterate through those using getattr. I'm also not really a fan of that. Is there any reason that these objects can't be namedtuples? That way, you could use the existing helper function to give you back a stable json SHA1 for any json representable object. You could also just give them a fingerprint method and use the same trick.

      I really don't like using repr here, nor do I like the dir trick. Those things will silently work on any object but might very well not be what was intended. I'd be much happier to see a real interface for things that need to be fingerprintable, and for that interface to be relatively unambiguous (and not accidentally spoofable by other objects).

    3. Using namedtuples is hard for the JarRules object because 1: it contains an instance and array of non-primitive objects, neither one of which is suitable for the existing helper function. Also the array is full of objects that can be of multiple types, so I would have to do something special there too.

      What I did was create a new type of PayloadField called FingerprintedField that allows you to define a fingerprint() method right in your object.

      For the implementation of the JarRule fingerprint, I used hash() on fields that are really only identifiable through an object instance. I don't know how you feel about that.

    4. Mentioned it in an issue--can't use hash. One of the annoying things about fingerprinting is that we always have to find a way to stably plumb every possible object that affects the fingerprint through a stable representation that can be SHA1'ed. The approach so far has been to find some translation of the thing into json.

  3. Is order important here? Also might want to guard against being passed a mutable list that's mutated out from under you later.

    1. Yes, I think that order is important here. I don't want to sort them if that's what your getting at. You could have a specific pattern that needs to match first before a more general pattern. I will make a defensive copy in the return value a list. I'm not sure its necessary to make a copy when the field is captured because this list is intended to come from a BUILD target definition.

  4. Why call foo.__repr__() rather than repr(foo)?

  5. type(deploy_jar_rules).__name__

  6. 
      
ZU
ZU
ZU
PA
  1. 
      
  2. Definitely can't do this! hash is unstable across interpreters and builds.

  3. 
      
ZU
  1. 
      
  2. OK, well in that case, I may just replaces these anonymouse placeholder objects

      SKIP = object()
      REPLACE = object()
      CONCAT = object()
      FAIL = object()
    

    and turn them into strings. Any objections there?

    1. I'm not clear on why that's better than just using strings in the first place. I suppose go for it and it'll be clearer from the diff?

    2. I guess we could do that. I'd rather not introduce a BUILD file incompatibility, so I just changed the assignment of the values to strings.

  3. 
      
ZU
PA
  1. 
      
    1. Meant to add, this is a "ship" sans issues.

  2. As silly as it looks, I think this is preferable.

  3. Assuming the mixin isn't an old-style class (which it shouldn't be), the object is unnecessary.

  4. Doesn't need to be a regexp match anymore, I think

  5. Please don't shadow str (even in a test, this is a pretty big one to avoid)

  6. 
      
ZU
ZU
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks for the review Patrick. Commit 6273e07

Loading...