Allow command to be a list in async version

[garyo]

The async command I need to issue to get a proper Windows path is a bit complex, and rather than deal with quoting issues I thought it would be easier to allow the command to be a list. This works for me:

% git diff
diff --git a/system-environment.el b/system-environment.el
index 647bf17..a62db89 100644
--- a/system-environment.el
+++ b/system-environment.el
@@ -1,3 +1,5 @@
+;; -*- lexical-binding: nil; -*-
+
 ;; for string-trim-right, required for unclean outputs (Windows...)
 (require 'subr-x)

@@ -44,7 +46,7 @@ Import PATH and SSH_AUTH_SOCK from zsh with verbose logging:
         (set-buffer (get-buffer-create buffer-name))
         (shell-command command
                        buffer-name
-                       "*Messages*")
+                       "*environment-import-stderr*")
         (system-environment-import-from-buffer "*environment-import*" variable-names is-blacklist verbose)
         (run-hooks 'system-environment-import-hook)
         (when (not keep-buffer) (kill-buffer buffer-name)))
@@ -70,9 +72,9 @@ specified to make sure both calls dump into unique buffers.
 (system-environment-import-from-async-command
  \"powershell -Command Get-ChildItem Env:|Foreach-Object {'{0}={1}' -f $_.Key,$_.Value }\")
 "
-  (if (executable-find (car (split-string command)))
+  (if (executable-find (if (listp command) (car command) (car (split-string command))))
       (let ((buffer-name (or buffer "*environment-import*"))
-            (command-list (split-string command)))
+            (command-list (if (listp command) command (split-string command))))
         (push buffer-name command-list)
         (push (nth 1 command-list) command-list)
         (set-buffer (get-buffer-create buffer-name))

Note that I also had to explicitly set lexical-binding to nil, otherwise I get variable undefined errors in the sentinel where it uses the free vars.

In case you're interested in my command to get a proper PATH on Windows, here's what I'm doing:

 (system-environment-import-from-async-command
              '("zsh" "-i" "-c" "echo PATH=$(cygpath -p -m $PATH)")
              '("PATH")
              nil keep-buffer verbose "*system-environment-PATH*"))

That converts the msys zsh path to Windows (with forward slashes), so "/usr/bin" turns into "c:/msys64/usr/bin" and so on, and it uses semicolon as the path separator (Windows-style) so it's correct for (non-cygwin) emacs. Later in the hook to update exec-path I handle the semicolon separator like this:

  (defun gco-update-exec-path-from-PATH ()
    "Update emacs's exec-path from PATH env var. Tries to determine
    path separator by looking for Windows drive letter. Used as a hook for system-environment."
    (let* ((path (getenv "PATH"))
           (pathsep (if (string-match-p "\\b[cdCD]:[\\/]" path) ";" ":")))
      (setq exec-path (seq-uniq
                       (append (split-string (getenv "PATH") pathsep) exec-path)
                       'string=))))

[bwachter]

Note that I also had to explicitly set lexical-binding to nil, otherwise I get variable undefined errors in the sentinel where it uses the free vars.

Which variables exactly is it complaining about? I'm a bit surprised by that - unless I missed something the sentinel should use only variables local to the output buffer, which should be available.

In case you're interested in my command to get a proper PATH on Windows, here's what I'm doing:

I'd add that as additional example to the documentation, if you don't mind. I'm using a native build of Emacs on my Windows machine nowadays, so I never looked into those path translations.

[bwachter]

Also, if I read the documentation correctly, lexical-binding should always be nil, unless you explicitly set it. Which emacs version are you using?

[garyo]

I always run a pretty recent master emacs with everything turned on. My emacs is native too, but my shell is msys64 zsh, hence the path translation. (I do set some vars in Windows registry but only a subset of what I actually use day to day.) Note that if you run your powershell example, the PATH you get back will be semicolon-separated (which is good because it'll have colons in the pathnames), so your hook to update exec-path will have to be modified to deal with that. My version above is a terrible hack, but it's good enough for me. :-)

The variable it typically complains about (and errors out) is variable-names. I see you're doing setq-local which should work, so I'm not sure why I see the errors actually. I agree, you shouldn't have to set lexical-binding to nil -- I could try it without that; maybe something else I did fixed it. (I was making two async calls before I switched one to sync; maybe that was confusing things?)

And yes you are welcome to add my example.

[bwachter]

I always run a pretty recent master emacs with everything turned on

I'm on 29, but I did package various emacs versions to be installed in parallel. I've just kicked the emacs30 build, so I'll have one tomorrow to test as well. I couldn't reproduce that variable issue on my version at least.

Note that if you run your powershell example, the PATH you get back will be semicolon-separated (which is good because it'll have colons in the pathnames), so your hook to update exec-path will have to be modified to deal with that.

I actually may have that, I'll need to check - I just did a quick check that everything works after splitting my ancient code out into this. I just have the Windows system due to a project requiring me to do some work on Arm64 Windows (which Emacs doesn't compile for natively), so it's all a very special kind of broken. I'll update that bit in the documentation, though.

I see you're doing setq-local which should work, so I'm not sure why I see the errors actually. I agree, you shouldn't have to set lexical-binding to nil

Could you remove it from the pull request for now? The rest looks good to merge. I don't think having that there should have any negative effect, as it's effectively just explicitly setting the default behaviour, but I'd rather not merge something I don't understand.

[garyo]

Will do. One other small thing: in my hook, I switched the order of exec-path to put my path first, then the default exec-path. I think it's best that way, since the exec-path doc says this:

By default the last element of this list is ‘exec-directory’. The
last element is not always used, for example in shell completion
(‘shell-dynamic-complete-command’).

[bwachter]

Will do.

Thanks, I've just merged it.

One other small thing: in my hook, I switched the order of exec-path to put my path first, then the default exec-path. I think it's best that way, since the exec-path doc says this:

That's a good point, thanks. I'll update that tomorrow.

I'm closing this issue now, if you do have more variable errors please open an issue for that - I'll also see if I can reproduce that myself with emacs30 tomorrow.