Skip to content

Read RECORD with the default csv reader, not str.splitlines() - #354

Open
dylanpulver wants to merge 1 commit into
pypa:mainfrom
dylanpulver:record-csv-newlines
Open

Read RECORD with the default csv reader, not str.splitlines()#354
dylanpulver wants to merge 1 commit into
pypa:mainfrom
dylanpulver:record-csv-newlines

Conversation

@dylanpulver

@dylanpulver dylanpulver commented Sep 2, 2026

Copy link
Copy Markdown

construct_record_file writes RECORD with csv.writer, so a path containing a newline comes out correctly quoted. Both read sites — WheelFile.validate_record and WheelFile.get_contents — called str.splitlines() before handing the result to parse_record_file, so installer cannot read the RECORD it just wrote.

Measured on 59f0a65, using installer's own writer and then its own reader:

wrote  '"demo/we\nird.py",sha256=...,6'
read   'demo/weird.py'           (the newline is gone)

validate_record() on that wheel:
  In demo-1.0-py3-none-any.whl, demo/we
  ird.py is not mentioned in RECORD

splitlines() also splits on characters csv does not treat as row separators. I ran the writer and then the reader for all ten: \n and \r come back with the character silently deleted; \v, \f, \x1c, \x1d, \x1e, \x85, U+2028 and U+2029 raise expected 3 elements, got 1. All ten read back byte-identical through csv.reader over the same file.

The recording-installed-projects spec pins the dialect: RECORD "must be readable with the default reader of Python's csv module", line terminator "either \r\n or \n". Interposing splitlines() is a different, larger set of terminators.

Fix is to hand the RECORD text to parse_record_file as io.StringIO(text, newline="").

Ran pytest tests/: 152 passed, coverage report still 100%. Reverting only src/installer/sources.py to origin/main fails both new cases (150 passed, 2 failed). splitlines(keepends=True) is the obvious narrower fix and it does pass the newline case — csv reassembles the quoted field from the kept terminators — but it still fails the U+2028 case, which is why the test parametrises both. ruff check reports the same 3 pre-existing PLR1704 on an unmodified checkout and none in what I touched.

I did not change parse_record_file itself beyond one docstring line; it already accepts any iterable of strings.

AI assistance: this change was drafted with Claude Opus 5 (claude-opus-5). The outputs above were produced by running installer's own writer and reader locally against this branch.

`construct_record_file` writes RECORD with `csv.writer`, so a path
containing a newline is correctly emitted as a quoted multi-line field.
Both read sites called `str.splitlines()` first, which drops the
terminator inside the quoted field and additionally splits on characters
`csv` does not treat as row separators (\v, \f, \x1c-\x1e, \x85, U+2028,
U+2029). `validate_record()` then reports a file as "not mentioned in
RECORD" for a RECORD that does mention it.

Pass the RECORD text through `io.StringIO(..., newline="")` instead, so
the reader sees the document as written.
PNHD

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants