wanted: RUN-PROGRAM :DIRECTORY argument

Bug #791800 reported by Nikodemus Siivola on 2011-06-02
This bug affects 1 person
Affects Status Importance Assigned to Milestone

Bug Description

RUN-PROGRAM should have a DIRECTORY argument for specifying a working directory for the child process.

Matthias Benkard (mulk) wrote :


I've written a small patch that adds a DIRECTORY keyword argument to RUN-PROCESS. I don't have a Winows machine handy, though. Could someone check whether it works there?

Stas Boukarev (stassats) on 2013-04-16
tags: added: review
Matthias Benkard (mulk) wrote :

I've added a NEWS entry to the patch (as in lp#457053).

Paul Khuong (pvk) wrote :

Can you think of some unit tests to add (see tests/run-program.impure.lisp for inspiration)?

Matthias Benkard (mulk) wrote :

Yes, that should be relatively straight-forward, but they will be OS-dependent. I'll try adding some.

Matthias Benkard (mulk) wrote :

This should be a good test. I can't really think of any others (other than using different values for DIRECTORY).

(with-test (:name (:run-program :set-directory))
  (let* ((directory #-win32 "/"
                    #+win32 "c:\\")
         (pwd (sb-ext:run-program #-win32 "/bin/sh"
                                  #-win32 '("-c" "pwd")
                                  #+win32 "c:\\windows\\system32\\cmd.exe"
                                  #+win32 '("/c" "cd")
                                  :output :stream
                                  :directory directory)))
         (assert (string= (read-line (sb-ext:process-output pwd)) directory))
      (sb-ext:process-close pwd))))

Again, I'm not sure about the Windows version.

Matthias Benkard (mulk) wrote :

Shall I add the test to the existing patch, or put it in a new one?

It'll be applied as a single patch, so a new patch would be slightly more convenient. Let's see if we can find someone with sbcl/win32 to test this…

Matthias Benkard (mulk) wrote :

So a single updated patch would be preferable? Here it is.

Stas Boukarev (stassats) wrote :

It works on windows, except for the test, here's a working version:
(with-test (:name (:run-program :set-directory))
  (let* ((directory #-win32 "/"
                    #+win32 "c:\\")
         (out (sb-ext:process-output
               (sb-ext:run-program #-win32 "/bin/sh"
                                   #-win32 '("-c" "pwd")
                                   #+win32 "cmd.exe"
                                   #+win32 '("/c" "cd")
                                   :output :stream
                                   :directory directory
                                   :search t))))
     (equal directory
            (string-right-trim '(#\Return) (read-line out))))))

Another correction would be removing spaces in
+ /* Change working directory if supplied. */
+ if ( pwd ) {
+ if ( chdir ( pwd ) < 0) {
+ goto error_exit;
+ }
+ }
so that it reads (pwd) and so on.

With these changes, I will commit it after the freeze.

Matthias Benkard (mulk) wrote :

Ah. I see. Thanks! :)

I added the spaces in the bottom part of the C part of the patch to maintain consistency with the surrounding code, which is also formatted in this way. That said, now that I'm looking at it again, I'm wondering whether that part is even necessary. Does the HANDLE spawn(...) function get used anywhere? As far as I can tell, on Windows, spawn isn't called at all, and the HANDLE-returning version seems to be Windows-specific.

Anyway, here's the updated patch.

Matthias Benkard (mulk) wrote :

Oops, forgot to rebase. Sorry!

Stas Boukarev (stassats) wrote :

Thanks, applied, slightly modified for better error handling.

commit 606dfed39b56dc435ff40e7baf47a455019aae49
Author: Stas Boukarev <email address hidden>
Date: Mon Apr 29 23:28:32 2013 +0400

    Add :directory argument to sb-ext:run-program.

    The implementation uses chdir(2) on Unices, the lpCurrentDirectory
    argument to CreateProcessW on Windows.
    Slightly adapted from the patch by Matthias Benkard.

Changed in sbcl:
status: Triaged → Fix Committed
Changed in sbcl:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers