Fix handling of data files when creating venvs. #1632
Conversation
This allows vendoring to perform the same fixups we do for resolves and leave vendored distributions with whole metadata.
This uses the RECORD as the basis for determining which files to copy and where they belong, fixing the issues with data files not being properly located.
|
Thanks in advance - this is large. It's not quite as large as it looks though. 1/2 the delta-lines are taken up by the re-vendor. |
| buffer.write(cast(bytes, line)) | ||
| first_non_shebang_line = None | ||
|
|
||
| def _fixup_record(self, modified_scripts=None): |
There was a problem hiding this comment.
| modified_scripts = list(self._fixup_scripts()) | ||
| self._fixup_record(modified_scripts=modified_scripts) | ||
|
|
||
| def _fixup_scripts(self): |
There was a problem hiding this comment.
| # https://github.com/pypa/pip/issues/1150 | ||
| # | ||
| # We also deal with this, accepting either `-` or `_` when a `-` is expected. | ||
| version_pattern = re.sub( |
There was a problem hiding this comment.
N.B.: An existing unit test and another IT that both started failing drove / forced this fix.
|
|
||
| # The RECORD is a csv file with the path to each installed file in the 1st column. | ||
| # See: https://www.python.org/dev/peps/pep-0376/#record | ||
| for line, (path, fingerprint, file_size) in enumerate( |
There was a problem hiding this comment.
Given the janky state of some PyPI artifacts, I'm guessing malformed RECORD files may exist, and this unpacking may fail? Possibly worth checking the length of the parsed line before attempting to unpack and skipping instead of failing on any messed up lines?
There was a problem hiding this comment.
Thankfully Pex is immune from all but vendored Pip`s quirks here because Pex uses it to do the install; i.e. vendored Pip writes the RECORD.
| """Represents the Pip installation scheme used for installing a wheel. | ||
|
|
||
| For more about installation schemes, see: | ||
| https://docs.python.org/3/install/index.html#alternate-installation |
There was a problem hiding this comment.
Is this the right link to put here? The link refers to distutils but these schemes are pip flags, so that link is a bit confusing (to me at least) in this context. E.g., naively, I don't see --root or --target mentioned there but I do see --home ?
There was a problem hiding this comment.
That's the definition at any rate. Pip comes up with custom schemes but those are only defined in code.
| """Re-installs the installed wheel in a venv. | ||
|
|
||
| N.B.: A record of reinstalled files is returned in the form of an iterator that must be | ||
| consumed to drive the installation to completetion. |
There was a problem hiding this comment.
| consumed to drive the installation to completetion. | |
| consumed to drive the installation to completion. |
There was a problem hiding this comment.
I'll try to get this in a follow-up that touches this file. I routinely pass on typo fixes in non-public docs since it burns trees and the proof the typo is not severe is already there in the fact it could be understood enough to be corrected.
There was a problem hiding this comment.
Yeah, I don't think typos are worth a CI round, but can be incidentally fixed in other changes.
|
Thanks Benjy. I'll be circling back here as part of #1631 and I'll address your typo feedback and any further feedback from others there or separately. |
The fix in pex-tool#1632 introduced a regression for the narrow case of `--venv` mode PEXes using `--no-venv-site-packages-copies` (symlinks) and running with a `PEX_PATH` including other PEXes with duplicate distributions where those distributions provide only top-level modules (as opposed to packages). Fix this issue in `Record.reinstall` and also fix duplicate handling in `PEX.resolve` and `PEXEnvironment.activate`. Add tests that cover all of this. Fixes pex-tool#1637
The fix in #1632 introduced a regression for the narrow case of `--venv` mode PEXes using `--no-venv-site-packages-copies` (symlinks) and running with a `PEX_PATH` including other PEXes with duplicate distributions where those distributions provide only top-level modules (as opposed to packages). Fix this issue in `Record.reinstall` and also fix duplicate handling in `PEX.resolve` and `PEXEnvironment.activate`. Add tests that cover all of this. Fixes #1637
Previously, Pex failed to install distributions containing data files
correctly. The data files were located under site-packages (which is
where package data, aka resources, belong) instead of at the
sys.prefixas mandated by distutils.See: https://docs.python.org/3/distutils/setupscript.html#distutils-additional-files
Fixes #1630