babashka-sci-dev 2022-03-19

@wilkerlucio I'll take the discussion here about meander. ๐Ÿงต

I've ran the tests:

Ran 155 tests containing 316 assertions.
2 failures, 11 errors.
{:test 155, :pass 303, :fail 2, :error 11, :type :summary}
So there are still a few glitches to iron out, hopefully we can make them pass all.

There seems to be an issue with gather

user=> (babashka.main/main "-cp" "src" "-e" "(require '[meander.epsilon :as r] :reload '[meander.match.epsilon] :reload) (r/match [1 2 3 4 5 6] (r/gather (r/pred any? !xs) ...) !xs)")
:count-pattern ...
:meander/match :syntax-phase
false :nat-int
true :symbol ...
----- Error --------------------------------------------------------------------
Type:     clojure.lang.ExceptionInfo
Message:  non exhaustive pattern match
user=> (babashka.main/main "-cp" "src" "-e" "(require '[meander.epsilon :as r] :reload '[meander.match.epsilon] :reload) (r/match [1 2 3 4 5 6] (r/pred any? !xs) !xs)")
[[1 2 3 4 5 6]]
0

@wilkerlucio has joined the channel

@borkdude OK think I addressed everything in the two PRs. Let me know if you see anything else.

oops, looks like I still have an issue to address with :local/root pods

or... perhaps not. anyway, testing that out now. I'll keep you posted. ๐Ÿ™‚

looks pretty good so far!

apart from the minor remarks, maybe we'll have to do a final bike-shed over the naming.

:local/root
is derived from deps.edn, and makes sense since it's the root directory of a dependency, but pods aren't directories. I think just using :path makes more sense, like the name that the pods lib already uses for it. We could make everything namespaced:
:pod/version ... :pod/path ...
but not sure if that has any benefits since we're already dealing with pods. However, we do use that in the manifests:
:pod/name        huahaiy/datalevin
 :pod/description "Datalevin Database"
 :pod/version     "0.6.1"
 :pod/license     "EPL"

but there are also other things like :os/name etc

I think :version and :path are good. I think I'd want to leave the namespacing for something it might differentiate down the road. Like a different kind of version, for example. But like you said, it's already in a pods context here.

multimethod can only dispatch on namespaced keywords right?

not that that matters here (yet)

hmm... not sure. I didn't think so

Hmm... getting tripped up by one thing: in b.pods.impl/resolve-pod it checks if pod-spec is a qualified symbol and calls resolver/resolve on it if so. but for my local pod implementation, the pod-spec is still a qualified symbol, it just has a :path opt instead of a :version one. OK to change that check to (when version ...) ?

@cap10morgan One more idea. As there is a discrepancy between local pods (which is really a dev thing maybe), we could do this: Support a :namespaces [...] option in bb.edn, as a requirement for being able to use local pods.

{:pods {foo/bar {:path "../dude" :namespaces [foo.dude foo.dude2]}}}
Then there is no need to call the local pod to inspect the namespaces.

and this :namespaces option overrides whatever is in the metadata, so you can tweak it further, e.g. disable it for a certain namespace for whatever reason

where is the discrepancy for local pods? I personally would not want to have to manually maintain a separate namespaces list in there while developing a pod. especially when the same code is already 99% capable of asking the local pod for this info. I just need to iron out this resolver issue.

the discrepancy is "local describe cannot be cached"

ah, yeah

I mean, it could be but then cache invalidation gets tricky so I wanted to avoid ๐Ÿ™‚

we could support bypassing the metadata with bb --force

which is already an option to bypass the clj .cpcache file

for development

so it will be cached by default, unless --force is used

that seems reasonable

I think I still have a slight preference for just not caching it in a dev scenario. but local pods might not just be for dev, I guess. I was using one in prod before I got it into the registry. custom registries is probably the better answer there, though.

ok so not caching local pods then?

that feels simplest and lowest support burden (for you, honestly) to me

should we even support local pods in bb.edn? I don't even know how we arrived at that

it would be really nice for pod development

but you can use a local registry for that

yeah, but with a lot of the same downsides (immutable versions or you need to bust the cache, etc.) versus just reloading the in-development code as it changes

What about an option :path ... :cache false

I could see a scenario where people in the cloud have their pods in a specific place and would still like to cache the namespaces for better warm startup times

maybe :cache false is the default but you can turn it back on for local pods with :path ... :cache true?

the default should not be the development setting

yeah, makes sense. the counter-argument for me is that I like defaults for non-exasperating correctness (i.e. why isn't my new namespace getting picked up?) and knobs for perf tuning

but happy to defer to you here ๐Ÿ™‚

I think the default should be the performant option

๐Ÿ‘ 1

as in: people are going to try this on the cloud and we want them to succeed before they give up without even looking at that option ;)

back to your original issue, what was that again?

oh yes, that seems ok

maybe (and qualified-symbol version)?

๐Ÿ‘ 1

I think we should also have some tests, but if the pod tests are still passing, we could just test it from the bb side

So we have:

:pods {fq/sym {:path "/foo" :version "0.1.0" :cache false}}

yes definitely

Let me know if the pod side is ready for merge, then I'll do that first

you wouldn't have version w/ path. one or the other.

will do (still working on local pods issue)

yes, agreed, they are mutually exclusive.

ah, you were just listing the options. gotcha

This is going to be sweet :)

yeah I'm excited about it ๐Ÿ™‚

think we should allow :cache false for registry pods too?

yeah, why not

๐Ÿ‘ 1

do you think this will warrant a version bump to 0.8.0? started updating the pods README to document bb.edn pods declarations and wanted to say "As of babashka [version] you can..." I can just use a placeholder there for now if that's better.

Yeah I'm ok with 0.8.0

๐Ÿ‘ 1

where should we store the cache file for local pods? same dir as the pod binary?

or somewhere in ~/.babashka/pods ?

I think the latter would be nicer

maybe we can make a sha512 of the bytes of the pod for the filename

or just replace the file delimiter with a different char

like ~/.babashka/pods/.cache/Users$borkdude$dev$pod-foobar.cache

the hash of the pod binary itself is intriguing... solves cache invalidation and re-uses the cache even if you move it around. ๐Ÿค”

also no problems with relative path differences

do we even need the :cache param w/ that?

probably not :)

I like it! good idea!

well maybe it's still a good idea to be able to disable the cache in case we get our hashing wrong

but we can try without first

and solve that problem later

sounds good

I've got some hashing code in this library: https://github.com/nextjournal/dejavu

which in turn uses mvxcvi/alphabase

we can probably inline that code

do you want it in base58 or is base64 ok? (assuming babashka has java.util.Base64 available)

don't remember why we chose base58, maybe because it plays better with browser urls

base64 is ok with me though

provided that file delimiters aren't part of base64

yeah, I'll double check

so : , / and \\ should not be part of that set

ok the url encoder can produce safe base64

safe as in, safe file path?

on Windows and linux?

then it sounds good to me

hmm, 63 is /

ah right, URLEncoder should do the trick