Clojurians Log v2

@tonsky with bb 1.12.209 out, I think https://github.com/tonsky/clojure-plus/pull/20 is ready for review. It adds bb support to all ns except print.

think I found another place where clj-reload works oddly with bb, test/clojure+/walk_test.cljc has a (defrecord Foo [a b c]), and when reloading it on bb I see this error

Reloading 13 namespaces...
  failed to load clojure+.walk-test #error {
 :cause Could not resolve symbol: clojure_PLUS_.walk_test.Foo
 :data {:type :sci/error, :line 84, :column 1, :file clojure+/walk_test.cljc, :phase analysis}
 :via
 [{:type clojure.lang.ExceptionInfo
   :message Could not resolve symbol: clojure_PLUS_.walk_test.Foo
   :data {:type :sci/error, :line 84, :column 1, :file clojure+/walk_test.cljc, :phase analysis}
   :at [sci.impl.utils$throw_error_with_location invokeStatic utils.cljc 49]}]
 :trace
 [[sci.impl.utils$throw_error_with_location invokeStatic utils.cljc 49]
  [sci.impl.resolve$throw_error_with_location invokeStatic resolve.cljc 11]
  [sci.impl.resolve$resolve_symbol invokeStatic resolve.cljc 281]

isn't that just this issue? https://github.com/babashka/babashka/issues/1839

I think that issue only happened because of the ^:clj-reload/keep metadata, but let me double check

with "that issue" do you mean the issue you posted or 1839?

1839

the underlying issue is the same. it is because of this:

(ns foo)
(defrecord Dude [])
(ns bar)
(remove-ns 'foo)
foo.Dude ;; still exists

I see!

lol. who uses a plus in their ns anyway :P but yes, this is a nice isolated repro worthy of its own github issue, thank you

I can also confirm that if I go into the clj-reload tests and add a defrecord with ^:clj-reload/keep it will fail to reload, but if I remove the metadata it will not

will make issue

thanks!

https://github.com/babashka/babashka/issues/1868

ok here's a new wip for clojure+ bb support: https://github.com/tonsky/clojure-plus/compare/main...filipesilva:clojure-plus:support-bb • print has a lot of java imports that bb doesn't have, unclear if it's worth even trying that one • print, error, and test need (.addMethod ^MultiFn ...) • walk I think is working but the test fails to load because of 1868 • that leaves hashp and core working and tested

why does it need addMethod instead of just calling defmethod

we should also add clojure.lang.ITransientCollection" . the string solution isn't a nice one

please make an issue for that

I'm willing to change the message "Could not" to "Unable" in Unable to resolve symbol please also an issue for that

I think it uses .addMethod because the code keeps the replacement and original fns, then the uninstall restores the original one

(defonce ^:private clojure-print-method
  (get-method print-method Throwable))

(defn- patched-print-method [^Throwable t ^Writer w]
  (let [t (if (:root-cause-only? config)
            (root-cause t)
            t)]
    (if *print-readably*
      (if (:reverse? config)
        (print-readably-reverse w t)
        (print-readably w t))
      (if (:reverse? config)
        (print-humanly-reverse w t)
        (print-humanly w t)))))

(defn install!
  "Improves the way exceptions are printed, including print*, pr*,
   and clojure.pprint/pprint.

   Possible options:

     :clean?           <bool> :: Converts Clojure-specific stack trace elements
                                 to be more clojure-like. True by default.
     :collapse-common? <bool> :: With chained exceptions, skips common part of
                                 stack traces. True by default.
     :trace-transform  <fn>   :: A fn accepting and returning trace--a sequence
                                 of maps {:keys [element file line ns method]}
                                 where element is original StackTraceElement.
     :color?           <bool> :: Whether to use color output. Autodetect by default.
     :reverse?         <bool> :: Whether to print stack trace and cause chain
                                 inner-to-outer (Java default) or outer-to-inner.
                                 Useful for REPL and small terminals as class,
                                 message and source will always be at the bottom.
                                 False by default.
     :root-cause-only? <bool> :: Only print root cause. False by default.
     :indent           <int> | <string> :: how many spaces to use to indent
                                 stack trace elements. Java uses \"\\t\", we use
                                 2 spaces by default."
  ([]
   (install! {}))
  ([opts]
   (alter-var-root #'config (constantly (merge (default-config) opts)))
   (.addMethod ^MultiFn print-method Throwable patched-print-method)))

(defn uninstall!
  "Restore default Clojure printer for Throwable"
  []
  (.addMethod ^MultiFn print-method Throwable clojure-print-method))

in error there's just the single one, but in print and test there's more

print also does it dynamically

(defn install-printers!
  "Install printers for most of Clojure built-in data structures.
   
   After running this, things like atoms and transients will print like this:
   
     (atom 123)          ; => #atom 123
     (transient [1 2 3]) ; => #transient [1 2 3]
   
   Possible opts:
   
     :include :: [sym ...] - list of tags to include (white list)
     :exclude :: [sym ...] - list of tags to exclude (black list)"
  ([]
   (install-printers! {}))
  ([opts]
   (let [catalogue (catalogue opts)]
     (doseq [{:keys [class print]} catalogue]
       (.addMethod ^MultiFn print-method class print)
       (.addMethod ^MultiFn print-dup class print)
       (.addMethod ^MultiFn pprint/simple-dispatch class #(print % *out*))))))

I think you can just use defmethod for this really.

user=> (macroexpand '(defmethod foo bar baz))
(. foo clojure.core/addMethod bar (clojure.core/fn baz))

but maybe it's slightly more annoying

ok, issue for .addMethod welcome too

I think the harder bit is getting the arg list from the fn, whereas with .addMethod you just provide the fn instead of args+fn

making issues then

sounds good!

the addMethod can be solved here: https://github.com/babashka/babashka/blob/e4ee490d5a96b38437959547a7f0ba44d44b580c/src/babashka/impl/classes.clj#L128-L130

adding classes: same file. Please one PR per issue and add a test

tests can be added in babashka.interop-test

(of course: patches are optional)

awesome, I was going to ask if I could these myself lol

always been curious about contributing to bb but it seemed a bit daunting

contributing to classes.clj is the easiest for getting started :)

https://github.com/babashka/babashka/blob/master/doc/dev.md#requirements

and: https://github.com/babashka/babashka/blob/master/doc/build.md

add ITransientCollection to :instance-checks - this will have the least impact on binary size

no need to write a test for that if it's only about (instance? ... x)

but .addMethod does need a test

issues: • https://github.com/babashka/babashka/issues/1871 • https://github.com/babashka/babashka/issues/1870 • https://github.com/babashka/babashka/issues/1869

👍

ah I forgot to mention why I removed some assertions from the hashp test:

(core/if-clojure-version-gte "1.12.0"
        (do
          (is (= "#p String/1 [<pos>]\njava.lang.String/1\n"                              (:out (eval "#p String/1"))))
          (when-not util/bb?
            (is (= "#p StringWriter/1 [<pos>]\njava.io.StringWriter/1\n"                  (:out (eval "#p StringWriter/1")))))))

ERROR in (interop-test) (clojure+/hashp_test.clj:117)
classes
expected: (= "#p StringWriter/1 [<pos>]\njava.io.StringWriter/1\n" (:out (eval "#p StringWriter/1")))
  actual: org.graalvm.nativeimage.MissingReflectionRegistrationError: The program tried to reflectively instantiate the array class

   java.io.StringWriter[]

without it being registered for runtime reflection. Add java.io.StringWriter[] to the reflection metadata to solve this problem. Note: Add "unsafeAllocated" to the array class registration to enable runtime instantiation. See  for help.
 at com.oracle.svm.core.reflect.MissingReflectionRegistrationUtils.errorForArray (MissingReflectionRegistrationUtils.java:121)

(when-not util/bb?
      (testing "instance fields"
        (is (= {:res 1 :out "#p (.-x (java.awt.Point. 1 2)) [<pos>]\n1\n"}  (eval "#p (.-x (java.awt.Point. 1 2))")))
        (is (= {:res 1 :out "#p (.x (java.awt.Point. 1 2)) [<pos>]\n1\n"}   (eval "#p (.x (java.awt.Point. 1 2))")))
        (is (= {:res 1 :out "#p (. (java.awt.Point. 1 2) -x) [<pos>]\n1\n"} (eval "#p (. (java.awt.Point. 1 2) -x)")))
        (is (= {:res 1 :out "#p (. (java.awt.Point. 1 2) x) [<pos>]\n1\n"}  (eval "#p (. (java.awt.Point. 1 2) x)")))))

I'll take a stab and making patches for the other issues a bit later, thanks for taking the time to give me directions there

Heya, about the test in https://github.com/babashka/babashka/pull/1873... I'm not sure if it's in the right place, but there were similar tests in that ns so I did it there. If I should put the test somewhere else please let me know.

looks great to me

heya, using master bb I get this diff now https://github.com/tonsky/clojure-plus/compare/main...filipesilva:clojure-plus:support-bb

basically everything except print works

walk works, except the test needs that defrecord

test works, but test is excluded from the normal tests, so I tested it manually

now error works as well, which it didn't before

I'm gonna take a stab at getting print to work with a lot of conditionals for all the types that it tries to import but aren't in bb

here's a super weird one that came up when I got print working https://github.com/babashka/babashka/issues/1874

welp, the more I drill into print, the more issues I get... I don't think this one is worth it. latest issue is Method getName on class sci.lang.Namespace not allowed! coming up on loads of tests

Just use ns-name

Why use all this JAva interop when there’s core functions for them

dunno, I'm mostly trying to change as little as possible

Well that’s a change that would be fine I think. I’ll get back to the rest, afk at the moment

put up a draft PR in https://github.com/tonsky/clojure-plus/pull/19 with base support, which looks pretty solid, and a separate commit with clojure+.print support, which I think isn't very solid

You can perhaps use fs/modified-since for this

You give it one or a coll of files to compare to and then a directory and it will return the newer files

Last modified

Or you can give it a collection of files with fs/glob to find only the Java files

Thanks! Yes, fs/modified-since had come to mind. Was just wondering if anyone has the perfect snippet at hand. Otherwise yes, using bb.fs should give plenty of options

I've also written a blog post about it here: https://blog.michielborkent.nl/speeding-up-builds-fs-modified-since.html

I kinda regret making fs/modified-since lazy so you have to call seq on it to get nil if nothing has changed, but whatever

and here's an example:

$ bb -e '(fs/modified-since "target" (fs/glob "." "**.java"))'
()
$ touch impl-java/src-java/babashka/impl/URLClassLoader.java
$ bb -e '(fs/modified-since "target" (fs/glob "." "**.java"))'
(#object[sun.nio.fs.UnixPath 0x77e29668 "impl-java/src-java/babashka/impl/URLClassLoader.java"])

Nice. So the final invocation may be as simple as approximately:

:init 
(do
  (when (seq (fs/modified-since "target" "../foo/java/src/**.java"))
    ;; delete target/ so that prepping will be forcibly run...
    )
  (clojure "-X:deps prep"))

yep

but this will always invoke clojure -X:deps prep, maybe you can prevent that when nothing has changed?

anyway, that's for you to decide

Good call!

Although yeah, in a monorepo there can be N other siblings, so better not to optimize

is this for prepping the current project btw? then you might need :current true

but if you use it as a dep, then not

It's prepping siblings

right

note that this will need a glob call:

(fs/modified-since "target" "../foo/java/src/**.java")

(fs/modified-since "target" (fs/glob "../foo/src" "**.java"))