Unix.environment on Windows: use _wenviron #1369

nojb · 2017-09-25T16:41:15Z

One more rebase fix following #1200.

The existing Unix.environment in trunk uses the native Win32 API GetEnvironmentStrings which works OK except that its result can sometimes get out of sync with the result of the "POSIX" API _wgetenv used for Unix.{get,put}env (see MPR#4499 for details).

The mixup ocurred because at some point we had switched to using the native Win32 API in all three functions but later decided to leave that change for later.

This patch reverts Unix.environment to read from the global variable _wenviron, similar to the analogous implementation for Unix.

alainfrisch · 2017-09-25T16:50:56Z

otherlibs/win32unix/envir.c

@@ -21,23 +21,26 @@
 #include <caml/osdeps.h>

 #include <Windows.h>
+#include <Stdlib.h>


stdlib (lowercase) is more standard (also for windows.h, btw) in our codebase.

FWIW, an quick check in the museum shows that Microsoft use Windows.h and stdlib.h (or, of course, STDLIB.H if you're very, very old...)

However, the MinGW headers use windows.h so it's anyone's guess really... let's hope no one is crazy enough to try running the kernel in case sensitive mode 😄

alainfrisch · 2017-09-25T16:52:34Z

otherlibs/win32unix/envir.c

-    v = caml_copy_string_of_utf16(p);
-    Store_field(result, i ++, v);
+  if (_wenviron != NULL) {
+    nbr = 0;


Cannot one use caml_alloc_array as caml_copy_string_array does (passing caml_copy_string_of_utf16 instead)?

alainfrisch · 2017-09-25T16:53:26Z

Could you create a test that would fail before this PR is merged?

nojb · 2017-09-26T09:03:27Z

The point of the PR is that it should be possible - except that today I can't seem to reproduce the failure locally, mmmph... Will investigate more, see if I can figure out what is going on.

In any case, it seems weird to use the native Win32 API for just one of the calls instead of all three.

alainfrisch · 2017-09-26T09:20:53Z

At least for the ANSI version, my understanding of MPR4499 is that putenv indeed sends the new value to the "real "environment" (the one manipulated through GetEnvironmentVariable and co). If this is also the case for the W version of putenv, everything would be ok (although I understand it is not very nice to mix the two kinds of functions). It would actually be a bit "better" since this would allow to see changes performed through SetEnvironmentVariable (e.g. from .NET). But yes, it would actually be confusing to see changes that are invisible to getenv...

dra27 · 2017-09-27T09:43:26Z

Ugh, it's one of many ugly sections of the Microsoft C runtime! It feels somewhat icky because _environ and _wenviron are deprecated (however, MSVC also provides no enumeration functions, as far as I'm aware, so go figure!).

OCaml is supposed to sit atop a C runtime, even if it's an often irritating one, so I agree that correcting this one function is the way to go.

nojb · 2017-09-27T21:19:51Z

Do I need to add an entry to Changes?

alainfrisch · 2017-09-27T21:24:53Z

Do I need to add an entry to Changes?

Perhaps mention it together with the entry for #1200.

nojb · 2017-09-27T21:26:56Z

Ah yes, of course!

olleolleolle · 2017-09-29T14:11:11Z

otherlibs/win32unix/envir.c

-  wchar_t * envp, * p;
-  int size, i;
+  CAMLparam0 ();
+  CAMLlocal2 (v, result);


Hi!

Are the spaces after the function names introduced by design?

It's on line 28, 29, 36.

Thanks for the comment; and not, not really. Will fix it and give AppVeyor an excuse to wake up again.

xavierleroy · 2017-10-03T18:11:18Z

@alainfrisch suggested a possibly simpler implementation. Please tell why it would not work.

nojb · 2017-10-04T03:53:17Z

Yes, sorry about that, I forgot to post an answer here. The reason was that caml_copy_array is not "polymorphic" enough, it has signature

value caml_alloc_array(value (*funct)(char const *), char const ** arr)

(we need a version with wchar_t in lieu of char). It is tempting to change char * to void * in this signature, but then the funct argument will have type value (*funct)(void const *) and will not accept something less general like value caml_copy_string(char const *).

Anyone see a way around this?

nojb · 2017-10-04T03:55:27Z

Of course I can just cast the argument to use it (in fact I already used this once: https://github.com/ocaml/ocaml/blob/trunk/byterun/sys.c#L362). Shall I do that?

nojb · 2017-10-05T09:14:55Z

OK, absent any explicit decision I went ahead and added a commit using caml_alloc_array (with a cast), which looks simpler.

gasche · 2017-10-05T09:22:17Z

Is it possible/practical to add a regression test for the bug you are fixing?

nojb · 2017-10-05T11:17:13Z

Yes, it is! I added a test (test_env.ml) which fails on trunk and passes on this branch. I also added a second (disabled) test test_env2.ml which fails due to the closely related MPR#4499. I am preparing a PR for after 4.06 to fix the second one.

gasche · 2017-10-05T14:18:14Z

Is someone willing to review and approve the PR? ( @alainfrisch, @dra27 ? )

alainfrisch · 2017-10-10T11:06:15Z

testsuite/tests/win-unicode/test_env2.ml

+let foo = "FOO"
+
+let () =
+  set_environment_variable foo "BAR";


It's good to have this test (disabled), but perhaps indicates somehow that it is related to MPR#4499, not the current PR.

Also, it might be good to use non-ASCII characters in the string.

alainfrisch · 2017-10-10T11:07:18Z

LGTM (modulo a minor comment about the disabled test). Waiting for the CI green lights.

nojb · 2017-10-10T12:03:18Z

Rebased and amended as suggested:

Added a test to check non-ASCII characters being returned by Unix.environment to win-unicode/mltest.ml (Unix.{getenv,putenv} were already being tested)
Added a new test lib-unix/win-env/test_env.ml for this PR (fails on trunk)
Added a new test lib-unix/win-env/test_env2.ml, currently disabled (with a comment explaining why), that tests MPR#4499 which is closely related to the present PR.

- Add lib-unix/win-env/test_env.ml, a test that fails in trunk and passes in this PR. - Add lib-unix/win-env/test_env2.ml: a test for MPR#4499 which is closely related to this PR. Currently disabled as it fails. - Added a new test in win-unicode/mltest.ml to test non-ASCII characters returned by Unix.environment (Unix.{getenv,putenv} were already being tested).

gasche · 2017-10-10T14:34:12Z

( @alainfrisch does your comment count as approval? Let's follow the process as it was suggested. )

alainfrisch · 2017-10-10T14:51:33Z

@damiendoligez @gasche I'd like to cherry-pick this to 4.06. Do you agree?

gasche · 2017-10-10T15:11:45Z

Yes, I think it's good to have it in the first release with #1200. I would encourage you to include it in 4.06.

* Unix.environment on Windows: use _wenviron * Add Changes entry * Update testsuite - Add lib-unix/win-env/test_env.ml, a test that fails in trunk and passes in this PR. - Add lib-unix/win-env/test_env2.ml: a test for MPR#4499 which is closely related to this PR. Currently disabled as it fails. - Added a new test in win-unicode/mltest.ml to test non-ASCII characters returned by Unix.environment (Unix.{getenv,putenv} were already being tested).

alainfrisch · 2017-10-10T16:53:30Z

Cherry-picked to 4.06 as 990845b.

alainfrisch reviewed Sep 25, 2017

View reviewed changes

damiendoligez added this to the 4.06.0 milestone Sep 27, 2017

gasche closed this Sep 27, 2017

gasche reopened this Sep 27, 2017

nojb force-pushed the fix_unix_wenvironment branch from 986ad5f to 6b4326f Compare September 27, 2017 21:26

olleolleolle reviewed Sep 29, 2017

View reviewed changes

nojb force-pushed the fix_unix_wenvironment branch 2 times, most recently from 674328c to 7822d54 Compare September 29, 2017 14:20

damiendoligez added the no-change-entry-needed label Sep 29, 2017

nojb force-pushed the fix_unix_wenvironment branch 2 times, most recently from 6b4326f to 652c6f5 Compare September 30, 2017 06:44

dra27 closed this Sep 30, 2017

dra27 reopened this Sep 30, 2017

dra27 closed this Oct 1, 2017

dra27 reopened this Oct 1, 2017

nojb force-pushed the fix_unix_wenvironment branch from 652c6f5 to 5af73a7 Compare October 4, 2017 03:56

nojb force-pushed the fix_unix_wenvironment branch 2 times, most recently from feda74a to 37ae174 Compare October 10, 2017 10:42

alainfrisch reviewed Oct 10, 2017

View reviewed changes

Unix.environment on Windows: use _wenviron

6c657bb

nojb force-pushed the fix_unix_wenvironment branch from 37ae174 to 1d9079a Compare October 10, 2017 11:59

nojb added 2 commits October 10, 2017 14:46

Add Changes entry

37e7fe1

nojb force-pushed the fix_unix_wenvironment branch from 1d9079a to 37e7fe1 Compare October 10, 2017 12:46

alainfrisch merged commit f32374d into ocaml:trunk Oct 10, 2017

EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024

Promote the changelog to non-experimental (ocaml#1369)

cf6d98f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unix.environment on Windows: use _wenviron #1369

Unix.environment on Windows: use _wenviron #1369

nojb commented Sep 25, 2017

alainfrisch Sep 25, 2017

dra27 Sep 27, 2017

alainfrisch Sep 25, 2017

alainfrisch commented Sep 25, 2017

nojb commented Sep 26, 2017

alainfrisch commented Sep 26, 2017

dra27 commented Sep 27, 2017

nojb commented Sep 27, 2017

alainfrisch commented Sep 27, 2017

nojb commented Sep 27, 2017

olleolleolle Sep 29, 2017

nojb Sep 29, 2017

xavierleroy commented Oct 3, 2017

nojb commented Oct 4, 2017

nojb commented Oct 4, 2017

nojb commented Oct 5, 2017

gasche commented Oct 5, 2017

nojb commented Oct 5, 2017

gasche commented Oct 5, 2017

alainfrisch Oct 10, 2017

alainfrisch commented Oct 10, 2017

nojb commented Oct 10, 2017

gasche commented Oct 10, 2017

alainfrisch commented Oct 10, 2017

gasche commented Oct 10, 2017

alainfrisch commented Oct 10, 2017

Unix.environment on Windows: use _wenviron #1369

Unix.environment on Windows: use _wenviron #1369

Conversation

nojb commented Sep 25, 2017

alainfrisch Sep 25, 2017

Choose a reason for hiding this comment

dra27 Sep 27, 2017

Choose a reason for hiding this comment

alainfrisch Sep 25, 2017

Choose a reason for hiding this comment

alainfrisch commented Sep 25, 2017

nojb commented Sep 26, 2017

alainfrisch commented Sep 26, 2017

dra27 commented Sep 27, 2017

nojb commented Sep 27, 2017

alainfrisch commented Sep 27, 2017

nojb commented Sep 27, 2017

olleolleolle Sep 29, 2017

Choose a reason for hiding this comment

nojb Sep 29, 2017

Choose a reason for hiding this comment

xavierleroy commented Oct 3, 2017

nojb commented Oct 4, 2017

nojb commented Oct 4, 2017

nojb commented Oct 5, 2017

gasche commented Oct 5, 2017

nojb commented Oct 5, 2017

gasche commented Oct 5, 2017

alainfrisch Oct 10, 2017

Choose a reason for hiding this comment

alainfrisch commented Oct 10, 2017

nojb commented Oct 10, 2017

gasche commented Oct 10, 2017

alainfrisch commented Oct 10, 2017

gasche commented Oct 10, 2017

alainfrisch commented Oct 10, 2017