Improve python_binary target fingerprinting.

Review Request #4353 — Created Nov. 4, 2016 and submitted

kwlzn
pants
4028
pants-reviews
jsirois, stuhood, yujiec

This repairs a user-reported caching bug seen in our repo related to python_binary targets and caching. With caching turned on, adding zip_safe=False to an existing, already built/cached python_binary target will produce a stale binary that does not properly enable the zip_safe flag as expected, leading to user confusion.

Summary:

  • Add Payload fields for PythonBinary attributes.
  • Add a test case that breaks without this change and passes after.
  • Add some improved logging for payload/task/target fingerprinting for easier field debugging of similar issues.
  • Add fingerprint=True to a couple of key python-repos options to improve options fingerprinting behavior.

CI is green: https://travis-ci.org/pantsbuild/pants/builds/173540934

+ local testing against a repro in our repo.

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
  1. 
      
  2. This line will now not do the right thing (renders as PythonBinary(address not yet set) now). You'll need to accept the address in the constructor and use it in place of self in the TargetDefinitionException constructor call... or something more useful like that.

    1. ah, good call. moved this back under the super().__init__ as it was.

  3. I have no strong preference and the way you did things scopes the change smaller, but many targets just store the fields in self.payload as you do here but also then read the fields via self.payload.<field>.

    1. I was wondering about that, but err'd on the side of minimal change - went ahead and applied this throughout.

  4. I think it would be great not to be mutating checked in files despite the care taken here. My preference is definitely to be able to see everything in the test; ie: emit the file contents to a temp build root here in the code, and then mutation also becomes no concern.  There is certainly plenty of precedent for this style of test though, so obviously your call.
    1. I don't feel strong about it, but +1 to John's suggestion.

    2. I wasn't able to find any prior art for temp build_root construction/usage in integration tests, so I whittled something out that seems to work. let me know if this is along the lines of what you were expecting or if there's a better way.

  5. 
      
  1. LGTM! Just a few nits.

  2. Just want to point out that this is indeed a copy. The original pex binary is under .pants.d. But if you are changing this on purpose I am fine with it. After all, users don't need to know whether it is a copy or not.

    1. yup, but its a copy only in the internal context of pants. to the users, this is the only pants-created pex they ever see publically - hence the removal of the copy terminology from the user-focused message.

  3. I think it is more common that 3rdparty import lies between builtin import and pants import.

    1. fixed - oddly enough, isort.sh did not seem to catch this.

  4. 
      
  1. Thanks for adding the `mock_buildroot` utility.
  2. Does a symlink cause issues for these? If not it seems like it would be nice for both speed and uniformity to just symlink them too.
    1. good call - seems like BUILD.tools does indeed have an issue when symlinked, but went ahead and moved the rest.

  3. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

thanks John & Yujie! this is submitted @ https://github.com/pantsbuild/pants/commit/d88e7595485d87071e8a1678a27b76f0fe2f3bc8

Loading...