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

Windows tests are failing #3

Closed
sylvestre opened this issue Jan 27, 2024 · 9 comments
Closed

Windows tests are failing #3

sylvestre opened this issue Jan 27, 2024 · 9 comments

Comments

@sylvestre
Copy link
Collaborator

sylvestre commented Jan 27, 2024

---- context_diff::tests::test_permutations_reverse stdout ----
thread 'context_diff::tests::test_permutations_reverse' panicked at src\context_diff.rs:653:33:
Output { status: ExitStatus(ExitStatus(3)), stdout: "patching file target/context-diff//alefr\r\n", stderr: "Assertation failed!\r\n\r\nProgram: C:\\Strawberry\\c\\bin\\patch.exe\r\nFile: .\\src\\patch\\2.5.9\\patch-2.5.9-src\\patch.c, Line 354\r\n\r\nExpression: hunk\r\n" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- ed_diff::tests::test_permutations stdout ----
thread 'ed_diff::tests::test_permutations' panicked at src\ed_diff.rs:222:38:
called `Result::unwrap()` on an `Err` value: Error { kind: NotFound, message: "program not found" }

I wonder if this isn't the Windows "patch" command failing with \n ?!

@sylvestre
Copy link
Collaborator Author

@calixteman has been looking into it :)

@cakebaker
Copy link
Contributor

The ed_diff tests fail because ed is not installed. I opened a separate ticket for it as it is not a Windows-only issue.

@oSoMoN
Copy link
Collaborator

oSoMoN commented Apr 15, 2024

The failures look very similar to the error described in https://gnuwin32.sourceforge.net/packages/patch.htm#install:

On MS-Windows, the patchfile must be a text file, i.e. CR-LF must be used as line endings. A file with LF may give the error: "Assertion failed, hunk, file patch.c, line 343," unless the option '--binary' is given.

@oSoMoN
Copy link
Collaborator

oSoMoN commented Apr 15, 2024

I ran some tests in a windows VM, and I can confirm that diff.exe uses CR-LF (\r\n) as line endings.

Our implementation mostly uses the writeln macro, the documentation of which explicitly states:

On all platforms, the newline is the LINE FEED character (\n/U+000A) alone (no additional CARRIAGE RETURN (\r/U+000D).

@oSoMoN
Copy link
Collaborator

oSoMoN commented Apr 20, 2024

According to actions/runner-images#5459 (comment), and seeing the contents of stderr in our failures, the patch.exe binary that gets used is from Strawberry Perl, and it is probably not what we want.
The Git for Windows installer installs recent versions of the diffutils tools in C:\Program Files\Git\usr\bin\.

@oSoMoN
Copy link
Collaborator

oSoMoN commented Apr 20, 2024

#57 makes 13 failures (out of 16) go away.

@oSoMoN
Copy link
Collaborator

oSoMoN commented Apr 20, 2024

I fixed the remaining test failures in #58.
The problem wasn't with line endings, after all.

That doesn't mean we shouldn't look into line ending differences on Windows (see #42), but it should be tracked in a separate issue.

@oSoMoN
Copy link
Collaborator

oSoMoN commented Apr 21, 2024

That doesn't mean we shouldn't look into line ending differences on Windows (see #42), but it should be tracked in a separate issue.

I ran some tests on Windows. With the very old build of diffutils distributed by the Gnuwin32 project (version 2.8.7, 20 years old!), diff.exe rewrites all line endings to \r\n, even content lines that ended with \n only. However with a more recent build distributed in the Git for Windows installer (version 3.10), the rewrite doesn't happen any longer, and diff.exe behaves like on Unix, i.e. it defaults to \n for line endings (and content lines ending with \r\n are preserved). This might be due to a build-time option or the build environment, but regardless, our implementation isn't completely inconsistent with what's out there, so I don't think this warrants a separate issue, and perhaps #42 can be abandoned?

@sylvestre
Copy link
Collaborator Author

yeah, sounds good, thanks

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

No branches or pull requests

3 participants