On Windows, CRLF line endings are not preserved when using '--inplace'

[cezchitos]

On Windows, CRLF line endings are not preserved when using '--inplace'.
After the formatter is run, the file gets converted to 'LF' (should remain 'CRLF').

[IEncinas10]

Hello!

A couple of questions:

1. This only happens for --inplace, right? (Just double-checking)
2. Can you compile verible from Windows?

I'm guessing this was caused by 13e0cd0. It should be an easy fix, the hard part is testing it

[hzeller]

The 13e0cd0 should've made it work with Windows at all, I suspect before all kinds of offset might've been wrong, but I also don't have access to a Windows machine, all fixes were informed by roundtrips with the github CI.

I agree there should probably a configuration option that allows to alternatively emit CRLF line endings, which can be set manually and is auto-configured depending on the original input. Please send a pull request.

[cezchitos]

@IEncinas10 Apologies, I was away for a bit.

1. I will test this later, but I believe so. I use it without '--inplace' indirectly through TerosHDL and VSCode without any problems, so I'm not sure if that does something in the background. But I will double check.
2. I just did, and I also tested with the change currently in common: util: stop using binary mode in SetContents #2371 ('wb' to 'w') and I've captured the result in a video since it was easier:

https://streamable.com/rvf634

Basically in 'wb' mode (as is on master) CRLF line endings get replaced with LF
In 'w' mode (change in PR), CRLF line endings are preserved, but an extra line is added / removed every time the program runs.

To note that verible-verilog-format.exe is build from master as-is, and verible-verilog-format-edit.exe is with the PR change. You can see the line endings in the editor in the bottom right in the video.

I think the extra line issue in 'w' mode might be related to #2383, as it also only happens on lines ending in comments.

Let me know if you want me to better explain what is happening in the video or if you want me to test something else.

PS. At the moment after using verible-format on Windows, I just read and write the file in python and that takes care of the line endings for me, just in case someone else also need a (temporary) fix.

[cezchitos]

I need to check more, but I think the change in #2371 combined with:

diff --git a/verible/verilog/parser/verilog.lex b/verible/verilog/parser/verilog.lex
index f90216cc..2acd4ea8 100644
--- a/verible/verilog/parser/verilog.lex
+++ b/verible/verilog/parser/verilog.lex
@@ -127,7 +127,7 @@ LeadingPathChars ({Letter}|{Digit}|[_.?*/])
 FilePath {LeadingPathChars}{PathChars}*

 /* white space */
-LineTerminator \r|\n|\r\n|\0
+LineTerminator [\r\n|\n|\r|\0]
 InputCharacter [^\r\n\0]
 InputCharacterNoBackslash [^\\\r\n\0]
 Space [ \t\f\b]

Also fixes #2383, or at least the error on my end that looked like that. And works for both CRLF and LF files (keeping the original formatting), but only on Windows. Should I raise a PR or @IEncinas10 will you look into this as part of your work in #2371?

[IEncinas10]

Thank you for the help and for testing the patches!

I would say go ahead and open another PR for the lex issue. I'll update my PR soon following @hzeller's advice.

[hzeller]

#2371 added an option --line_terminator=CRLF that makes the output be \r\n; this is now submitted (thanks to @IEncinas10 !).

The functionality does not auto-detect (yet), so for now you have to provide this option to get the desired result @cezchitos

[cezchitos]

Thank you for your work on this, @IEncinas10 @hzeller.

I have tried using it with the line_terminator=CRLF option and I get the following error:

Formatted output is lexically different from the input.    Please file a bug.  Details:
First mismatched token [4]: ("// end of line comment") (#714: "// 32-bit Priority Encoder Module\r") vs. ("// end of line comment") (#714: "// 32-bit Priority Encoder Module")

If I use it with the line_terminator=LF option, there is no error, but as expected line endings change.

This is using the verible-v0.0-3997-gd6924079-win64.zip release.

[IEncinas10]

Thats a shame :( Thanks for testing it! I'll try to trigger it on my side. Could you share a small file that causes the issue?

[cezchitos]

I have attached a test file. Interestingly, if I remove the timescale 1ns / 1ps line, it works ok.

test_file.txt