Fork me on GitHub
#babashka-sci-dev
<
2022-01-14
>
lispyclouds09:01:15

@borkdude we dont have a formal ability to send multiple values to the same key in the query params in bb.curl right? we currently pass a map to :query-params. I need this in https://github.com/lispyclouds/contajners/issues/8 . We can send a vector of vectors and that could work as a hack but if you think its a useful thing to add i can add the same functionality as https://github.com/into-docker/unixsocket-http/issues/17

borkdude09:01:23

@rahul080327 Is the proposal to add a vector as a value for repeated args?

borkdude09:01:42

The [[k v] [k v]] approach already works too right?

lispyclouds09:01:25

yes it would but feels a bit hacky maybe? i was thinking we can keep it like clj-http does?

borkdude09:01:39

If I were you I would maybe have replied that this is a sufficient way to do it. It's not accidental at all, the implementation treats the associative as a seq of key-vals.

borkdude09:01:50

Also the [[k v] [k v]] approach allows you to specify the order of params

borkdude09:01:59

if that is important for whatever reason

lispyclouds09:01:24

right, the thing was it would need some translation in the contajners layer and its a bit of a mismatch when people read the docker/podman docs and try to send a list like its documented and it fails

borkdude09:01:48

is the issue that contaijners uses bb curl?

lispyclouds09:01:04

yeah thats how i make it run on bb

lispyclouds09:01:26

i can just do the translation in contajners too

borkdude09:01:34

I think it's sufficient to document the [[k v]] approach and translate it in contaijners

lispyclouds09:01:53

cool will go ahead with that

borkdude09:01:42

the clj-http also has :multi-param-style which you didn't introduce but just went ahead and treated vectors always as if you would specify multiple keys

borkdude09:01:15

oh sorry I misread the clj-http docs

borkdude09:01:17

;; default style, with :multi-param-style unset
:a [1 2 3] => "a=1&a=2&a=3"
;; with :multi-param-style :array, a repeating param with array suffix
;; (PHP-style):
:a [1 2 3] => "a[]=1&a[]=2&a[]=3"

borkdude09:01:54

ah well, if this is important, we can support this too I guess :)

borkdude09:01:05

but [[k v]] should remain working

borkdude09:01:53

I think you would have to do the translation to support older bb's anyway

lispyclouds09:01:18

wouldnt really need a translation in contajners if we implement this in bb.curl. its broken currently if you pass a list 😛

borkdude09:01:00

what happens then? glancing at the code, it looks like it should work

lispyclouds10:01:52

well then contajners code is fine, its just that the vec that you pass is converted to a str and docker/podman isnt very happy at that

lispyclouds10:01:54

what it wants is ?containers=id1&containers=id2 and what it sees now is containers=[id1, id2]

borkdude10:01:46

sure but if you pass [["containers" "id1"] ["containers" "id2"]] to bb curl it should work right

lispyclouds10:01:38

yep it would. with the translation in contajners. wanted your opinion if its something we'd wanna add in bb.curl too, to keep it like clj-http

borkdude10:01:11

I already replied above. you will have to do the translation anyway for older bb's

lispyclouds10:01:15

it worked like this in unixsocket-http too, but xsc added the chnage anyways

borkdude10:01:52

but adding that seems fine. you don't think it's a breaking behavior?

lispyclouds10:01:28

i was thinking i would just check if v is a coll? in https://github.com/babashka/babashka.curl/blob/master/src/babashka/curl.clj#L102 and do the recur differently?

lispyclouds10:01:46

instead of just (str v)

borkdude10:01:48

please answer my question ;)

lispyclouds10:01:09

would not be breaking right? {:a 42} and {:a [42 43]} should both work if passed to :query-params as well as [[:a 42] [:a 43]]?

borkdude10:01:49

you're changing the behavior or {:a [42 43]} then right?

lispyclouds10:01:16

yeah i want it to also accept a coll as the v in the query params map

borkdude10:01:31

I don't think coll? is the right predicate. it should be sequential? imo

borkdude10:01:42

a map is also a coll?

lispyclouds10:01:07

right, brb in a bit!

borkdude10:01:39

So to summarize: 1. You would have to do the translation in contaijners anyway, since older bb's don't have this new query-params feature 2. Adding it to bb.curl seems fine, but I'm a bit concerned about breaking changes: the behavior of {:a [1 2 3]} is changed, instead of a=[1 2 3] we get a=1&a=2&a=3. Not sure if this is even worth the change. My inclination is to keep the old behavior and just document how to get multiple keys with the same name, but I could be convinced otherwise with strong arguments :)

lispyclouds11:01:14

