Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow and warn on RECORD lines with more than three elements #6191

Merged
merged 2 commits into from
Jan 24, 2019

Conversation

cjerdonek
Copy link
Member

This fixes #6165.

@pradyunsg
Copy link
Member

I'd prefer that instead of using row[0], row[1], row[2] we use ",".join(row[:-2]), row[-2], row[-1].

That way, it'll result in a valid RECORD for the case where we're failing. It's better than printing a broken RECORD.

@cjerdonek
Copy link
Member Author

The reason I did it that way is because that's how it was before (link to the change):

for row in reader:
row[0] = installed.pop(row[0], row[0])
if row[0] in changed:
row[1], row[2] = rehash(row[0])
outrows.append(tuple(row))
for f in generated:
digest, length = rehash(f)
outrows.append((normpath(f, lib_dir), digest, length))
for f in installed:
outrows.append((installed[f], '', ''))

It seems like restoring to the original code is safest and less likely to introduce new problems. Right now, pip just ignores elements beyond the third one. One problem with ",".join(row[:-2]), row[-2], row[-1] is that it can change something that would have been okay for pip to something that isn't. For example:

('path', 'digest', 'size', 'garbage')

would become the following, which seems worse:

('path,digest', 'size', 'garbage')

We don't know how many cases there are like this in the wild, so there's a chance it could create a bigger or at least different problem. It doesn't seem like a hotfix is the best time to experiment with fixing this other problem. Releasing with the warning should give us a better sense of the types of invalid RECORD files that are out there and so in a better position to revisit it.

@pradyunsg
Copy link
Member

Releasing with the warning should give us a better sense of the types of invalid RECORD files that are out there and so in a better position to revisit it.

Fair enough. :)

@cjerdonek
Copy link
Member Author

Okay, thanks a lot, @pradyunsg. Let's remember to perhaps create a new issue regarding how to handle invalid RECORD lines like this (if there isn't already an issue). (It's a somewhat similar discussion to #5913, regarding a different way in which RECORD files can be problematic.)

@lock
Copy link

lock bot commented May 30, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 30, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pip install fails with ValueError: too many values to unpack
2 participants