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.

Sign up for GitHub

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

Jump to bottom

Use Windows Unicode API in C stubs #843

Merged
merged 1 commit into from
Nov 8, 2021
Merged

Use Windows Unicode API in C stubs #843

merged 1 commit into from
Nov 8, 2021

Conversation

MisterDA
Copy link
Contributor

@MisterDA MisterDA commented Mar 4, 2021

This was discussed a bit in #790.

I looked for windows functions in src/unix/{,windows_c}/*.c.

Two possible changes to this patch:

  1. define the macros inside the C files;
  2. use the W suffix to CreateProcess.

@raphael-proust
Copy link
Collaborator

I don't have a windows machine to try this on. Anyone has one?

Alternatively, I can merge the Github Actions PR first so we have windows in the CI.

@dra27
Copy link
Contributor

dra27 commented Mar 5, 2021

2. use the W suffix to CreateProcess.

This part isn't strictly necessary - defining UNICODE causes the Windows API to #define the correct function for CreateProcess.

@MisterDA
Copy link
Contributor Author

MisterDA commented Mar 8, 2021

  1. use the W suffix to CreateProcess.

This part isn't strictly necessary - defining UNICODE causes the Windows API to #define the correct function for CreateProcess.

Yes, I was suggesting it for the sake of explicitness, considering that I'm removing the comment below, in which we could also have explicitly called CreateProcessA.

/* The cast to LPSTR below is only legitimate because we are calling
  CreateProcessA. See https://github.com/ocsigen/lwt/pull/790. */

