bug#24956: 26.0.50; On Windows, setting PATH in compilation-environment has no effect

bug#24956: 26.0.50; On Windows, setting PATH in compilation-environment has no effect

[Óscar Fuentes]

emacs -Q. Evaluate:

(let ((compilation-environment (list "PATH=d:\\foo")))
  (compile "echo %PATH%"))

The output is the original contents of PATH. Until recently (June, at
least) it was possible to set PATH in compilation-environment and pass
it to child processes. On GNU/Linux, it still is.


In GNU Emacs 26.0.50.1 (x86_64-w64-mingw32)
 of 2016-10-17 built on WIN8-64-VM
Repository revision: 032a299b0ed25f067f4133c547a822b283bc4cb8
Windowing system distributor 'Microsoft Corp.', version 6.3.9600

bug#24956: 26.0.50; On Windows, setting PATH in compilation-environment has no effect

[Noam Postavsky]

tag 24956 confirmed
quit

On Wed, Nov 16, 2016 at 5:22 PM, Óscar Fuentes <ofv@wanadoo.es> wrote:
>
> emacs -Q. Evaluate:
>
> (let ((compilation-environment (list "PATH=d:\\foo")))
>   (compile "echo %PATH%"))
>
> The output is the original contents of PATH. Until recently (June, at
> least) it was possible to set PATH in compilation-environment and pass
> it to child processes. On GNU/Linux, it still is.

It seems that my change in 73f0715d "Keep w32 environment settings
internal only", had an unexpected effect on the way differently cased
environment variables are handled.

With latest master

