Fork me on GitHub
#babashka-sci-dev
<
2022-03-25
>
borkdude14:03:21

@cap10morgan Some feedback on the PR: I think it's worth figuring out why resources will be in the uberjar by default, other than "it seems to work". Also I think it's better to use a temporary directory to copy bb.edn into than the resources directory, as there might already be such a directory and you would be overwriting the contents.

borkdude14:03:47

babashka has its own old version of depstar built in for making uberjars, perhaps we could tweak that to include certain files as well

borkdude14:03:50

or we could move to the new tools.build uberjar code instead. it's fully compatible with bb except for the basis stuff, which we don't really need

borkdude14:03:00

the uberjar code is pretty thin

borkdude14:03:17

but let's move in the smallest step possible and just do with what we have right now

cap10morgan14:03:00

looks like in depstar it's just b/c its on the classpath: https://github.com/seancorfield/depstar/issues/27

cap10morgan14:03:13

so we could just put a temp dir on the classpath while we're building the uberjar

borkdude14:03:40

@cap10morgan yes, but resources isn't always on bb's classpath, only if you add that

cap10morgan14:03:43

I tried out the tools.build api a little but got confused by the basis stuff and how it would relate here

borkdude14:03:48

so I don't understand how it ends up working

cap10morgan14:03:20

probably just on the classpath where I tried it (e.g. in the :paths vector in my deps.edn files)

cap10morgan14:03:28

so I'll just do it the way you suggest instead

borkdude14:03:30

you only need a classpath in the end. if you have that, you don't need the basis stuff which depends on mvn etc

borkdude14:03:55

bb should not look at deps.edn, unless you tell it to

👍 1
cap10morgan14:03:58

seemed like the uber task in tools.build wanted a basis, but maybe it isn't required or it doesn't need much of it

borkdude14:03:21

yes, the uber task wants a basis but once you replace that with a classpath, the rest of the code works.

cap10morgan14:03:37

I can try converting it to that if you want

borkdude15:03:31

let's save that for another time

👍 1
borkdude15:03:38

separate issue

cap10morgan18:03:34

@borkdude for the temp dir that we copy the bb.edn into and add to the classpath while building the uberjar, you think it matters if it's inside the project root (vs. just the system's temp dir)?

cap10morgan18:03:49

it gets deleted once the jar is built

cap10morgan18:03:47

pushed the system temp dir version just now, but happy to change if you want

borkdude19:03:28

@cap10morgan I haven't tried, but won't deps from bb.edn be fetched again when you execute the uberjar, while they are already within the uberjar, when you start that uberjar? We could also write something in the uberjar's manifest about the config.

cap10morgan19:03:20

not sure, I haven't tried that either

borkdude19:03:48

Well I don't see any special handling not to do this, it seems the bb.edn is picked up as if you're not inside an uberjar

borkdude19:03:57

which should not happen

cap10morgan19:03:20

not sure I follow

cap10morgan19:03:29

like a bb.edn in the same dir as the .jar file?

cap10morgan19:03:57

yeah I saw that too

cap10morgan19:03:21

I can make it not do that

borkdude19:03:37

Also tasks etc do not really apply

borkdude19:03:54

or do they...?

borkdude19:03:03

bb uberjar.jar run task1...

borkdude19:03:54

I think we should be conservative and only copy the pods config into some uniquely named file and write something in the manifest file that pods should be loaded from there

cap10morgan19:03:39

I think the latest code in the PR already skips using a bb.edn in the same dir as the .jar file. Because of the cond. It will hit the jar branch and either find a file in the jar or not, but if it doesn't it should just return nil, not skip down to the :else where the ./bb.edn loading happens.

borkdude19:03:10

yeah that's good but the uberjar invocation will still trigger dependency downloading which should not happen anymore

borkdude19:03:22

also tasks should probably be elided from the uberjar

borkdude19:03:43

that's why I think eliding most of the bb.edn in the uberjar would be better

borkdude19:03:55

e.g. (select-keys bb.edn [:pods])

cap10morgan19:03:17

seems reasonable

borkdude19:03:17

until we have a reason to support more of it

borkdude19:03:09

in the future we could support bundling the pods with the uberjar too but this is out of scope for now ;)

cap10morgan19:03:53

hmm... it's not putting deps into the uberjar

cap10morgan19:03:17

oh, perhaps b/c if its only declared in bb.edn's :deps, I need to manually add it to my uberjar classpath? b/c clojure -Spath will only give me the deps.edn ones, huh?

borkdude19:03:54

I don't follow, why are you talking about clojure -Spath?

cap10morgan19:03:25

from the babashka book: bb -cp $(*clojure* -A:remove-clojure -Spath) uberjar foo.jar -m foo

cap10morgan19:03:41

that's how I'm getting my classpath for the uberjar I'm testing with

borkdude19:03:59

that's an example from before bb.edn

borkdude19:03:29

we should update this

cap10morgan19:03:22

ok, yeah, eliding that worked (even better)

cap10morgan19:03:53

:pods only is pushed

borkdude19:03:14

a minor thing but:

->> bb-edn-pods (hash-map :pods)
can be written as {:pods bb-edn-pods}, I find that a little easier to read

cap10morgan19:03:47

Yep, no problem

borkdude19:03:14

ok, now we should also have tests for this. One thing to note is that local sources are always resolved relative to the bb.edn file, unless you set --deps-root but I don't know how that plays out if you invoke the uberjar from another directory now, e.g.

/tmp/foo $ bb /tmp/bar/uberjar.jar  

borkdude19:03:21

I suspect that will error out unless we set --deps-root (or unset it)

borkdude19:03:34

but maybe not, I'm just not sure :)

cap10morgan19:03:17

my simple tester jar works from a different dir

borkdude19:03:28

ok, that's assuring

cap10morgan19:03:48

writing up a test now

borkdude19:03:04

Maybe this part of the code isn't hit anymore: https://github.com/babashka/babashka/blob/72ae6638422c542055dda8fc068f995cbfd9f61f/src/babashka/impl/deps.clj#L63 in the uberjar, since we cut out the deps part, so that makes sense.

cap10morgan21:03:05

ok just pushed a test. let me know if there are other aspects you'd like tested

borkdude21:03:45

cool, I'll take a look tomorrow again, thanks!

👍 1