Make tox -evendor idempotent. #651
Conversation
This affords adding a CI check to ensure we never commit vendored code with modifications beyond import re-writes. Fixes pex-tool#649 Fixes pex-tool#650
pex/vendor/__main__.py
Outdated
| if result != 0: | ||
| raise VendorizeError('Failed to vendor {!r}'.format(vendor_spec)) | ||
|
|
||
| # We know we can get these as a by-product of a pip install but never need them. | ||
| safe_rmtree(os.path.join(vendor_spec.target_dir, 'bin')) | ||
| # We know we can get this as a by-product of a pip install but never need it. |
There was a problem hiding this comment.
NB: Instead of deleting bin/ post-install, we now instruct install to use a tmp dir for bin now. This is because the scripts wheel writes to bin/ form part of the checked-in RECORD file digests and their contents can change (namely the #!/path/to/python shebang). Using a tempdir - a dir outside the --target install root - eliminates the bin/ scripts from being enteed into the RECORD.
There was a problem hiding this comment.
Upon further review had to go a different route to deal with this robustly; thus: 7a72d65
We now have the vendor-check / git as the stand-in for the RECORD, so this is ok.
There was a problem hiding this comment.
Code looks good to me. However, I can't get tox to complain for improperly changing a file.
I pulled this down, modified /pex/vendor/_vendored/wheel/wheel/metadata.py like I had originally done, ran tox, and everything passed. It should have failed on the vendor-check stage, right?
Here's the diff to repro, followed by tox
diff --git a/pex/vendor/_vendored/wheel/wheel/metadata.py b/pex/vendor/_vendored/wheel/wheel/metadata.py
index eca49bc..7afdf78 100644
--- a/pex/vendor/_vendored/wheel/wheel/metadata.py
+++ b/pex/vendor/_vendored/wheel/wheel/metadata.py
@@ -13,7 +13,7 @@ from .pkginfo import read_pkg_info
# Wheel itself is probably the only program that uses non-extras markers
# in METADATA/PKG-INFO. Support its syntax with the extra at the end only.
-EXTRA_RE = re.compile("""^(?P<package>.*?)(;\s*(?P<condition>.*?)(extra == '(?P<extra>.*?)')?)$""")
+EXTRA_RE = re.compile(r"^(?P<package>.*?)(;\s*(?P<condition>.*?)(extra == '(?P<extra>.*?)')?)$")
MayRequiresKey = namedtuple('MayRequiresKey', ('condition', 'extra'))One further consideration: originally I thought you were going to change the file permissions for _vendored files to be -r--r--r-- only. How come you kept it at -rw--r--r--? Necessary for us to rewrite the imports, it sounds like?
--
One change I'd request is to mention in the squashed commit message that this reverts 16cfd97.
|
@Eric-Arellano did you run As to perms, the idea was malformed. Git only differentiates the executable bit - you can't store read-only. |
|
I'll mention the (partial) revert. |
|
Ah that's it. I was running Is this expected? I had the file |
|
It's not a with-travis thing, it's an idempotency against the committed state thing. You should repro on your machine if you 1st commit your edit. |
There was a problem hiding this comment.
Ah that's it, thanks John for clarifying.
I was able to reproduce that this would have caught my original mistake from #651. Both the behavior and the code look good.
This affords adding a CI check to ensure we never commit vendored code
with modifications beyond import re-writes.
Fixes #649
Fixes #650