Add content_hashing feature flag #107
Conversation
There was a problem hiding this comment.
Hey, thanks for the pr! I think what you're saying makes sense.
Maybe we could put this behind a feature flag rather than adding a new argument to python_binary? That way we can enable it by default with the next major version as this will be a breaking change. There's an example in the go-rules plugin, where you can then access it in the build def with something like if "content_hash" in CONFIG.PYTHON.FEATURE_FLAGS....
Also do we need to add the lib_rule to the deps of the pex_rule? Maybe I'm missing something but they're not used by the pex_rule and they're added to the final binary as a dep. The hash will just be passed into the pex tool via the command line arg.
|
@samwestmoreland the dependency is necessary, because if not, the lib rule is not built, and its hash isn't available. But if you have a nicer option, feel free to share it :) Also, the feature flag seems like a great idea, thanks. Do you have any pointer about how the feature flags thing work ? I haven't encountered this construst yet. If you think this is a straightforward thing, I'll have a look at the code code to figure it out |
elthariel
force-pushed
the
feat-pex-content-stamp
branch
from
5323bbe to
e155691
Compare
|
I think I've implemented it the way you suggested ? I could not find any reference of the WheelHashing feature flag anywhere, so I removed it. Was I right ? |
elthariel
changed the title
|
Thanks for the changes, @elthariel. Yeap the wheelhashing feature flag can be removed, and everything else looks good, thanks! |
Fixes #106, by allowing to use the dependencies content hash instead of the
$RULE_HASHas the pex stamp.I don't think I properly understand the reasoning behind the use of RULE_HASH, but I still intuitively feels that using the rule hash instead of the content hash brings a quite weird user experience and that content_stamp=True might be a reasonable default ? WDYT ?
Also, I think the generated hash is a bit long. Maybe we could only take the last N characters of the hash ?