Clojurians Log v2

@mfikes: i'm getting an kJSTypeObject as the only member of argv in my shellexec handler when invoking the sh function with a string. When I try to invoke it with an integer or an actual object I correctly get the following error:

cljs.user=> (require '[planck.shell])
nil
cljs.user=> (planck.shell/sh (clj->js {:a "b"}))
In: [0] val: #js {:a "b"} fails spec: :planck.shell/sh-args at: [:cmd] predicate: string?
ERROR
cljs.user=> (planck.shell/sh 9)
In: [0] val: 9 fails spec: :planck.shell/sh-args at: [:cmd] predicate: string?
ERROR
cljs.user=> 

also any idea how to find out what keys are on that object (which would be my next step for trying to diagnose it myself)?

@johanatan: I'm AFK now but I bet the PLANK_REQUEST http get stuff (already implemented in planck-c) has some similarities with sh.

the http stuff actually expects an object (and apparently gets one)

there are other examples that expect strings though

and i didn't see any noticeable difference in the way they're setup and the way mine is

function_raw_write_stdout e.g.

i've pushed my code if you'd like to take a look when you have a chance: https://github.com/johanatan/planck/tree/shellExecWorkInProgress

@johanatan: worth looking at the ClojureScript and Objective-C. Perhaps the first arg is passed through.

(Not)

oh, hmm. ok, i'll try to track that down

but given that you said there weren't any changes required to those for the rest of planck-c i figured they would remain constant for this one function as well

Let me look.

k

@johanatan: I don’t know why clj->js is being applied to cmd here https://github.com/mfikes/planck/blob/master/planck-cljs/src/planck/shell.cljs#L39

but that would still produce a string though no?

cljs.user=> (clj->js "blah")
"blah"

@johanatan: cmd is an array of strings

ahh, right. i was just wondering about that

Actually 🙂 via the spec one or more strings: https://github.com/mfikes/planck/blob/master/planck-cljs/src/planck/shell.cljs#L58

so, i think this makes sense then

aren't arrays also objects in js?

If you look at the Objective-C you will see how it is dealt with on that side...

ok, cool. thx!

https://github.com/mfikes/planck/blob/7b8fb645177a5828de87b51cca9ab883402e9436/planck/PLKClojureScriptEngine.m#L615-L619

@johanatan: ^ fortunately, half of that code is actually C already 🙂

ha, right

Hah, the use of C to interoperate with JavaScriptCore traces back to a suggestion of David Nolen’s from a little more than a year ago. I guess he knew one day Planck would be ported to straight C. 🙂 http://blog.fikesfarm.com/posts/2015-05-23-ambly-using-javascriptcore-c-api.html

nice

btw, do you know if there's anything like C++'s std::vector in our C environment here? really not looking forward to a bunch of malloc and free calls to do dynamic allocation

i.e., NSMutableArray from the Obj-C example you linked to

so, this works: https://github.com/johanatan/planck/commit/62a7fb8484edac14840273c5039626da8c4e5456 [wish it were a little tighter but I guess that's C for ya]

Cool. C is for ClojureScript 🙂

🙂

Yeah, the last time I was doing malloc / free was in 1994, I think. By the way this will help if you want to check for memory corruption: https://github.com/mfikes/planck/blob/master/planck-c/Makefile#L7 (add the sanitize address flag)

ahh, cool.

i unfortunately have been doing this stuff as recently as 2008-09 lol

do you know if C/Linux file handling functions know/understand file:// url syntax?

No clue

If you don't have access to Linux, I can check the behavior of code for you

What would we do about supporting :in (and :in-enc) if it turns out file:// isn't supported on C/Linux?

@johanatan: I’m actually not familiar with that particular feature… but I’m guessing we could convert file:// urls to their local path, if it is possible to do so

Ya, I think it would be to merely strip that prefix and replace it with '/'

In the long run, handling of file:// in general might be possible to cope with via the <http://planck.io/IOFactory|planck.io/IOFactory> machinery. (That’s the way http:// can be handled, etc., in a generic fashion.) But yeah, just stripping file:// probably suffices for now.

@mfikes: ok, i think we have a bigger issue with the char encodings. std C doesn't have any notion of these and you'd have to have a bunch of functions like the ones on this page to convert between the encodings (if you wanted to): http://codereview.stackexchange.com/questions/40780/function-to-convert-iso-8859-1-to-utf-8

[unless there is a 3rd party library that provides all of this for us]

for now, i'm going to ignore :in-enc and :out-enc

Yeah… I wouldn’t worry too much about that. I bet most people use UTF-8 if anything and we might be able to take advantage of some stuff that might already exist in JavaScriptCore. For example https://github.com/mfikes/planck/commit/45874f341c02f241f2880b3ae985828f2bbdf99f#diff-b1129fb19d4dc0f5029761d2161e6f22L76

Okay

One other thing: how do you invoke sh with the optional arguments?

I couldn't find any examples in the codebase or tests that did anything but set string varargs for the 'cmd'

This was my best attempt in the REPL:

cljs.user=> (planck.shell/sh "0" {:env {"blah" "blah"}})
In: [1] val: ({:env {"blah" "blah"}}) fails spec: :planck.shell/sh-args predicate: (cat :cmd (+ string?) :opts (* :planck.shell/sh-opt)),  Extra input
ERROR

@mfikes: ^

They are keyword arguments. So

cljs.user=> (planck.shell/sh "env" :env {"foo" "bar"})
{:exit 0, :out "foo=bar\n", :err “"}

What’s exciting these days is you can consider using spec to generate example arguments:

cljs.user=> (s/exercise :planck.shell/sh-args)
([("") {:cmd [""]}]
 [("o" "m" :in "9") {:cmd ["o" "m"], :opts [[:in {:key :in, :val "9"}]]}]
 [("" "v0" "Cl" :env {}) {:cmd ["" "v0" "Cl"], :opts [[:env {:key :env, :val {}}]]}]
 [("Gs" "" :in-enc "RFY" :in "G" :out-enc "3dP")
  {:cmd ["Gs" ""], :opts [[:in-enc {:key :in-enc, :val "RFY"}] [:in {:key :in, :val "G"}] [:out-enc {:key :out-enc, :val "3dP"}]]}]
 [("O5" "n7j" "d" "" :out-enc "" :out-enc "K0" :dir "a0a")
  {:cmd ["O5" "n7j" "d" ""],
   :opts [[:out-enc {:key :out-enc, :val ""}] [:out-enc {:key :out-enc, :val "K0"}] [:dir {:key :dir, :val [:string "a0a"]}]]}]
 [("cY2" "6Y0" "I8Da" "F" "" "hDrjK" :in "Pb") {:cmd ["cY2" "6Y0" "I8Da" "F" "" "hDrjK"], :opts [[:in {:key :in, :val "Pb"}]]}]
 [("q4i" :in "7gDvI7") {:cmd ["q4i"], :opts [[:in {:key :in, :val "7gDvI7"}]]}]
 [("G3YDS" "Z2q9" "798rPJT" "8fq" "cN" "u" :env {} :dir "a3oHJ8")
  {:cmd ["G3YDS" "Z2q9" "798rPJT" "8fq" "cN" "u"], :opts [[:env {:key :env, :val {}}] [:dir {:key :dir, :val [:string "a3oHJ8"]}]]}]
 [("e" "3Cibazb0" "F3q" :env {}) {:cmd ["e" "3Cibazb0" "F3q"], :opts [[:env {:key :env, :val {}}]]}]
 [("e44fT" "" "AjhU2X2XC" "8jVEyD3" "SgHp" "R6lKza78P" "TtFDVMdVz" "Se5B53Cdh" "F948Sdg5")
  {:cmd ["e44fT" "" "AjhU2X2XC" "8jVEyD3" "SgHp" "R6lKza78P" "TtFDVMdVz" "Se5B53Cdh" "F948Sdg5"]}])

Of course, the spec hasn’t been told that, for example that ”9” is not a legal string encoding 🙂

I suppose the spec could also be improved to require non-empty strings, etc.

@johanatan: By the way, in case it helps, planck.shell/sh is an imitation of clojure.java.shell/sh https://clojuredocs.org/clojure.java.shell/sh

Ahh, nice. I had actually tried a variation of that with :cmd before the first string arg

Any idea on this one? Looks like the value I'm specifying for :dir is being sent through as a vector which as-file doesn't have a protocol method define for.

cljs.user=> (planck.shell/sh "0" :env {"zblahbb" "blahzz" "abc" "def"} :dir "/home/jonathan")
No protocol method Coercions.as-file defined for type cljs.core/PersistentVector: [:string "/home/jonathan"]
	cljs.core/missing-protocol (cljs/core.cljs:266:4)
	<http://planck.io/as-file|planck.io/as-file> (planck/io.cljs:259:33)
	planck.shell/sh (planck/shell.cljs:49:60)

doh! same error happens when the directory actually exists:

cljs.user=> (planck.shell/sh "0" :env {"zblahbb" "blahzz" "abc" "def"} :dir "/Users/jonathan")
No protocol method Coercions.as-file defined for type cljs.core/PersistentVector: [:string "/Users/jonathan"]
	cljs.core/missing-protocol (cljs/core.cljs:266:4)
	<http://planck.io/as-file|planck.io/as-file> (planck/io.cljs:259:33)
	planck.shell/sh (planck/shell.cljs:49:60)

@johanatan: there is an error in the spec handling here...

(s/conform ::sh-args [ "0" :env {"zblahbb" "blahzz" "abc" "def"} :dir "/Users/erik"])
{:cmd ["0"], :opts [[:env {:key :env, :val {"zblahbb" "blahzz", "abc" "def"}}] [:dir {:key :dir, :val [:string "/Users/erik"]}]]}

https://github.com/mfikes/planck/issues/330

https://github.com/mfikes/planck/pull/331

btw, love this one:

(map (comp (juxt :key :val) second) opts)

and the fact that I’m able to read what it does 🙂

Great catch… merged in the fix 🙂

My pleasure 🙂

@mfikes: after taking your latest commits, I'm seeing the following error:

Jonathans-MacBook-Pro:planck-c jonathan$ ./planck 
Planck 2.0
WARN: no bundled sources, need to run script/bundle-c
ClojureScript (Unknown)
    Docs: (doc function-name-here)
          (find-doc "part-of-name-here")
  Source: (source function-name-here)
    Exit: Control+D or :cljs/quit or exit or quit
 Results: Stored in vars *1, *2, *3, an exception in *e

WARN: no bundled sources, need to run script/bundle-c
The goog base JavaScript text could not be loaded

i don't see a script/bundle-c anywhere

Do make bundle-and-build in the planck-c at least once to get it bundle things up. The bundle-c script is off in another part of the tree but the makefile knows where to run it

ahh, ok

After that, you can simply do make to build the native part quickly

(You can rapidly iterate on the native part, with bundle.c containing all of the AOT-compiled JavaScript.)

yea, that's what i've been doing and so forgot about the initial make bundle-and-build

i did check that bundle.h/c were present though and they were-- i suppose they were perhaps stale due to the code changes though

oops, sorry. nevermind

Yeah, IIRC they start off “empty"

ya, :cmd isn't part of the options hash

just needs to be flattened list of strings

Don’t understand… unless you are saying that the command is just the initial segment of the argument list that is strings, until you hit the first non-string.

yes, i think that's right

yep

the first non-string being a keywd to denote which part of options follows

spec’s regular expression machinery (`cat`, et. al.) is awesome for expressing these concepts

ya, i saw cat but didn't dig into what it means just yet 🙂

The high level idea is that a structure can be a sequence or a map. cat is perhaps the top-level for the first, while keys is the top-level for the other. (probably not 100% accurate), but fits my mental model for now.

haha, right

one question: should it be safe to take a c-string obtained from value_to_c_string and do an unbounded strlen or other str operation on it? I am imagining so as I can't see how one could get an unbounded string from ClojureScript through the JS machinery and into these native bits

i.e., it should be safe to assume that the strings obtained in this manner are null-terminated

@mfikes: ^

one other thing: can you explain what self.context is/where it comes from and how it is different than ctx (if it is actually different)? I don't think I have anything analogous to that in the native function: https://github.com/mfikes/planck/blob/7b8fb645177a5828de87b51cca9ab883402e9436/planck/PLKClojureScriptEngine.m#L638-L640 would it be ok to just create those return values on the ctx that is passed into the function?

@johanatan: It should be safe to assume that C strings obtained from that function are NULL-terminated.

and for my second question, i just went ahead and tried it with ctx and it seems to work

@johanatan: self.context and ctx would be the same thing

ahh, nice

so, i have a working example here now. it handles env and dir

might be ready for PR/review

Very nice!!!

(It doesn’t have to be absolutely perfect… the 2.0 branch being under construction.)

i still need to make it handle :in if provided

ok, cool.

in that case, i'd say let's go ahead and get a PR in for this as it would be super useful even without :in support

Yes, I agree 🙂

It may even get us to the point where we can run the existing test infrastructure which uses sh

https://travis-ci.org/mfikes/planck/builds/139812933

Line 498 says the binary was placed but line 499 couldn't find it

Not sure how it has anything to do with my changes

Oops, i see some errors above that

Right… dunno where those defines are supposed to come from

stdio.h apparently

which is included

http://pubs.opengroup.org/onlinepubs/009695399/functions/stdin.html

oh, oops. unistd.h

i'll try adding an explicit include to that one

Yeah

Looks like that fixed it

@johanatan: Looks like you may have inadvertently left some debug console printing

oh, that was intentional.

there are some other #ifdef DEBUGs in the project

[but i just found a bug in read_child_pipe]

OK… that’s likely to cause it to not be usable when running the existing integration tests

Will see...

oh, i can remove them (or hide them behind a diff flag like SHELLDEBUG)

Very cool. It’s working 🙂 !!!

Ok, i just pushed the bug fix for read_child_pipe.

[and removed DEBUG prints]

@johanatan: Cool. Perhaps after that is in, I can make some new Linux binaries and put them on the Planck alpha download page 🙂

This is good stuff 🙂

Should be good to go now. 🙂

Ahh, I see you merged it. 🙂

@johanatan: Yeah, I merged prior to your latest push. (Sigh, mutable PRs.)

Oooh, do i need to create another one?

Yes… your latest stuff is not in Planck master

Hmm, not sure how to do that. I don't see any "create PR" button on my fork now

Oops, nevermind. Found it

[Sorry about the race condition-- i figured i could get the fix in before you merged the PR]

No problem. (This is one of the reasons Cognitect only accepts patches, evidently.)

Do tests pass with the DEBUG statements removed?

Yes https://travis-ci.org/mfikes/planck/builds/139841792

But… if you are asking about the full Planck test suite… not yet… there are other things failing stil.

https://twitter.com/mfikes/status/746064830935728128

Hmm, I don't see any tests in this one-- only the build itself: https://clojurians.slack.com/archives/planck/p1466707684000309

Right. Travis CI is currently only being used to assure Planck 2.0 can be built.

Right, so when I asked about the tests, I meant the ones that you said might run with the impl of sh with debugging removed.

[or were you just referring to the build itself?]

@johanatan: I was referring to the stuff exercised by script/test (and script/test-c)

script/test-c doesn't fully work yet given the gaps that Planck 2.0 has relative to 1.x

ahh, but sh gets us closer to the target I presume?

Part of Planck's tests originate from prior to cljs.test being functional under bootstrap, and involve crude comparisons of stdout with what is expected. (So writing debug output would cause those crude tests to fail.)

And, yes, parts of the test infrastructure use Planck itself to run scripts. Your changes will help Planck fundamentally be able to execute all of that on Linux.

Nice. And I removed the DEBUG prints so should be good to go in that respect.

So, next step for me will be to get async version of sh going. Do you have any guidance on how to accomplish? Should I add a dependency on core.async and add a new function: sh-async which returns a channel which the {:out ... :err ... :status ..} payload is sent over when available?

@mfikes: ^

@johanatan: I wouldn't force people to take on a core.async dep, but instead support a callback. A callback can be converted to core.async by users interested in it. This is also good post in the subject: http://www.lispcast.com/core-async-code-style

Ah, yea, I remember that. As long as the callback is the last argument or something, there's an automatic conversion?

are you ok with the name sh-async or is there a common naming pattern for that sort of thing?

There is no automatic conversion, but it is easy for a client to pass a in lambda that puts a result on a channel.

In terms of API, I'd take a look at the http and other APIs in Planck to get a sense. (I suspect a new arity won't work, and http and sh have different ways of passing optional stuff, so maybe -async might feel right in the end.) might be worth experimenting

API design is hard :(

@johanatan: I'd suggest looking at cljs.js. It went with cb as last argument

(Damn, sh has no "last" argument.)

@mfikes: pretty sure there is an automatic conversion provided by promesa or one of the libs I used recently

i have an example if you'd like me to dig up the code. 🙂

Is promesa a Clojure library?

Yep

https://funcool.github.io/promesa/latest/

Ahh. Yeah. Zach Tellman also has a set of async abstractions.

oh, no. actually it was <<< from here: http://www.lispcast.com/core-async-code-style

Right.

which i adapted to my purposes

Seems like accepting a cb then allows for any preferred style to be used on top of it.

Yep

But how to handle the fact that our arg list is variadic?

Exactly :)

I'm thinking another wrapper that does: (sh-async other-args :cb callback) might work

Yep

It is tempting to revise sh to return nil if :cb is present. Wonder if that would be natural or a hack

Almost any sync API can be revised to return nil for the callback arity or optional cb case. Wonder if that is a common pattern or an anti-pattern

Maybe a separate -async fn is cleaner than trying to complect the two