(let ((process-environment (cons "PATH=d:\\foo" process-environment)))
  (call-process "cmd" nil '(t t) nil "/C" "echo %PATH%"))

inserts the original PATH contents, whereas

(let ((process-environment (cons "Path=d:\\foo" process-environment)))
  (call-process "cmd" nil '(t t) nil "/C" "echo %PATH%"))

inserts "d:\foo". In Emacs 25.1, or reverting the commit I mentioned,
the opposite occurs.

bug#24956: 26.0.50; On Windows, setting PATH in compilation-environment has no effect

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Thu, 17 Nov 2016 12:59:19 -0500
> Cc: 24956@debbugs.gnu.org
> 
> > (let ((compilation-environment (list "PATH=d:\\foo")))
> >   (compile "echo %PATH%"))
> >
> > The output is the original contents of PATH. Until recently (June, at
> > least) it was possible to set PATH in compilation-environment and pass
> > it to child processes. On GNU/Linux, it still is.
> 
> It seems that my change in 73f0715d "Keep w32 environment settings
> internal only", had an unexpected effect on the way differently cased
> environment variables are handled.
> 
> With latest master
> 
> (let ((process-environment (cons "PATH=d:\\foo" process-environment)))
>   (call-process "cmd" nil '(t t) nil "/C" "echo %PATH%"))
> 
> inserts the original PATH contents, whereas
> 
> (let ((process-environment (cons "Path=d:\\foo" process-environment)))
>   (call-process "cmd" nil '(t t) nil "/C" "echo %PATH%"))
> 
> inserts "d:\foo". In Emacs 25.1, or reverting the commit I mentioned,
> the opposite occurs.

I think this is because PATH doesn't exist in the original environment
inherited from the shell, only Path does.  We push PATH into the
environment only in w32.c:init_environment (and do the same trick with
ComSpec vs COMSPEC).  But since the above-mentioned change, that
function no longer affects process-environment.

So to fix this bug, we need to push PATH and COMSPEC into
process-environment, replacing their camel-case variants.

Does this make sense?

bug#24956: 26.0.50; On Windows, setting PATH in compilation-environment has no effect

[Noam Postavsky]

On Thu, Nov 17, 2016 at 3:59 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Noam Postavsky <npostavs@users.sourceforge.net>
>> Date: Thu, 17 Nov 2016 12:59:19 -0500
>> Cc: 24956@debbugs.gnu.org
>>
>> > (let ((compilation-environment (list "PATH=d:\\foo")))
>> >   (compile "echo %PATH%"))
>> >
>> > The output is the original contents of PATH. Until recently (June, at
>> > least) it was possible to set PATH in compilation-environment and pass
>> > it to child processes. On GNU/Linux, it still is.
>>
>> It seems that my change in 73f0715d "Keep w32 environment settings
>> internal only", had an unexpected effect on the way differently cased
>> environment variables are handled.
>>
>> With latest master
>>
>> (let ((process-environment (cons "PATH=d:\\foo" process-environment)))
>>   (call-process "cmd" nil '(t t) nil "/C" "echo %PATH%"))
>>
>> inserts the original PATH contents, whereas
>>
>> (let ((process-environment (cons "Path=d:\\foo" process-environment)))
>>   (call-process "cmd" nil '(t t) nil "/C" "echo %PATH%"))
>>
>> inserts "d:\foo". In Emacs 25.1, or reverting the commit I mentioned,
>> the opposite occurs.
>
> I think this is because PATH doesn't exist in the original environment
> inherited from the shell, only Path does.  We push PATH into the
> environment only in w32.c:init_environment (and do the same trick with
> ComSpec vs COMSPEC).  But since the above-mentioned change, that
> function no longer affects process-environment.
>
> So to fix this bug, we need to push PATH and COMSPEC into
> process-environment, replacing their camel-case variants.
>
> Does this make sense?

Yes. https://cygwin.com/cygwin-ug-net/using-cygwinenv.html lists a few
other variables that it upcases (at the bottom of the page), should we
do those ones as well?

bug#24956: 26.0.50; On Windows, setting PATH in compilation-environment has no effect

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Thu, 17 Nov 2016 16:27:17 -0500
> Cc: Óscar Fuentes <ofv@wanadoo.es>, 24956@debbugs.gnu.org
> 
> >> (let ((process-environment (cons "Path=d:\\foo" process-environment)))
> >>   (call-process "cmd" nil '(t t) nil "/C" "echo %PATH%"))
> >>
> >> inserts "d:\foo". In Emacs 25.1, or reverting the commit I mentioned,
> >> the opposite occurs.
> >
> > I think this is because PATH doesn't exist in the original environment
> > inherited from the shell, only Path does.  We push PATH into the
> > environment only in w32.c:init_environment (and do the same trick with
> > ComSpec vs COMSPEC).  But since the above-mentioned change, that
> > function no longer affects process-environment.
> >
> > So to fix this bug, we need to push PATH and COMSPEC into
> > process-environment, replacing their camel-case variants.
> >
> > Does this make sense?
> 
> Yes. https://cygwin.com/cygwin-ug-net/using-cygwinenv.html lists a few
> other variables that it upcases (at the bottom of the page), should we
> do those ones as well?

No, I don't think so.  (Some of those are up-cased in the original
environment on my machine, anyway.)  Let's not do something like that
without a good reason.

Thanks.

bug#24956: 26.0.50; On Windows, setting PATH in compilation-environment has no effect

[Noam Postavsky]

[-- Attachment #1: Type: text/plain, Size: 17 bytes --]

Here is a patch.

[-- Attachment #2: v1-0001-Upcase-Path-and-ComSpec-in-process-environment.patch --]
[-- Type: application/octet-stream, Size: 4061 bytes --]

From b9a9230430c2b7dd5c3c81dacc8277ad7e6cfb32 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Fri, 18 Nov 2016 16:26:53 -0500
Subject: [PATCH v1] Upcase Path and ComSpec in process-environment

Since 2016-07-18 "Keep w32 environment settings internal only", the
upcasing of environment variables "Path" and "ComSpec" occured after
initializing process-environment.  This meant that Lisp code trying to
override "PATH" environment had no effect (Bug #24956).

* src/w32.c (init_environment): Remove code upcasing "Path" and "ComSpec".
* src/callproc.c (set_initial_environment) [WINDOWSNT]: And put it here,
before adding those variables to process-environment.
---
 src/callproc.c | 39 ++++++++++++++++++++++++++++++++++-----
 src/w32.c      | 19 -------------------
 2 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/src/callproc.c b/src/callproc.c
index dc3ca4a..8736f35 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -1607,11 +1607,40 @@ set_initial_environment (void)
 {
   char **envp;
   for (envp = environ; *envp; envp++)
-    Vprocess_environment = Fcons (build_string (*envp),
-				  Vprocess_environment);
-  /* Ideally, the `copy' shouldn't be necessary, but it seems it's frequent
-     to use `delete' and friends on process-environment.  */
-  Vinitial_environment = Fcopy_sequence (Vprocess_environment);
+    {
+      Lisp_Object envstr = build_string (*envp);
+      Vinitial_environment = Fcons (envstr, Vinitial_environment);
+#ifdef WINDOWSNT
+      /* Special case: on NT, the PATH variable is actually named
+         "Path" although cmd.exe (perhaps NT itself) arranges for
+         environment variable lookup and setting to be case
+         insensitive.  However, Emacs assumes a fully case sensitive
+         environment, so we need to change "Path" to "PATH" to match
+         the expectations of various elisp packages.  We do this by
+         the sneaky method of modifying the string in the C runtime
+         environ entry.
+
+         The same applies to COMSPEC.
+
+         We want to do this here so that Vprocess_environment is
+         affected by this modification, without getting all the other
+         NT-specific modifications we do in init_environment.  */
+      const char *upcased_var = NULL;
+      if (_strnicmp (*envp, "PATH=", 5) == 0)
+        upcased_var = "PATH";
+      else if (_strnicmp (*envp, "COMSPEC=", 8) == 0)
+        upcased_var = "COMSPEC";
+
+      if (upcased_var)
+        {
+          memcpy (*envp, upcased_var, strlen (upcased_var));
+          envstr = build_string (*envp);
+        }
+#endif
+      /* `process_environment' is often modified destructively, so
+         don't share any structure initial-environment.  */
+      Vprocess_environment = Fcons (envstr, Vprocess_environment);
+    }
 }
 
 void
diff --git a/src/w32.c b/src/w32.c
index ad7d94a..d4a7d93 100644
--- a/src/w32.c
+++ b/src/w32.c
@@ -2852,25 +2852,6 @@ init_environment (char ** argv)
   /* Rebuild system configuration to reflect invoking system.  */
   Vsystem_configuration = build_string (EMACS_CONFIGURATION);
 
-  /* Another special case: on NT, the PATH variable is actually named
-     "Path" although cmd.exe (perhaps NT itself) arranges for
-     environment variable lookup and setting to be case insensitive.
-     However, Emacs assumes a fully case sensitive environment, so we
-     need to change "Path" to "PATH" to match the expectations of
-     various elisp packages.  We do this by the sneaky method of
-     modifying the string in the C runtime environ entry.
-
-     The same applies to COMSPEC.  */
-  {
-    char ** envp;
-
-    for (envp = environ; *envp; envp++)
-      if (_strnicmp (*envp, "PATH=", 5) == 0)
-	memcpy (*envp, "PATH=", 5);
-      else if (_strnicmp (*envp, "COMSPEC=", 8) == 0)
-	memcpy (*envp, "COMSPEC=", 8);
-  }
-
   /* Remember the initial working directory for getcwd.  */
   /* FIXME: Do we need to resolve possible symlinks in startup_dir?
      Does it matter anywhere in Emacs?  */
-- 
2.6.2.windows.1

bug#24956: 26.0.50; On Windows, setting PATH in compilation-environment has no effect

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Fri, 18 Nov 2016 17:11:22 -0500
> Cc: Óscar Fuentes <ofv@wanadoo.es>, 24956@debbugs.gnu.org
> 
> Here is a patch.

Thanks.

Two comments:

  . I'd rather have the WINDOWSNT-specific code separate (on w32.c),
    so perhaps the for-loop you modified could be left intact, and
    instead we perform an additional pass over Vprocess_environment,
    after it is already copied, to replace these two variables with
    their upper-case equivalents, in that special code on w32.c.

  . If you didn't already, please verify that these 2 variables in the
    Emacs's own environment are also upcased, because otherwise I
    think we will hit more such problems.

bug#24956: 26.0.50; On Windows, setting PATH in compilation-environment has no effect

[Noam Postavsky]

[-- Attachment #1: Type: text/plain, Size: 755 bytes --]

On Sat, Nov 19, 2016 at 2:16 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>
>   . I'd rather have the WINDOWSNT-specific code separate (on w32.c),
>     so perhaps the for-loop you modified could be left intact, and
>     instead we perform an additional pass over Vprocess_environment,
>     after it is already copied, to replace these two variables with
>     their upper-case equivalents, in that special code on w32.c.

Okay, here is a patch that does that.

>  . If you didn't already, please verify that these 2 variables in the
>    Emacs's own environment are also upcased, because otherwise I
>    think we will hit more such problems.

That would be the for loop just above my additions (I increased the
context so it's fully visible in the patch).

[-- Attachment #2: v2-0001-Upcase-Path-and-ComSpec-in-process-environment.patch --]
[-- Type: application/octet-stream, Size: 2099 bytes --]

From 1fe5e0dfb01a0400a90088d44f2f7fc2bd244a43 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Fri, 18 Nov 2016 16:26:53 -0500
Subject: [PATCH v2] Upcase Path and ComSpec in process-environment

Since 2016-07-18 "Keep w32 environment settings internal only", the
upcasing of environment variables "Path" and "ComSpec" occured after
initializing process-environment.  This meant that Lisp code trying to
override "PATH" environment had no effect (Bug #24956).

* src/w32.c (init_environment): Upcase the "Path" and "ComSpec" entries
in Vprocess_environment.
---
 src/w32.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/src/w32.c b/src/w32.c
index ad7d94a..0739e51 100644
--- a/src/w32.c
+++ b/src/w32.c
@@ -2867,10 +2867,28 @@ init_environment (char ** argv)
     for (envp = environ; *envp; envp++)
       if (_strnicmp (*envp, "PATH=", 5) == 0)
 	memcpy (*envp, "PATH=", 5);
       else if (_strnicmp (*envp, "COMSPEC=", 8) == 0)
 	memcpy (*envp, "COMSPEC=", 8);
+
+    /* Make the same modification to `process-environment' which has
+       already been initialized in set_initial_environment.  */
+    Lisp_Object env = Vprocess_environment;
+    Lisp_Object path = build_string ("PATH=");
+    Lisp_Object path_len = make_number (SBYTES (path));
+    Lisp_Object comspec = build_string ("COMSPEC=");
+    Lisp_Object comspec_len = make_number (SBYTES (comspec));
+    for (; CONSP (env); env = XCDR (env))
+    {
+      Lisp_Object entry = XCAR (env);
+      if (EQ (Fcompare_strings (entry, Qnil, path_len, path, Qnil, Qnil, Qt), Qt))
+        for (ptrdiff_t i = 0; i < SBYTES (path); i++)
+          SSET (entry, i, SREF (path, i));
+      else if (EQ (Fcompare_strings (entry, Qnil, comspec_len, comspec, Qnil, Qnil, Qt), Qt))
+        for (ptrdiff_t i = 0; i < SBYTES (comspec); i++)
+          SSET (entry, i, SREF (comspec, i));
+    }
   }
 
   /* Remember the initial working directory for getcwd.  */
   /* FIXME: Do we need to resolve possible symlinks in startup_dir?
      Does it matter anywhere in Emacs?  */
-- 
2.6.2.windows.1

bug#24956: 26.0.50; On Windows, setting PATH in compilation-environment has no effect

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Mon, 21 Nov 2016 18:19:53 -0500
> Cc: Óscar Fuentes <ofv@wanadoo.es>, 24956@debbugs.gnu.org
> 
> On Sat, Nov 19, 2016 at 2:16 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> >
> >   . I'd rather have the WINDOWSNT-specific code separate (on w32.c),
> >     so perhaps the for-loop you modified could be left intact, and
> >     instead we perform an additional pass over Vprocess_environment,
> >     after it is already copied, to replace these two variables with
> >     their upper-case equivalents, in that special code on w32.c.
> 
> Okay, here is a patch that does that.

Thanks.

> +    /* Make the same modification to `process-environment' which has
> +       already been initialized in set_initial_environment.  */
> +    Lisp_Object env = Vprocess_environment;
> +    Lisp_Object path = build_string ("PATH=");
> +    Lisp_Object path_len = make_number (SBYTES (path));
> +    Lisp_Object comspec = build_string ("COMSPEC=");
> +    Lisp_Object comspec_len = make_number (SBYTES (comspec));
> +    for (; CONSP (env); env = XCDR (env))
> +    {
> +      Lisp_Object entry = XCAR (env);
> +      if (EQ (Fcompare_strings (entry, Qnil, path_len, path, Qnil, Qnil, Qt), Qt))
> +        for (ptrdiff_t i = 0; i < SBYTES (path); i++)
> +          SSET (entry, i, SREF (path, i));
> +      else if (EQ (Fcompare_strings (entry, Qnil, comspec_len, comspec, Qnil, Qnil, Qt), Qt))
> +        for (ptrdiff_t i = 0; i < SBYTES (comspec); i++)
> +          SSET (entry, i, SREF (comspec, i));
> +    }

Why not compare in using _strnicmp instead of Fcompare_strings?  That
would save you the need to cons Lisp strings, and will be more
efficient (Fcompare_strings is quite complex).  Not that speed matters
in this case, but it's just looks strange to me.  Am I missing
something?

bug#24956: 26.0.50; On Windows, setting PATH in compilation-environment has no effect

[Noam Postavsky]

[-- Attachment #1: Type: text/plain, Size: 635 bytes --]

On Tue, Nov 22, 2016 at 11:01 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>
> Why not compare in using _strnicmp instead of Fcompare_strings?  That
> would save you the need to cons Lisp strings, and will be more
> efficient (Fcompare_strings is quite complex).  Not that speed matters
> in this case, but it's just looks strange to me.  Am I missing
> something?

Oops. Looking at Fupcase, I had somehow thought that unibyte vs
multibyte would be a problem (and hence I should use Fcompare_strings
to hide the details), but actually (as I think you've mentioned
before) that's not the case for environment variables.

Here's a new patch.

[-- Attachment #2: v3-0001-Upcase-Path-and-ComSpec-in-process-environment.patch --]
[-- Type: application/octet-stream, Size: 2108 bytes --]

From ecb8a7ea028235e67a955e63c5ee1ca5d574b04f Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Fri, 18 Nov 2016 16:26:53 -0500
Subject: [PATCH v3] Upcase Path and ComSpec in process-environment

Since 2016-07-18 "Keep w32 environment settings internal only", the
upcasing of environment variables "Path" and "ComSpec" occured after
initializing process-environment.  This meant that Lisp code trying to
override "PATH" environment had no effect (Bug #24956).

* src/w32.c (init_environment): Upcase the "Path" and "ComSpec" entries
in Vprocess_environment.
---
 src/w32.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/src/w32.c b/src/w32.c
index ad7d94a..086c1ac 100644
--- a/src/w32.c
+++ b/src/w32.c
@@ -2863,12 +2863,29 @@ init_environment (char ** argv)
      The same applies to COMSPEC.  */
   {
     char ** envp;
+    const char *path = "PATH=";
+    int path_len = strlen (path);
+    const char *comspec = "COMSPEC=";
+    int comspec_len = strlen (comspec);
 
     for (envp = environ; *envp; envp++)
-      if (_strnicmp (*envp, "PATH=", 5) == 0)
-	memcpy (*envp, "PATH=", 5);
-      else if (_strnicmp (*envp, "COMSPEC=", 8) == 0)
-	memcpy (*envp, "COMSPEC=", 8);
+      if (_strnicmp (*envp, path, path_len) == 0)
+        memcpy (*envp, path, path_len);
+      else if (_strnicmp (*envp, comspec, comspec_len) == 0)
+        memcpy (*envp, comspec, comspec_len);
+
+    /* Make the same modification to `process-environment' which has
+       already been initialized in set_initial_environment.  */
+    for (Lisp_Object env = Vprocess_environment; CONSP (env); env = XCDR (env))
+    {
+      Lisp_Object entry = XCAR (env);
+      if (_strnicmp (SDATA (entry), path, path_len) == 0)
+        for (int i = 0; i < path_len; i++)
+          SSET (entry, i, path[i]);
+      else if (_strnicmp (SDATA (entry), comspec, comspec_len) == 0)
+        for (int i = 0; i < comspec_len; i++)
+          SSET (entry, i, comspec[i]);
+    }
   }
 
   /* Remember the initial working directory for getcwd.  */
-- 
2.6.2.windows.1

bug#24956: 26.0.50; On Windows, setting PATH in compilation-environment has no effect

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Tue, 22 Nov 2016 17:35:17 -0500
> Cc: Óscar Fuentes <ofv@wanadoo.es>, 24956@debbugs.gnu.org
> 
> Oops. Looking at Fupcase, I had somehow thought that unibyte vs
> multibyte would be a problem (and hence I should use Fcompare_strings
> to hide the details), but actually (as I think you've mentioned
> before) that's not the case for environment variables.
> 
> Here's a new patch.

Looks OK to me, thanks.

bug#24956: 26.0.50; On Windows, setting PATH in compilation-environment has no effect

[Óscar Fuentes]

Eli Zaretskii <eliz@gnu.org> writes:

>> Oops. Looking at Fupcase, I had somehow thought that unibyte vs
>> multibyte would be a problem (and hence I should use Fcompare_strings
>> to hide the details), but actually (as I think you've mentioned
>> before) that's not the case for environment variables.
>> 
>> Here's a new patch.
>
> Looks OK to me, thanks.

Ping.

I'll like to mark this as resolved. Noam?

bug#24956: 26.0.50; On Windows, setting PATH in compilation-environment has no effect

[npostavs]

Óscar Fuentes <ofv@wanadoo.es> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Oops. Looking at Fupcase, I had somehow thought that unibyte vs
>>> multibyte would be a problem (and hence I should use Fcompare_strings
>>> to hide the details), but actually (as I think you've mentioned
>>> before) that's not the case for environment variables.
>>> 
>>> Here's a new patch.
>>
>> Looks OK to me, thanks.
>
> Ping.
>
> I'll like to mark this as resolved. Noam?

I'm not in front of my Windows box right now, I'm planning to push this tomorrow.

bug#24956: 26.0.50; On Windows, setting PATH in compilation-environment has no effect

[Noam Postavsky]

tag 24956 fixed
close 24956
quit

On Sun, Nov 27, 2016 at 3:13 PM,  <npostavs@users.sourceforge.net> wrote:
> Óscar Fuentes <ofv@wanadoo.es> writes:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> Oops. Looking at Fupcase, I had somehow thought that unibyte vs
>>>> multibyte would be a problem (and hence I should use Fcompare_strings
>>>> to hide the details), but actually (as I think you've mentioned
>>>> before) that's not the case for environment variables.
>>>>
>>>> Here's a new patch.
>>>
>>> Looks OK to me, thanks.
>>
>> Ping.
>>
>> I'll like to mark this as resolved. Noam?
>
> I'm not in front of my Windows box right now, I'm planning to push this tomorrow.

Pushed as 753c565df6 "Upcase Path and ComSpec in process-environment".