Fork me on GitHub
#babashka-sci-dev
<
2023-01-24
>
lispyclouds09:01:29

ive noticed that when i use :local/root deps with relative paths in bb.edn and use it from another place via --config, the local paths are resolved relative to my pwd and not the bb.edn. is it due to deps.clj being used there and its an expected behaviour? if yes, should it be changed as we say --config <file> Replacing bb.edn with file. Relative paths are resolved relative to file.?

borkdude09:01:04

Hmm interesting issue. Might be solvable in deps.clj perhaps

borkdude09:01:38

By changing the working dir of the java process

lispyclouds09:01:40

i can take a stab at it hopefully soon

borkdude09:01:36

Logging an issue would be a good first step

lispyclouds09:01:05

i should do it in the bb repo right?

borkdude09:01:32

I wonder how it interacts with the cache, I don’t think the working dir is reflected in there as the cache is simply a hash of deps.edn

borkdude09:01:58

So that would be something to write down in the issue as well

borkdude10:01:06

deps.clj already has https://github.com/borkdude/deps.clj/blob/e9318a759628221c48b9259bc8885d4aee1d7262/src/borkdude/deps.clj#L41 so I think it would be just a matter or setting that to the correct dir

borkdude10:01:19

small change probably

borkdude10:01:43

instead of rewriting the paths after the fact here: https://github.com/babashka/babashka/blob/64dcb3335d6eb35bc8a59e36b5d6104ebd46e1fe/src/babashka/impl/deps.clj#L63 I think we can just bind the *dir* var to the deps-root directory and then it will work for both paths and local/roots

borkdude10:01:18

o wait, no paths isn't shelling out to deps.clj, so let's not touch that part

lispyclouds10:01:35

so would the user need to specify the deps-root as well here?

borkdude10:01:58

no, deps-root is set to --config by default, unless the user sets it otherwise

lispyclouds10:01:57

got it. yeah i meant should i need to explicitly specify --deps-root

borkdude10:01:34

no, just use the :deps-root like it's used in the paths handling

2
lispyclouds14:01:58

looking into the tests

borkdude14:01:56

you can run the tests locally before you push with lein do clean, test

borkdude14:01:06

also you can write a test locally before you push :)

borkdude14:01:17

you can pull the :deps-root into a let binding maybe

borkdude14:01:33

I think the problem might be that you need to bind it to an absolutized path

borkdude14:01:45

or something

lispyclouds15:01:55

good points, will get back to it soon. now my local stuff is working fine, should be better 😄

borkdude15:01:32

I'll take a look at a fix in deps.clj right now

2
borkdude15:01:49

I hope absolutizing both files is a good solution