yeah my only argument is that this makes it explicit that this key in the query params has multiple values and since bb.curl was inspired by clj-http, we can follow its behavior 😄 but i see the point of the change of behaviour and i can just add it as a translation to contajners

borkdude11:01:04

> i can just add it as a translation to contajners not only can, you should because of older versions of bb, a point which I've been trying to make several times now ;)

borkdude11:01:13

imo [["a" "1"] ["a" "2"]] is pretty explicit and resembles the query string even more than {:a [1 2]} but perhaps if you have a lot of those, it becomes annoying.

borkdude11:01:58

adding too much magic always is going to shoot you in the foot in the long run

lispyclouds11:01:17

yes, i was missing the point of bb being a runtime and not another lib 😅

lispyclouds11:01:21

> but perhaps if you have a lot of those, it becomes annoying its all gonna be generated in my case so not an issue 😄

borkdude11:01:04

perfect, thank you

1
cap10morgan15:01:08

hmm, I think I should add the arch into the graalvm dir. think it's not re-downloading b/c it has the amd64 one cached already.

borkdude15:01:30

@cap10morgan hmm.

bin/gu: cannot execute binary file: Exec format error

Exited with code exit status 126

borkdude15:01:39

cached, how so?

cap10morgan15:01:46

in circleci's cache

borkdude15:01:54

oh yes, I see

borkdude15:01:04

you can add the arch name into the cache key perhaps

👍 1
borkdude15:01:08

I think this time it will work

🤞 1
borkdude15:01:05

after that I can release to pod registry. but perhaps we also want to update the cognitect lib, while I'm at it. I'll do that after merge

borkdude15:01:29

you probably know that you can load a pod by path too right?

cap10morgan19:01:59

ok finally got that arm64 pod build packaged up and deployed w/ my app. getting this error from it: /opt/pods/pod-babashka-aws: error while loading shared libraries: cannot make segment writable for relocation: Cannot allocate memory

cap10morgan19:01:16

this is an aws lambda function

borkdude19:01:04

does that arm64 environment have glibc?

cap10morgan20:01:10

it's amazon linux 2, so I would think so

borkdude20:01:49

what do you get for ldd /opt/pods/pod-babashka-aws and are you 100% sure you have the right binary? :)

cap10morgan20:01:32

pretty sure but I'll double check

borkdude20:01:10

perhaps some security thing?

cap10morgan20:01:16

ldd pod-babashka-aws outputs:

linux-vdso.so.1 (0x0000ffffb6ed9000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x0000ffffb093b000)
	libdl.so.2 => /lib64/libdl.so.2 (0x0000ffffb091a000)
	libc.so.6 => /lib64/libc.so.6 (0x0000ffffb0794000)
	/lib/ld-linux-aarch64.so.1 (0x0000ffffb6e9b000)

cap10morgan20:01:05

seems like I can run the pod in an amazonlinux:2 docker container locally, so yeah security thing in the lambda env seems likely

borkdude20:01:09

perhaps @rahul080327 knows something

borkdude20:01:15

are you using a custom docker env for lambda?

cap10morgan20:01:21

don't think so

cap10morgan20:01:29

it's the provided.al2 runtime

borkdude20:01:19

perhaps we need to leave out the --static flag altogether and only provide the other one

borkdude20:01:34

worth a shot

borkdude20:01:07

So here:

if [ "$BABASHKA_STATIC" = "true" ]; then
    args+=("--static")
    if [ "$BABASHKA_MUSL" = "true" ]; then
        args+=("--libc=musl"
               # see 
               "-H:CCompilerOption=-Wl,-z,stack-size=2097152")
    else
        # see 
        args+=("-H:+StaticExecutableWithDynamicLibC")
    fi
fi

borkdude20:01:31

move args+=("--static") to the musl part only

borkdude20:01:55

if that is the solution, then we need to change it in the bb bash as well

cap10morgan20:01:22

yep, got it. thanks!

cap10morgan20:01:30

trying it now

cap10morgan20:01:03

hmm, same error

cap10morgan20:01:25

maybe it just needs a higher mem limit

borkdude20:01:35

what's the memory limit now?

borkdude20:01:00

In the blog post I linked earlier they have some kind of patch tool to work around that security thing

cap10morgan20:01:00

I think that fixed it

cap10morgan20:01:10

just needed a higher mem limit

cap10morgan20:01:58

I'll try the original binary again too

borkdude20:01:31

ah alright. weird! graalvm images normally don't take a lot of mem

cap10morgan20:01:01

ok original binary works too

cap10morgan20:01:09

want me to revert that last change?

borkdude20:01:28

yeah, let's do that

👍 1
borkdude20:01:17

after that we can merge and then do some updating, etc. I'll roll out the new version tomorrow

👍 1