feat: normalize line endings to LF in noarch packages - #2625
Conversation
Text files that are checked out or generated on Windows often carry CRLF (or bare CR) line endings. When those files end up in a `noarch` package, the resulting archive differs from one built on Unix, breaking reproducibility across platforms (issue #837). `write_to_dest` now normalizes UTF-8 text file line endings to LF for any `noarch` package (both `generic` and `python`, gated on `target_platform == Platform::NoArch`). To stay safe, only valid UTF-8 text is rewritten; binary files and other encodings (e.g. UTF-16/UTF-32, where a naive byte-level CRLF rewrite could corrupt multi-byte code units) are copied verbatim, as are files that already use LF exclusively (which also preserves the fast copy path and file metadata). File permissions are preserved when a file is rewritten. Uses the existing `line-ending` crate for normalization. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015wnV3gL96hhbBuUd7hjejY
Classify each file from its first kilobyte and only pull UTF-8 text files that actually contain a carriage return fully into memory. Binary files, other encodings, and LF-only text now go straight through `fs::copy` without an intermediate in-memory buffer, and the redundant normalized-vs-original comparison is replaced by a cheap `memchr` scan for '\r'. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015wnV3gL96hhbBuUd7hjejY
The project's clippy config disallows std::fs::{read,write,metadata,
set_permissions}. Switch the new test helpers to the fs_err alias used
throughout the crate.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015wnV3gL96hhbBuUd7hjejY
pb01ka
left a comment
There was a problem hiding this comment.
I am thinking about the case where a file is the diff between CRLF/CR and LF. Then \r will be normalised there as well but may be the presence of \r was needed there for test purposes (or anything else?). May be it's a rare case, but plausible for tooling/CI-related packages.
I guess we can make this normalisation optional? Just an open question.
conda/ceps#174 - Worth noting here, especially - conda/ceps#174 (comment).
Rest everything looks good. :).
| // Only touch files that actually contain a carriage return, and only if the | ||
| // whole file is valid UTF-8 (the leading kilobyte can be misleading). | ||
| if memchr::memchr(b'\r', &buffer).is_some() | ||
| && let Ok(text) = std::str::from_utf8(&buffer) | ||
| { | ||
| fs::write(dst, LineEnding::normalize(text).as_bytes())?; | ||
| // Preserve the permissions (e.g. the executable bit) that `fs::copy` | ||
| // would otherwise carry over. | ||
| fs::set_permissions(dst, fs::metadata(src)?.permissions())?; | ||
| return Ok(()); | ||
| } | ||
| } | ||
|
|
||
| // Binary, non-UTF-8, or already LF-only files are copied unchanged. |
There was a problem hiding this comment.
I think once the buffer has the full file from src, we can directly copy from buffer to dst instead of falling through to fs::copy(src, dst). What do you say?
Side note - I tried the following change, cargo test -p rattler_build_core --lib packaging::file_mapper and it passed the tests.
| // Only touch files that actually contain a carriage return, and only if the | |
| // whole file is valid UTF-8 (the leading kilobyte can be misleading). | |
| if memchr::memchr(b'\r', &buffer).is_some() | |
| && let Ok(text) = std::str::from_utf8(&buffer) | |
| { | |
| fs::write(dst, LineEnding::normalize(text).as_bytes())?; | |
| // Preserve the permissions (e.g. the executable bit) that `fs::copy` | |
| // would otherwise carry over. | |
| fs::set_permissions(dst, fs::metadata(src)?.permissions())?; | |
| return Ok(()); | |
| } | |
| } | |
| // Binary, non-UTF-8, or already LF-only files are copied unchanged. | |
| // Only trust the classification once the whole file is confirmed valid | |
| // UTF-8 (the leading kilobyte can be misleading). We've already paid the | |
| // cost of reading the whole file into memory at this point, so write it | |
| // back out directly instead of re-reading/re-copying it from disk via | |
| // `fs::copy`, whether or not it actually needed normalizing. | |
| if let Ok(text) = std::str::from_utf8(&buffer) { | |
| if memchr::memchr(b'\r', &buffer).is_some() { | |
| fs::write(dst, LineEnding::normalize(text).as_bytes())?; | |
| } else { | |
| fs::write(dst, &buffer)?; | |
| } | |
| // Preserve the permissions (e.g. the executable bit) that `fs::copy` | |
| // would otherwise carry over. | |
| fs::set_permissions(dst, fs::metadata(src)?.permissions())?; | |
| return Ok(()); | |
| } | |
| } | |
| // Binary or non-UTF-8 files are copied unchanged. |
|
I started to be unsure about this PR actually .. maybe it is OK to tell people that rebuilding a noarch package identically on Windows is not possible. I think if we want to do this normalization we should probably also have a way to escape it. And then it's usually easy enough to just build on a UNIX platform ... |
I agree. And the consensus on my CEP is also similar. So we can do this here as well. If it helps (and sounds good to you) I can do it by checking out from your branch and creating a new PR. Please let me know. |
Summary
Makes
noarchpackages reproducible across Windows and Unix by normalizing text-file line endings to LF at packaging time. Closes #837.This is a reworked version of #2193, which had gone stale against
main(the packaging code was moved into therattler_build_corecrate) and reimplemented line-ending handling by hand. This version rebases ontomain, plugs into the live packaging path (write_to_dest), and uses theline-endingcrate that was already a workspace dependency.What changed
copy_normalizing_line_endings(src, dst)incrates/rattler_build_core/src/packaging/file_mapper.rs.write_to_destcalls it instead offs::copywhentarget_platform == Platform::NoArch— this is the canonical noarch check and covers bothnoarch: genericandnoarch: python, while correctly excluding platform-specific ABI3 (version_independent) packages.Behavior
line_ending::LineEnding::normalize.\r\n → \nrewrite corrupts multi-byte encodings — for exampleU+0A0Din UTF-16LE is the byte sequence0D 0A, whose0Dwould be wrongly stripped. Restricting normalization to valid UTF-8 avoids this class of corruption.fs::copy, preserving the fast copy path and original file metadata.Tests
test_copy_normalizing_line_endings: CRLF→LF, bare CR→LF, LF-unchanged, mixed endings, binary-verbatim, and a UTF-16LE case proving the multi-byte corruption is avoided.test_copy_normalizing_line_endings_preserves_mode(unix): verifies both that CRLF is normalized and that the0o755mode is preserved.Notes for reviewers
noarch: genericornoarch: pythonshould have the unix line ending #837 ("most or all files ... unix line ending"). If you'd prefer an opt-out or a narrower set of files, that's easy to add.1.95.0toolchain lacks the clippy component and the stable fallback is too old for the workspace), but the crate compiles warning-free under1.95.0and all tests pass.Prompt
🤖 Generated with Claude Code
Generated by Claude Code