Java thrift library hash key for cache to include thrift fields
Review Request #2258 — Created May 22, 2015 and discarded
This rb fixes the issue which pants cache for 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
The fix is to
1) Add compiler, language, rpc style to payload, so they are part of the hash.
2) Fold java thrift library options in global_options into java_thrift_library since they are used as defaults for this class now.
See the relevent discussion here https://groups.google.com/forum/#!topic/pants-devel/9I2oiTNmOhg
./pants test tests/python/pants_test/targets:java_thrift_library
./pants test contrib/scrooge/tests/python/pants_test/contrib/scrooge/tasks
|Chatted with Chris. He is going to bring back options the same way as they were before so that rpc_style, ...||IT ity|
|With this rb, the defaults cannot be override by any config? is that right? Changing compilers or rpc_style will now ...||TE tejal|
|See src/python/pants/backend/jvm/targets/jvm_target.py for how to extend a payload. You need to accept an optional payload as an argument from potential ...||PA patricklaw|
|Why not pull these off of the payload to avoid redundancy?||PA patricklaw|
|I still need to support these options, but I need to migrate them to java_thrift_library. I want to make sure ...||CH chrischen|
The general thrust of this change looks good to me, and I don't think Foursquare has much internally that could interfere with this change.
src/python/pants/backend/jvm/targets/jvm_target.pyfor how to extend a payload. You need to accept an optional payload as an argument from potential subclasses, and then default to a new
Why not pull these off of the payload to avoid redundancy?
LGTM, I think making java_thrift_library take payload as an argument is good feedback.
I am going to write another implementation using FingerprintStrategy which will be cleaner and has better support for options.
Implemented a different approach which was submitted.