Use binary mode in snapshot tool for fopen #2371
Conversation
akosthekiss
left a comment
akosthekiss
left a comment
There was a problem hiding this comment.
If this PR is about the handling of text/binary file differences on Windows, then it should also touch the other command line tools as well:
- main-unix.c uses a function called
read_fileto read both js and snapshot files. That will have problems with the snapshots on Windows, too. IMHO it should be updated to read js files as text and snapshot files as binary. (I.e., add an extra param for the mode and let the caller ofread_filedecide.) - main-unit-test.c is like main-unix.c.
Moreover, main-unix-snapshot has one more fopen in process_merge at line 489. That should also handle files as binaries IMO.
| const char *file_name) /**< file name */ | ||
| { | ||
| FILE *file = fopen (file_name, "r"); | ||
| FILE *file = fopen (file_name, "rb"); |
There was a problem hiding this comment.
The same comment goes here as for main-unix.c and main-unix-test.c: the caller of read_file should decide whether its a text or binary mode open.
There was a problem hiding this comment.
The reason for always reading a binary file is to avoid any line ending conversion on Windows, even when we are reading text files.
| } | ||
|
|
||
| FILE *literal_file_p = fopen (save_literals_file_name_p, "w"); | ||
| FILE *literal_file_p = fopen (save_literals_file_name_p, "wb"); |
There was a problem hiding this comment.
Yes, but I want to preserve the line endings so they are the same on Windows and Linux.
akosthekiss
added
tools
galpeter
force-pushed
the
fix-snapshot-fopen
branch
from
5185f2e to
58a3e22
Compare
|
Travis: tests/jerry/windows-line-ending.js: incorrect license |
akosthekiss
left a comment
akosthekiss
left a comment
There was a problem hiding this comment.
The PR looks good to me in general. But the license issue must be solved. It comes most probably from the line endings (i.e., the script that checks the license headers cannot deal with CRLF and thus doesn't identify the existing header as the expected one).
|
Created a PR to improve the license checker: #2391 |
In the snapshot tool the files were opened in text mode. However the snapshot files are binary files thus it is advised to use the binary mode when opening the files. Specifying the binary mode is a must on Windows platform otherwise the read/write operations are inserting extra '\r' characters. To make the tools consitent across OSes all fopen are now opening files in binary mode. Also update jerry-libc to accept the 'b' modifier and add a test case where the JS file uses CR-LF line endings. JerryScript-DCO-1.0-Signed-off-by: Peter Gal pgal.u-szeged@partner.samsung.com
galpeter
force-pushed
the
fix-snapshot-fopen
branch
from
58a3e22 to
5937a6f
Compare
|
@akosthekiss I've updated the PR, the CRLF problems are solved. |
4 participants
In the snapshot tool the files were opened
in text mode. However the snapshot files
are binary files thus it is advised to use the
binary mode when opening the files.
Specifying the binary mode is a must on Windows
platform otherwise the read/write operations
are inserting extra '\r' characters.
Also update jerry-libc to accept the 'b' modifier.