if (!CreateProcess(NULL, (LPSTR)String_val(cmdline), NULL, NULL, TRUE, 0,

@MisterDA
Copy link
Contributor Author

I noticed that this patch requires OCaml >= 4.06, so changed that on Windows.
I believe it is good to go, but it means that Lwt with OCaml >= 4.02 & < 4.06 isn't buildable on Windows anymore.

@MisterDA MisterDA mentioned this pull request Sep 17, 2021
Merged
@MisterDA MisterDA force-pushed the windows-unicode branch 3 times, most recently from fa5551f to 099acb5 Compare September 27, 2021 14:03
@MisterDA MisterDA force-pushed the windows-unicode branch 3 times, most recently from 0c1dd3c to 7989c85 Compare October 4, 2021 16:18
@MisterDA
Copy link
Contributor Author

MisterDA commented Oct 4, 2021 •
edited
Loading

Sorry for all the force-pushes, it's a bit late in the day. Can confirm that this PR works now, it was nicely broken before.
EDIT: rebased to get the green CI (even though the CI isn't testing the Windows code atm!)

@smorimoto
Copy link
Member

I'm just starting to think we should test on all, even on Windows.

@MisterDA
Copy link
Contributor Author

I'm just starting to think we should test on all, even on Windows.

At least the oldest and the newest. I'm interested by the newest, where all the cool bug fixes go!
I don't think it's very interesting to keep compatibility with OCaml < 4.06 versions on Windows.

@smorimoto
Copy link
Member

Certainly, testing the oldest and newest ones sounds like a good idea.

@smorimoto
Copy link
Member

I just merged #896

@dra27
Copy link
Contributor

dra27 commented Oct 15, 2021

I just merged #896

So this PR now needs to update the test matrix to test 4.06 on Windows instead of 4.02

@MisterDA MisterDA force-pushed the windows-unicode branch 3 times, most recently from 8583dff to 1a55ce2 Compare October 15, 2021 15:31
@smorimoto
Copy link
Member

Just needs a CHANGES entry.

@MisterDA
Copy link
Contributor Author

MisterDA commented Nov 7, 2021

Rebased and added an entry.

@smorimoto
Copy link
Member

Great thank you

@smorimoto smorimoto merged commit 461d808 into ocsigen:master Nov 8, 2021
@MisterDA MisterDA deleted the windows-unicode branch November 10, 2021 09:04
MisterDA added a commit to MisterDA/lwt that referenced this pull request Nov 18, 2021
@MisterDA
CreateProcess needs CREATE_UNICODE_ENVIRONMENT
99468c5 
> If this flag is set, the environment block pointed to by
> lpEnvironment uses Unicode characters. Otherwise, the environment
> block uses ANSI characters.

[doc][]

This was forgotten from PR ocsigen#843 that switched to Unicode strings on
Windows (including the environment).

[doc]: https://docs.microsoft.com/en-us/windows/win32/procthread/process-creation-flags#CREATE_UNICODE_ENVIRONMENT
MisterDA added a commit to MisterDA/lwt that referenced this pull request Nov 19, 2021
@MisterDA
CreateProcess needs CREATE_UNICODE_ENVIRONMENT
518ed27 
> If the environment block pointed to by lpEnvironment contains
> Unicode characters, be sure that dwCreationFlags includes
> CREATE_UNICODE_ENVIRONMENT. If this parameter is NULL and the
> environment block of the parent process contains Unicode characters,
> you must also ensure that dwCreationFlags includes
> CREATE_UNICODE_ENVIRONMENT.

From [doc1][].

> If this flag is set, the environment block pointed to by
> lpEnvironment uses Unicode characters. Otherwise, the environment
> block uses ANSI characters.

From [doc2][].

This was forgotten from PR ocsigen#843 that switched to Unicode strings on
Windows (including the environment).

[doc1]: https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw
[doc2]: https://docs.microsoft.com/en-us/windows/win32/procthread/process-creation-flags#CREATE_UNICODE_ENVIRONMENT
MisterDA added a commit to MisterDA/lwt that referenced this pull request Nov 19, 2021
@MisterDA
CreateProcess needs CREATE_UNICODE_ENVIRONMENT
a9d7cfe 
> If the environment block pointed to by lpEnvironment contains
> Unicode characters, be sure that dwCreationFlags includes
> CREATE_UNICODE_ENVIRONMENT. If this parameter is NULL and the
> environment block of the parent process contains Unicode characters,
> you must also ensure that dwCreationFlags includes
> CREATE_UNICODE_ENVIRONMENT.

From [doc1][].

> If this flag is set, the environment block pointed to by
> lpEnvironment uses Unicode characters. Otherwise, the environment
> block uses ANSI characters.

From [doc2][].

This was forgotten from PR ocsigen#843 that switched to Unicode strings on
Windows (including the environment).

[doc1]: https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw
[doc2]: https://docs.microsoft.com/en-us/windows/win32/procthread/process-creation-flags#CREATE_UNICODE_ENVIRONMENT
@MisterDA MisterDA mentioned this pull request Nov 19, 2021
Merged
smorimoto pushed a commit to raphael-proust/lwt that referenced this pull request Dec 2, 2021
@MisterDA @smorimoto
CreateProcess needs CREATE_UNICODE_ENVIRONMENT
0e2eb96 
> If the environment block pointed to by lpEnvironment contains
> Unicode characters, be sure that dwCreationFlags includes
> CREATE_UNICODE_ENVIRONMENT. If this parameter is NULL and the
> environment block of the parent process contains Unicode characters,
> you must also ensure that dwCreationFlags includes
> CREATE_UNICODE_ENVIRONMENT.

From [doc1][].

> If this flag is set, the environment block pointed to by
> lpEnvironment uses Unicode characters. Otherwise, the environment
> block uses ANSI characters.

From [doc2][].

This was forgotten from PR ocsigen#843 that switched to Unicode strings on
Windows (including the environment).

[doc1]: https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw
[doc2]: https://docs.microsoft.com/en-us/windows/win32/procthread/process-creation-flags#CREATE_UNICODE_ENVIRONMENT
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smorimoto smorimoto smorimoto approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MisterDA @raphael-proust @dra27 @smorimoto