user=> (binding [deps/*exit-fn* identity deps/*dir* ".."] (deps/-main "-M" "-e" "(System/getProperty (str 'user.dir))"))
"/Users/borkdude/dev"
nil
user=> (binding [deps/*exit-fn* identity deps/*dir* "../.."] (deps/-main "-M" "-e" "(System/getProperty (str 'user.dir))"))
"/Users/borkdude"

borkdude15:01:32

I can think of one other edge case: in Windows when you invoke the script from another disk drive

borkdude15:01:36

let me also test this...

borkdude15:01:27

yep, this should work:

user=> (binding [deps/*dir* "/C:/foo"] (deps/relativize "/D:/foo/bar"))
#object[sun.nio.fs.UnixPath 0x1d23ff23 "../../D:/foo/bar"]

borkdude15:01:48

I'm copying half of babashka.fs in there...

borkdude15:01:00

ok pushed a commit to deps.clj which should help

lispyclouds15:01:18

right, will pull in the changes

lispyclouds15:01:14

tests pass, pushed

borkdude16:01:20

• [ ] test • [ ] changelogs

lispyclouds17:01:43

i was using the test-utils/bb for a test like:

(testing "relative paths in :deps are relative to bb.edn"
  (test-utils/with-config '{:deps {babashka/process {:local/root "./process"}}}
    (is (= (str (fs/cwd) "/process/src")
           (first
             (bb "-e" "(do (require '[babashka.classpath :as cp])
                        (cp/split-classpath (cp/get-classpath)))"))))))
but the test utils passes a --deps-root as . in https://github.com/babashka/babashka/blob/master/test/babashka/test_utils.clj#L52 is this test okay as it resolves to the root of the bb repo and not the location of the bb.edn or a different util method without the --deps-root is to be used?

lispyclouds17:01:02

or i guess there is a better testing suggestion in general? 😅

lispyclouds18:01:24

also not sure if that (str (fs/cwd) "/process/src") works as expected on windows

borkdude18:01:07

this doesn't change the deps-root right, so how does this test the change that the test is for?

borkdude18:01:15

I'd look for a test that tests the deps-root feature and add another test like that but with a local/root the refers to a relative dir

2
borkdude20:01:34

I guess you're right

borkdude20:01:54

there is no test 😨

lispyclouds20:01:10

well, adding that too 😎

lispyclouds09:01:01

compared to the https://github.com/babashka/babashka/pull/1478#issuecomment-1407181862 from mac, linux has a very different error:

lein test babashka.bb-edn-test
=== deps-root-tests

Error building classpath. ../../home/lispyclouds/code/repos/babashka (Is a directory)
java.io.FileNotFoundException: ../../home/lispyclouds/code/repos/babashka (Is a directory)
        at java.base/java.io.FileInputStream.open0(Native Method)
        at java.base/java.io.FileInputStream.open(FileInputStream.java:219)
        at java.base/java.io.FileInputStream.<init>(FileInputStream.java:158)
        at java.base/java.io.FileReader.<init>(FileReader.java:75)
        at $slurp_edn.invokeStatic(io.clj:42)
        at clojure.tools.deps$slurp_edn_map$fn__664.invoke(deps.clj:39)
        at clojure.tools.deps$slurp_edn_map.invokeStatic(deps.clj:39)
        at clojure.tools.deps$slurp_deps.invokeStatic(deps.clj:96)
        at clojure.tools.deps.script.make_classpath2$read_deps.invokeStatic(make_classpath2.clj:163)
        at clojure.tools.deps.script.make_classpath2$run.invokeStatic(make_classpath2.clj:174)
        at clojure.tools.deps.script.make_classpath2$_main.invokeStatic(make_classpath2.clj:228)
        at clojure.tools.deps.script.make_classpath2$_main.doInvoke(make_classpath2.clj:195)
        at clojure.lang.RestFn.applyTo(RestFn.java:137)
        at clojure.lang.Var.applyTo(Var.java:705)
        at clojure.core$apply.invokeStatic(core.clj:667)
        at clojure.main$main_opt.invokeStatic(main.clj:514)
        at clojure.main$main_opt.invoke(main.clj:510)
        at clojure.main$main.invokeStatic(main.clj:664)
        at clojure.main$main.doInvoke(main.clj:616)
        at clojure.lang.RestFn.applyTo(RestFn.java:137)
        at clojure.lang.Var.applyTo(Var.java:705)
        at clojure.main.main(main.java:40)
Tests failed.

lispyclouds09:01:38

some level of deps-root testing is there on my PR though

borkdude09:01:49

it seems it tries to read the deps.edn file as a directory?

borkdude09:01:59

(the other way around)

lispyclouds09:01:18

its the same test im running here too, not sure if there is a better way to test this that doenst depend on where its running

borkdude09:01:43

I'm not sure what you are doing. Is this up somewhere?

lispyclouds09:01:57

im running this in the same deftest i added to the PR

lispyclouds09:01:10

didnt push it yet as it breaks

borkdude09:01:49

push it with [skip ci] and I'll have a look locally

borkdude09:01:23

I'm getting:

lein test babashka.bb-edn-test
=== deps-root-tests

Error building classpath. Local lib local/dep not found: /Users/borkdude/dev/babashka/local-dep

lispyclouds09:01:04

this error should come without my changes

borkdude10:01:54

the issue is that the tests set a default config and deps-root so I made this change in the test code:

args (cond-> args
               (and *bb-edn-path* (not (some #(= "--config" %) args)))
               (->> (list* "--config" *bb-edn-path*))
               (and *bb-edn-path* (not (some #(= "--deps-root" %) args)))
               (->> (list* "--deps-root" ".")))

2
borkdude10:01:16

but in the normal bb runs the deps-root is the parent dir of the config

lispyclouds10:01:01

which should be okay right? as long as the deps root is passed to the deps/dir with my changes?

borkdude10:01:28

I pushed a wip commit and commented out the tests that don't yet work

borkdude10:01:53

You can run that test with: lein do clean, test :only babashka.bb-edn-test/unix-task-test

borkdude10:01:13

sorry, this one: lein do clean, test :only babashka.bb-edn-test/deps-root-test

borkdude10:01:12

Now I'm also getting:

Error building classpath. Can't create directory: /private/Users/borkdude/.clojure/.cpcache
Ah, this might make sense now, maybe a thing in deps.clj where it tries to write the cache directory but it shouldn't take *dir* into account here

lispyclouds10:01:26

yeah i suppose theres gonna be differing behaviour due to this

borkdude10:01:56

I don't see any reference to *dir* here in deps.clj:

config-dir
        (or (*getenv-fn* "CLJ_CONFIG")
            (when-let [xdg-config-home (*getenv-fn* "XDG_CONFIG_HOME")]
              (.getPath (io/file xdg-config-home "clojure")))
            (.getPath (io/file (home-dir) ".clojure")))

borkdude10:01:16

no, I don't

borkdude10:01:23

user-cache-dir
          (or (*getenv-fn* "CLJ_CACHE")
              (when-let [xdg-config-home (*getenv-fn* "XDG_CACHE_HOME")]
                (.getPath (io/file xdg-config-home "clojure")))
              (.getPath (io/file config-dir ".cpcache")))

borkdude10:01:58

oh I see:

cache-dir
          (if (.exists (io/file deps-edn))
            (.getPath (io/file *dir* ".cpcache"))
            user-cache-dir)

lispyclouds10:01:13

hehe finally 😛

borkdude10:01:17

in bb.edn we set deps-file to ""

borkdude10:01:27

in order to suppress loading a local deps.edn

borkdude10:01:32

and this causes that behavior

lispyclouds10:01:29

there's quite a few knobs to be aware about

borkdude10:01:33

pushed another fix

borkdude10:01:44

Can you explain this test to me?

(testing "custom deps-root path"
        (let [out     (bb "--config" config "--deps-root" "/tmp" "cp")
              entries (cp/split-classpath out)]
          (is (= 1 (count entries)))
          (is (= "/tmp/src" (first entries)))))

borkdude10:01:44

oh I get it yes, but you have to make this more cross platform, "/tmp" doesn't work on Windows I think

borkdude10:01:49

use another tmp dir

lispyclouds10:01:23

yeah that was one of my doubts, looking at it now

borkdude10:01:54

also don't use "/foo/" string comparisons, use babashka.fs

2
borkdude10:01:19

lein test :only babashka.bb-edn-test/deps-root-test
FAIL in (deps-root-test) (bb_edn_test.clj:498)
custom deps-root path
expected: (= "/tmp/src" (first entries))
  actual: (not (= "/tmp/src" "C:\\tmp\\src"))
=== deps-race-condition-test

borkdude10:01:03

I guess my arg fixes weren't necessary since the last --deps-root argument wins

borkdude10:01:07

I'll remove those

2
borkdude10:01:41

but this test:

(testing "default deps-root path is same as bb.edn"
        (let [out     (bb "--config" config "cp")
              entries (cp/split-classpath out)]
          (is (= (fs/parent f) (fs/parent (first entries))))))
succeeds, but in the tests we always add --deps-root "." so this test isn't testing what it's supposed to

borkdude10:01:59

I would expect it to fail, considering what it's saying

borkdude10:01:47

so maybe the arg fix is necessary after all

borkdude10:01:14

oh no, this is good since we don't set *bb-edn-path* in this test, we don't use with-config

borkdude10:01:26

so these are the printed args :

("--config" "/var/folders/j9/xmjlcym958b1fr0npsp9msvh0000gn/T/4a86539f-dff2-4c9e-8c4d-f8931232a5902671065622392340497/bb.edn" "cp")
:entries ["/var/folders/j9/xmjlcym958b1fr0npsp9msvh0000gn/T/4a86539f-dff2-4c9e-8c4d-f8931232a5902671065622392340497/src"]

borkdude10:01:53

yep, all good

lispyclouds10:01:54

looks good to me

borkdude10:01:07

we could force *bb-edn-path* to nil, just to be sure

lispyclouds10:01:37

should it be set just for this test right?

borkdude10:01:30

since in this test you're spitting your own bb.edn and setting config + deps-root yourself

borkdude10:01:47

ok, it all makes sense now. when you fix the windows issue, I'll merge

lispyclouds10:01:08

we finally have more tests too

🎉 2
borkdude10:01:12

Tests are still failing after I removed the arg fixes

lispyclouds10:01:33

yes the linux error i mentioned above

lispyclouds10:01:42

you can see it in the CI

borkdude10:01:49

that's what I meant, CI

lispyclouds10:01:28

yeah i mean a not my machine specific error 😅

lispyclouds10:01:58

does it pass for you?

borkdude10:01:34

No:

Ran 213 tests containing 567 assertions.
4 failures, 0 errors.

borkdude10:01:47

ah this is something else

borkdude11:01:12

some JDK19 issue (because I switched JDK for your issue)

borkdude11:01:35

not a real issue, but error messages being different

borkdude11:01:47

and then some string comparisons fail, I will test with my regular graalvm

borkdude11:01:08

Ran 213 tests containing 567 assertions.
0 failures, 0 errors.

lispyclouds11:01:09

but if you run just this test, does it pass?

lispyclouds11:01:39

interesting, does it only fail on linux somehow then?

borkdude11:01:34

I'll make another check in deps.clj to not read deps.edn when it's not a file

2
lispyclouds11:01:17

but why is it even going to ../../home/circleci/repo?

borkdude11:01:44

dunno. maybe you can dig into this, I'm going for a run

lispyclouds11:01:04

yeah, good that i can reproduce this here

borkdude11:01:50

it might be good to insert some printlns etc in the deps.clj (use the -debug function, as out and err are captured in the tests)

lispyclouds11:01:48

i was doing it in places til borkdude.deps/deps is called.

lispyclouds11:01:33

i'll try to dig in a few hours, need to finish off some stuff!

borkdude12:01:47

ok, I returned from my run. I can try on my linux machine

lispyclouds12:01:23

some stuff i found: deps-edn is an empty string in deps clj and its being set to config-project and (relativize config-project) gives that weird path

borkdude12:01:52

I can repro on linux too

lispyclouds12:01:51

so somehow it cant find the deps.edn in the *dir*?

lispyclouds12:01:58

ah so *dir* is the dir where the local-dep dir is and that has the deps.edn. so it cant find it

lispyclouds12:01:47

not sure why its different in mac?

borkdude12:01:47

setting deps-file to an empty string is a workaround to ignore the local deps.edn

borkdude12:01:54

which should be the behavior for bb.edn

lispyclouds12:01:27

brb in some time

borkdude12:01:51

I pushed another "fix"

borkdude12:01:31

This "fix" seems to work, but it will fail if there is a "nil" file ;)

borkdude12:01:11

you can repro this with:

bb -Sdeps '{:deps {borkdude/deps.clj {:mvn/version "1.11.1.1208"}}}' -m borkdude.deps -Sdeps-file uuuuxx
vs
bb -Sdeps '{:deps {borkdude/deps.clj {:mvn/version "1.11.1.1208"}}}' -m borkdude.deps -Sdeps-file /tmp

borkdude12:01:14

I think the -Sdeps-file workaround can be entirely removed now though since we always override deps and paths

borkdude13:01:35

hmm, that makes a test fail

borkdude13:01:48

fixed it by using a file name that should not fail ;)

lispyclouds13:01:08

yay! nice!

🎉 2
lispyclouds13:01:59

checking out the "fixes" in deps

borkdude13:01:19

it wasn't "fixed" there (the latest change)

borkdude13:01:38

I just "fixed" it like this in bb:

"-Sdeps-file" "__babashka_no_deps_file__.edn"

borkdude13:01:26

__don_t_create_a_file_with_this_name_or_you_will_be_fired__

lispyclouds13:01:26

well fun stuff. im happy we found new stuff and had more tests than we started

borkdude13:01:50

yep, thanks

lispyclouds13:01:27

definitely makes me more comfortable with the codebase now