Fork me on GitHub
#planck
<
2016-06-23
>
johanatan00:06:11

@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=> 
any idea why my string arg is being converted into a js object?

johanatan00:06:11

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)?

mfikes00:06:07

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

johanatan00:06:34

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

johanatan00:06:40

there are other examples that expect strings though

johanatan00:06:53

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

johanatan00:06:16

function_raw_write_stdout e.g.

johanatan00:06:59

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

mfikes01:06:16

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

johanatan01:06:39

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

johanatan01:06:30

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

mfikes01:06:16

Let me look.

johanatan01:06:27

but that would still produce a string though no?

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

mfikes01:06:30

@johanatan: cmd is an array of strings

johanatan01:06:42

ahh, right. i was just wondering about that

johanatan01:06:09

so, i think this makes sense then

johanatan01:06:15

aren't arrays also objects in js?

mfikes01:06:25

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

johanatan01:06:41

ok, cool. thx!

mfikes01:06:21

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

mfikes01:06:20

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

johanatan01:06:59

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

johanatan01:06:10

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

johanatan01:06:30

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

mfikes01:06:25

Cool. C is for ClojureScript 🙂

mfikes01:06:25

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)

johanatan01:06:19

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

johanatan01:06:16

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

mfikes01:06:11

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

johanatan01:06:47

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

mfikes02:06:14

@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

johanatan02:06:26

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

mfikes02:06:23

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

johanatan03:06:36

@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

johanatan03:06:50

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

johanatan03:06:12

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

mfikes03:06:21

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

johanatan03:06:36

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

johanatan03:06:03

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

johanatan03:06:43

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

mfikes03:06:33

They are keyword arguments. So

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

mfikes03:06:27

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"]}])

mfikes03:06:10

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

mfikes03:06:36

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

mfikes04:06:51

@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

johanatan04:06:16

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

johanatan04:06:41

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)
	 (planck/io.cljs:259:33)
	planck.shell/sh (planck/shell.cljs:49:60)

johanatan04:06:35

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)
	 (planck/io.cljs:259:33)
	planck.shell/sh (planck/shell.cljs:49:60)

slipset08:06:56

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

slipset08:06:10

(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"]}]]}

slipset08:06:22

btw, love this one:

slipset08:06:42

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

slipset08:06:54

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

mfikes12:06:52

Great catch… merged in the fix 🙂

slipset12:06:52

My pleasure 🙂

johanatan14:06:27

@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

johanatan14:06:39

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

mfikes14:06:35

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

mfikes14:06:18

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

mfikes14:06:54

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

johanatan14:06:15

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

johanatan14:06:36

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

johanatan14:06:09

oops, sorry. nevermind

mfikes14:06:55

Yeah, IIRC they start off “empty"

johanatan14:06:10

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

johanatan14:06:23

just needs to be flattened list of strings

mfikes14:06:20

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.

johanatan14:06:32

yes, i think that's right

johanatan14:06:51

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

mfikes14:06:28

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

johanatan14:06:49

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

mfikes14:06:02

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.

johanatan14:06:54

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

johanatan14:06:04

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

johanatan16:06:34

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?

mfikes16:06:04

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

johanatan16:06:29

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

mfikes16:06:44

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

johanatan16:06:59

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

johanatan16:06:08

might be ready for PR/review

mfikes16:06:12

Very nice!!!

mfikes16:06:52

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

johanatan16:06:56

i still need to make it handle :in if provided

johanatan16:06:13

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

mfikes16:06:21

Yes, I agree 🙂

mfikes16:06:43

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

johanatan17:06:01

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

johanatan17:06:08

Not sure how it has anything to do with my changes

johanatan17:06:37

Oops, i see some errors above that

mfikes17:06:28

Right… dunno where those defines are supposed to come from

johanatan17:06:49

stdio.h apparently

johanatan17:06:51

which is included

johanatan17:06:18

oh, oops. unistd.h

johanatan17:06:24

i'll try adding an explicit include to that one

johanatan17:06:04

Looks like that fixed it

mfikes17:06:49

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

johanatan17:06:58

oh, that was intentional.

johanatan17:06:13

there are some other #ifdef DEBUGs in the project

johanatan17:06:27

[but i just found a bug in read_child_pipe]

mfikes17:06:45

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

mfikes17:06:03

Will see...

johanatan17:06:05

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

mfikes17:06:52

Very cool. It’s working 🙂 !!!

johanatan17:06:17

Ok, i just pushed the bug fix for read_child_pipe.

johanatan17:06:39

[and removed DEBUG prints]

mfikes17:06:32

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

mfikes17:06:49

This is good stuff 🙂

johanatan18:06:20

Should be good to go now. 🙂

johanatan18:06:40

Ahh, I see you merged it. 🙂

mfikes18:06:12

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

johanatan18:06:35

Oooh, do i need to create another one?

mfikes18:06:54

Yes… your latest stuff is not in Planck master

johanatan18:06:28

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

johanatan18:06:25

Oops, nevermind. Found it

johanatan18:06:42

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

mfikes18:06:16

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

johanatan18:06:39

Do tests pass with the DEBUG statements removed?

mfikes18:06:33

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

johanatan22:06:09

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

mfikes22:06:49

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

johanatan22:06:36

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.

johanatan22:06:44

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

mfikes22:06:30

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

mfikes22:06:30

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

johanatan22:06:36

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

mfikes22:06:55

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.)

mfikes22:06:21

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.

johanatan23:06:31

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

johanatan23:06:35

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?

mfikes23:06:45

@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

johanatan23:06:18

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

johanatan23:06:17

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

mfikes23:06:19

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

mfikes23:06:01

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

mfikes23:06:58

API design is hard :(

mfikes23:06:33

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

mfikes23:06:23

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

johanatan23:06:37

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

johanatan23:06:30

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

mfikes23:06:56

Is promesa a Clojure library?

mfikes23:06:10

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

johanatan23:06:34

which i adapted to my purposes

mfikes23:06:26

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

johanatan23:06:50

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

mfikes23:06:02

Exactly :)

johanatan23:06:26

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

mfikes23:06:18

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

mfikes23:06:39

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

mfikes23:06:07

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