[asdf-devel] asdf:run-shell-command - fix, delete or don't touch?
Do we want to (a) leave run-shell-command half-broken on various combination of OSes and implementations as soon as any argument needs quoting, do we want to (b) use heavy artillery to solve the problem correctly, or should we not just (c) delete this broken functionality that obviously nobody can or should be relying on for anything serious? My preference goes to (c) delete the damn thing (replacing the function body by an error that suggests a good replacement), but only if it doesn't create a quagmire for users. So I could really use some feedback from the ASDF constituency on that. If we want option (b), I just updated xcvb-driver so its run-program/process-output-stream does all that asdf:run-shell-command does, in addition to its previous functionality that allows actually using the command output. And xcvb-driver supports all the implementations that asdf:run-shell-command does, and more (cormanlisp, though untested). Moreover it works correctly on Windows, including with ClozureCL despite a bug and on Allegro (wanted here). But it's about 300 lines of complex code, as opposed to about 100 of trivial wrappers in the current half-broken implementation. See from requires-escaping-p to run-program/process-output-stream in xcvb/driver.lisp on http://common-lisp.net/gitweb?p=projects/xcvb/xcvb.git Note that unlike XCVB, ASDF itself doesn't use run-shell-command. The obvious suspects cffi-grovel and sb-grovel don't use it either. Actually, I wonder if *anyone* uses asdf:run-shell-command. In my local checkouts, only asdf-install (obsolete) and an experimental file in mcclim (that could be fixed if needed) explicitly call asdf:run-shell-command. Xach - would it be easy for you to test Quicklisp with a version of ASDF without run-shell-command (or fmakunbound'ing it early on) and see if anything breaks? —♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org
On Thu, 6 Oct 2011, Faré wrote:
Do we want to (a) leave run-shell-command half-broken on various combination of OSes and implementations as soon as any argument needs quoting, do we want to (b) use heavy artillery to solve the problem correctly, or should we not just (c) delete this broken functionality that obviously nobody can or should be relying on for anything serious?
My preference goes to (c) delete the damn thing (replacing the function body by an error that suggests a good replacement), but only if it doesn't create a quagmire for users.
There are so many problems with "portable shell programming" that I don't think (b) can be done in general. Tools can be written to paper over the commonly used functions, but even POSIXy shells have surprising variety. C shells are not POSIX, and the DOS prompt is a whole other beast. Autoconf has perhaps the best attempt at documenting and addressing the issues. http://www.gnu.org/software/autoconf/manual/html_node/Portable-Shell.html For most programmatic use, the shell is simply a hurdle in the way of invoking another process. So why use it? IMO, the right approach is to have a core CL library that gives direct access to the exec() family of functions on unix and the closest equivalent on other OSes (createprocess on MSWin). Argument quoting for string manipulation is a shell thing and should be discouraged in programmatic use (one thing the CL path designers got right). Quotes cannot be added programmatically in all cases; so the user really must be responsible for adding escapes where needed, again preferably using exec so the shell is not involved. (MSWin is a basket case in that each process implements its own command-line parsing however it pleases. So auto-quote becomes a generic function dispatching on the target process...) Other libraries can then build features on top of this core. However, "run this program with these arguments" is handled nicely by execv, and pretty much everything more sophisticated is better handled inside CL. Fortunately, most CLs expose a variant of execv Allegro excl.osi:execve ECL ext:run-program SBCL sb-ext:run-program LW system:call-system (with the arglist options) ... Long story short, I wouldn't invest more in ASDF's command. I would try to promote an execv-style wrapper as a de facto standard, possibly with an optional attempt at quoting for MSWin. Once that is chosen (and probably polished a bit more), port the handful of systems using run-shell-command to this other library, and then step (c) is the logical conclusion. After step (c), ASDF could ship with a copy of this shell library, much as most distros include ASDF... Later, Daniel
On Thu, Oct 6, 2011 at 23:14, Daniel Herring <dherring@tentpost.com> wrote:
There are so many problems with "portable shell programming" that I don't think (b) [fix run-shell-command] can be done in general. [...] IMO, the right approach is to have a core CL library that gives direct access to the exec() family of functions on unix and the closest equivalent on other OSes (createprocess on MSWin). Argument quoting for string
Well, this is *exactly* what asdf:run-shell-command or xcvb-driver:run-program/process-output-stream is all about: it's a wrapper around whatever functionality is provided by the implementation, that basically allows you to spawn a process in a portable way (and, in XCVB, also to process the output of said command). If a shell is ever involved, it's only because many implementations do not provide any better access to subprocesses than system(3). asdf:run-shell-command adopts a "least common denominator" approach, where nothing can be portably run that has any kind of character in it that any implementation may interpret specially, including any of " $\\;^!`\"'(){}|<>?*&~". If you don't care for portability, you might use the richer interface of whichever implementation you're using.
(MSWin is a basket case in that each process implements its own command-line parsing however it pleases. So auto-quote becomes a generic function dispatching on the target process...) xcvb-driver:run-program/process-output-stream allows you to either give a list of strings as a command, that gets quoted the "standard" way or a string, where you control what quoting you use.
Long story short, I wouldn't invest more in ASDF's command. I would try to promote an execv-style wrapper as a de facto standard, possibly with an optional attempt at quoting for MSWin. That's what xcvb-driver:run-program/process-output-stream does. For a more powerful tool than this portable wrapper, one could use iolib's portable reimplementation of run-program, but it doesn't look like anyone's willing to implementing it for Windows, and it comes with a lot of baggage (libfixposix, cffi, etc.)
Once that is chosen (and probably polished a bit more), port the handful of systems using run-shell-command to this other library, and then step (c) [delete asdf:run-shell-command] is the logical conclusion. After step (c), ASDF could ship with a copy of this shell library, much as most distros include ASDF...
Yup. —♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org
Faré wrote:
If a shell is ever involved, it's only because many implementations do not provide any better access to subprocesses than system(3).
Slightly off-topic for asdf, but IMO bug reports should be filed against all such implementations. The system() call is nice for easy things, but really hairy for complex stuff. For example, the most popular answer to a Python programmer's question about system() quoting was "use subprocess.call instead" (an execv-style command). http://stackoverflow.com/questions/204017/how-do-i-execute-a-program-from-py... - Daniel
Faré <fahree@gmail.com> writes:
Do we want to (a) leave run-shell-command half-broken on various combination of OSes and implementations as soon as any argument needs quoting, do we want to (b) use heavy artillery to solve the problem correctly, or should we not just (c) delete this broken functionality that obviously nobody can or should be relying on for anything serious?
My preference goes to (c) delete the damn thing (replacing the function body by an error that suggests a good replacement), but only if it doesn't create a quagmire for users.
My preference is to leave it as-is.
Xach - would it be easy for you to test Quicklisp with a version of ASDF without run-shell-command (or fmakunbound'ing it early on) and see if anything breaks?
Of Quicklisp projects, it breaks CommonQT, elephant, gsll, umlisp, and cl-gene-searcher. Zach
I responded to this, based on some other systems I know of. Unfortunately, I responded from a different email address. I don't believe that the email made it to the list, did it? Does anyone still perform the moderator chores on this list? cheers, r
On Fri, Oct 7, 2011 at 08:40, Zach Beane <xach@xach.com> wrote:
Faré <fahree@gmail.com> writes:
Do we want to (a) leave run-shell-command half-broken on various combination of OSes and implementations as soon as any argument needs quoting, do we want to (b) use heavy artillery to solve the problem correctly, or should we not just (c) delete this broken functionality that obviously nobody can or should be relying on for anything serious?
My preference goes to (c) delete the damn thing (replacing the function body by an error that suggests a good replacement), but only if it doesn't create a quagmire for users.
My preference is to leave it as-is.
Xach - would it be easy for you to test Quicklisp with a version of ASDF without run-shell-command (or fmakunbound'ing it early on) and see if anything breaks?
Of Quicklisp projects, it breaks CommonQT, elephant, gsll, umlisp, and cl-gene-searcher.
You win, thanks to superior data. Thanks, Zach! My plan: 1- declare the interface deprecated for now 2- work with the authors of said software to migrate off of it. 3- when no one is known to use it anymore, make it issue a warning at compile and/or runtime. 4- after a few versions, make it issue an error As for replacements, are there serious contenders besides xcvb-driver:run-program/process-output-stream? If not, I'll list it as the "official" replacement -- though first I may need to get it into Quicklisp. PS: I looked at the following, and none of them provide either the implementation coverage, os portability, correctness wrt escaping arguments, or output processing option that xcvb-driver:run-program/process-output-stream provides. Therefore, I propose that the "official" replacement be xcvb-driver. GBBopen:run-external-program asdf-install::shell-command cffi-grovel::%invoke uffi-compat:run-shell-command elephant::launch-background-program executor::spawn-process-from-executable external-program::run external-program::start gsharp-play::play-tracks iolib-grovel::%invoke kmrcl::command-output mcclim's clim-listener::run-program, dwim::run-shell-command colorize::system from parse-declarations/doc/colorize-lisp-examples.lisp or cffi/doc/colorize-lisp-examples.lisp philip-jose::spawn-process trivial-shell::%shell-command PS2: Yes, Daniel, you can implement a successful synchronous run-program on top of system(3), thanks to careful quoting, which of course depends on POSIX vs Windows. See how I do it in xcvb-driver. Therefore, it's not necessarily a bug to not support more than this. For asynchronous use, you'd need good support for either threads or event loops, at which point it's way out of the scope of either ASDF or XCVB, and for portability you might as well use IOLib (though it doesn't support Windows at this point). —♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org
On 10/7/11 Oct 7 -10:35 AM, Faré wrote:
On Fri, Oct 7, 2011 at 08:40, Zach Beane <xach@xach.com> wrote:
Faré <fahree@gmail.com> writes:
Do we want to (a) leave run-shell-command half-broken on various combination of OSes and implementations as soon as any argument needs quoting, do we want to (b) use heavy artillery to solve the problem correctly, or should we not just (c) delete this broken functionality that obviously nobody can or should be relying on for anything serious?
My preference goes to (c) delete the damn thing (replacing the function body by an error that suggests a good replacement), but only if it doesn't create a quagmire for users.
My preference is to leave it as-is.
Xach - would it be easy for you to test Quicklisp with a version of ASDF without run-shell-command (or fmakunbound'ing it early on) and see if anything breaks?
Of Quicklisp projects, it breaks CommonQT, elephant, gsll, umlisp, and cl-gene-searcher.
You win, thanks to superior data. Thanks, Zach!
My plan: 1- declare the interface deprecated for now 2- work with the authors of said software to migrate off of it. 3- when no one is known to use it anymore, make it issue a warning at compile and/or runtime. 4- after a few versions, make it issue an error
I don't think that's a full solution, per my somewhat garbled email of last night. The problem is that we will end up with (defsystem foo (:defsystem-depends-on xcvb-run-program) ... system definition ... ) The problem is that unlike before, we can't have nice (defmethod PERFORM ((op my-op) (c my-component-class)) (run-shell-program ....)) Instead, we end up with the much nastier (defmethod PERFORM ((op my-op) (c my-component-class)) (funcall (intern (symbol-name #:fare-run-shell-program) :xcvb-runner ....))) I suppose if there is a one function-sized API, this won't be too bad, because we can just define (defun run-shell-command (&rest args) (apply (intern (symbol-name #:fare-run-shell-program) :xcvb-runner) ....)) once and then writing the PERFORM methods will be OK.... :DEFSYSTEM-DEPENDS-ON doesn't interact very gracefully with the package system.... Cheers, r
My plan: 1- declare the interface deprecated for now 2- work with the authors of said software to migrate off of it. 3- when no one is known to use it anymore, make it issue a warning at compile and/or runtime. 4- after a few versions, make it issue an error
I don't think that's a full solution, per my somewhat garbled email of last night. The problem is that we will end up with
(defsystem foo (:defsystem-depends-on xcvb-run-program) ... system definition ... )
Is that a problem? I mean, beside the fact that defsystem-depends-on only exists since 2.010 or so and is only fully supported since 2.015.12. If that's the issue, there is still the old-style way of having the .asd file explicitly call (asdf:load-system :xcvb-driver) which is what the defsystem-depends-on does under the hood.
The problem is that unlike before, we can't have nice
(defmethod PERFORM ((op my-op) (c my-component-class)) (run-shell-program ....))
Instead, we end up with the much nastier
(defmethod PERFORM ((op my-op) (c my-component-class)) (funcall (intern (symbol-name #:fare-run-shell-program) :xcvb-runner ....)))
:DEFSYSTEM-DEPENDS-ON doesn't interact very gracefully with the package system.... Well, you can't use packages defined in such a system in the defsystem form itself, but if you define your classes in the ASDF or KEYWORD package, you're good. And really, there's no reason not to, fears of namespace
There is no such constraint in a .asd file. Remember: a .asd file is loaded as source, not compiled as .fasl, so it IS possible for it to load some other files that create packages and then use symbols from those packages later in the .asd file. So as long as you evaluate your (defsystem ... :defsystem-depends-on ...) first, or the old-style equivalent (asdf:load-system ...), then you're good and you can (defmethod perform ((op my-op) (c my-component-class)) (xcvb-driver:run-program/for-side-effects `(,*my-compiler* "-o" (output-file op c) ,@(input-files op c)))) and it will do the Right Thing(tm), even when the paths include spaces, even on Windows using Allegro or ClozureCL. Actually, you might still need a native-namestring abstraction for such a method to work at all on implementations like SCL, and to cover all the corner cases in other implementations like SBCL or ClozureCL. XCVB-Driver does not yet provide such a functionality at this moment, but may very well, in the near future. pollution notwithstanding. You know how to write a symbol whose name want clash in practice. In the end, I agree with you that this kind of functionality should be a portable part of the "batteries included" with every CL system. I just don't think it belongs to ASDF itself, since ASDF can run without it, and we want to keep ASDF minimal, and what's currently in ASDF for legacy reason seems both broken and not widely used (the latter probably because of the former). My plan of deprecating asdf:run-shell-command still holds so far. But before I declare the function officially deprecated, I'll make sure a better replacement is widely available. —♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org
In 2.017.11, I fixed the allegro version of asdf:run-shell-command on Unix. I also changed the way that same function is broken on Windows so at least it gives users some control, and behaves in a more regular way across implementations — i.e. some may call CMD, some may try to call sh instead, but at least you control exactly what's given to either interpreter, rather than have it garbled. In the comments I also declare the function deprecated and recommend xcvb-driver:run-program/process-output-stream and derivatives as replacement. Now to get xcvb-driver in Quicklisp and contact clients to stop using this mostly broken function... —♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org I don't know if might makes right, but lack of might sure makes lack of right.
participants (5)
-
Daniel Herring -
dherring＠tentpost.com
-
Faré -
Robert Goldman -
Zach Beane