Add java_thrift_library fingerprint strategy
Review Request #2265 - Created May 24, 2015 and submitted
|areitz, benjyw, fkorotkov, ity, jinfeng, jsirois, patricklaw, stuhood, tejal, zundel|
This rb adds fingerprint strategy for java_thrift Library. It fixes the pants cache bug - java thrift library does not include language, compiler and rpc style in its hash key generation. As a result, if any of those values changes, pants will throw an error due to cache.
For example, if a java_thrift_library target was compiled, but then changed its rpc style from sync to fingle, and compile again. Since hash didn't change, pants doesn't invalidate the target. Pants would eventually return an error because it couldn't find the file at the cached location.
To provide a more specific error, it would try to find a file like
but the actual file is .pants.d/gen/scrooge/java-sync/gen-file-map-by-target/src.thrift.com.twitter.bouncer.bounce-action-thrift-java
See the relevent discussion here https://groups.google.com/forum/#!topic/pants-devel/9I2oiTNmOhg
Please discard RB #2258.
./pants test contrib/scrooge/tests/python/pants_test/contrib/scrooge/tasks
and manually verified invalidation by changing rpc_style and verified pants cache worked correctly when pants target didn't change.
This seems expedient, but it's not awesome. Adding those parameters to the payload in the constructor would be best, but AFAIK, the options aren't available to target constructors at the moment.
Please document (in both places) why we're using a figerprintstrategy rather than using the payload directly. A cleanup of task/target payloads seems inevitable, and leaving behind a clear explanation for the choice would be helpful during that process.
Address RB comments
1) Add comments on the reason behind FingerPrintStrategy
2) Simplify JavaThriftLibrary accessors by moving default value logic into tasks
Just make all these these static methods instance methods since they all need self.context.options - no need to pass it in.
Really these 3 options should be housed in a Subsystem and then ScroogeGen and ApacheThriftGen could use the subsystem to query these properties, leaving the option export and access encapsulated in the Subsystem.
An example of a Subsystem is here:
To use it in tasks, you'd just:
@classmethod def global_subsystems(cls): return (ThriftLibrarySettings,) ... thrift_library_settings = ThriftLibrarySettings.global_instance() language = thrift_library_settings.get_language(target) ...
If you're up for that switch - its where things should really be. If not, please add a TODO.
-Converted static methods to instance methods
-Added TODO for the subsystem since I am trying to meet the release deadline.
-Make conditionals more readable.
Revision 3 (+176 -25)