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

Use open_in_bin instead of open_in for Windows compatibility #222

Merged
merged 3 commits into from
Jan 22, 2021

Conversation

MisterDA
Copy link
Contributor

No description provided.

The problem is with the snippet:

```ocaml
let read_whole_file path =
    let ch = open_in path in
    let len = in_channel_length ch in
    let s = really_input_string ch len in
    close_in ch;
    s
```

So on Windows with crlf line endings, `open_in` (text mode) does the
conversion but `in_channel_length` does not and counts two characters
per line, as in binary mode; but `really_input_string` is in text mode
and sees only one character per endline. So in the end it will have
seen less characters than expected (the `len` argument) and raise
`End_of_file`.
@MisterDA
Copy link
Contributor Author

MisterDA commented Jan 20, 2021

The problem is with the snippet:

let read_whole_file path =
    let ch = open_in path in
    let len = in_channel_length ch in
    let s = really_input_string ch len in
    close_in ch;
    s

So on Windows with crlf line endings, open_in (text mode) does the
conversion but in_channel_length does not and counts two characters
per line, as in binary mode; but really_input_string is in text mode
and sees only one character per endline. So in the end it will have
seen less characters than expected (the len argument) and raise
End_of_file.

For the URL change, (edit: without the patch) I fixed it by using
dos2unix on the file (that’s where I encountered the exception). For
the other change, I’ve not had any problem but my understanding is
that you’re serializing something and my gut instinct tells me to open
the file in binary mode :)

I’ve also seen code such as below, and I’m wondering if it’ll cause
problems later; I’ll find a way to check that.

let outfile path data =
  let ch = open_out path in (* but was it text mode or bin? *)
  let data = data ^ "\n" in (* check the endline style accordingly *)
  output_string ch data;
  close_out ch

Also:
- Use 4.08's `Fun.protect` to ensure files are closed on error too.
- Add a unit test for the file store.
@talex5
Copy link
Contributor

talex5 commented Jan 22, 2021

Ah, I see! I've pushed some more commits to use binary mode for saving too, and to fix the compiler warning on 4.12.

@talex5 talex5 merged commit 8365bca into mirage:master Jan 22, 2021
@MisterDA MisterDA deleted the open_in_bin branch January 22, 2021 11:36
talex5 added a commit to talex5/opam-repository that referenced this pull request Apr 6, 2021
…nix and capnp-rpc-lwt (1.0)

CHANGES:

- Skip the setting of `object_id` if it is empty (@LasseBlaauwbroek mirage/capnp-rpc#224).
  This improves interoperability with the C++ implementation.

- Use `open_in_bin` instead of `open_in` for Windows compatibility (@MisterDA mirage/capnp-rpc#222).
talex5 added a commit to talex5/opam-repository that referenced this pull request Apr 6, 2021
…nix and capnp-rpc-lwt (1.0)

CHANGES:

- Skip the setting of `object_id` if it is empty (@LasseBlaauwbroek mirage/capnp-rpc#224).
  This improves interoperability with the C++ implementation.

- Use `open_in_bin` instead of `open_in` for Windows compatibility (@MisterDA mirage/capnp-rpc#222).
talex5 added a commit to talex5/opam-repository that referenced this pull request Apr 6, 2021
…nix and capnp-rpc-lwt (1.0)

CHANGES:

- Skip the setting of `object_id` if it is empty (@LasseBlaauwbroek mirage/capnp-rpc#224).
  This improves interoperability with the C++ implementation.

- Use `open_in_bin` instead of `open_in` for Windows compatibility (@MisterDA mirage/capnp-rpc#222).
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