Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

gptel-curl: don't try converting CR-LF on Windows #456

Merged
merged 1 commit into from
Nov 3, 2024

Conversation

ssafar
Copy link
Contributor

@ssafar ssafar commented Nov 2, 2024

We use curl's header size output to goto-char to the end of the response header (to parse the JSON that starts there).

On Windows, however, processes we launch will typically output Windows-style line endings, with CR-LF; by default, Emacs tries to convert this to just CR, to avoid seeing ugly ^M characters at the end of lines.

curl dumps what it gets from the server directly... but HTTP also specifies
CR-LF line endings, at least at the end of headers. On Windows, Emacs converts
these to CRs, removing the LFs, and thus causing our header size measurement to
be slightly off (the buffer now includes fewer characters of header vs. what
curl told us we'd have). So goto-char seeks too much ahead, missing the
beginning of the JSON; we get a JSON parse error.

This PR fixes this by forcing the process coding system to the UNIX variant (with no CR-LF conversion).

We gate this behind an "is this Windows" check so it shouldn't affect other systems.

Fixes #455.

We use curl's header size output to `goto-char` to the end of the response
header (to parse the JSON that starts there).

On Windows, however, processes we launch will typically output Windows-style
line endings, with CR-LF; by default, Emacs tries to convert this to just CR,
to avoid seeing ugly `^M` characters at the end of lines.

 `curl` dumps what it gets from the server directly... but HTTP also specifies
 CR-LF line endings, at least at the end of headers. On Windows, Emacs converts
 these to CRs, removing the LFs, and thus causing our header size measurement to
 be slightly off (the buffer now includes fewer characters of header vs. what
 `curl` told us we'd have). So `goto-char` seeks too much ahead, missing the
 beginning of the JSON; we get a JSON parse error.

This PR fixes this by forcing the process coding system to the UNIX
variant (with no CR-LF conversion).

We gate this behind an "is this Windows" check so it shouldn't affect other
systems.

Fixes karthink#455.
@karthink
Copy link
Owner

karthink commented Nov 3, 2024

Thanks. Do you think we need the memq check? It seems like we can set this unconditionally.

@ssafar
Copy link
Contributor Author

ssafar commented Nov 3, 2024

Just checked on linux, it's (utf-8-unix utf-8-unix) by default already, so it should be fine... but then the two windows systems had (utf-8-dos utf-8-unix) and (iso-latin-2-dos iso-latin-2-unix) respectively, so maybe there are other unix ones that have non-obvious defaults? Thus being conservative with this (... we're surely not breaking anyone else this way & we're making windows strictly better).

@karthink karthink merged commit b7ce74b into karthink:master Nov 3, 2024
@karthink
Copy link
Owner

karthink commented Nov 3, 2024

Very well, let's see if this fixes some other bugs (i.e. not json-read-error) users are facing with gptel+Curl on Windows.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Windows] Unsupported map type ‘symbol’: json-read-error (due to CR-LF issues)
2 participants