Support clj-reload workflow

[filipesilva]

Fix #849

Before submitting a PR make sure the following things have been done:

• [x] The commits are consistent with our contribution guidelines
• [x] You've added tests to cover your change(s)
• [x] All tests are passing
• [x] You've updated the README
• [x] Middleware documentation is up to date
  • Please check out and modify the cider.nrepl ns which has all middleware documentation.
  • Run lein docs afterwards, and commit the results.

Note: If you're just starting out to hack on cider-nrepl you might find nREPL's documentation and the "Design" section of the README extremely useful.*

Thanks!

[filipesilva]

@vemv running PARSER_TARGET=parser-next make test fails with:

Running tests in #{"/Users/filipesilva/work/cider-nrepl/test/spec" "/Users/filipesilva/work/cider-nrepl/test/clj" "/Users/filipesilva/work/cider-nrepl/test/common" "/Users/filipesilva/work/cider-nrepl/test/cljs"}
Execution error (FileNotFoundException) at cider.nrepl.middleware.reload-test/eval21172$loading (reload_test.clj:1).
Could not locate clj_reload/core__init.class, clj_reload/core.clj or clj_reload/core.cljc on classpath. Please check that namespaces with dashes use underscores in the Clojure file name.

But PARSER_TARGET=parser-next make fast-test does not fail. Do you know why?

[vemv]

Thanks for applying the feedback!

I think it's a known issue with mranderson - see:

https://github.com/clojure-emacs/cider-nrepl/blob/3824d72f9ba1a14f9ce9910500db0885f6b39c23/test/clj/cider/nrepl/middleware/refresh_test.clj#L131-L136

You could perform the require dynamically, only running the deftest if (try (require 'clj-reload.core) true (catch Exception _ false)) ;; only run the deftest if inlining hasn't been performed

[filipesilva]

@vemv all the tests in cider.nrepl.middleware.reload-test depend on the init, so disabling it means it wouldn't run at all in make test... WDYT of using init from the cider.nrepl.middleware.reload ns like I have in this last commit?

[filipesilva]

That last commit seems to have sorted the cli-reload errors. But now a see two errors that I really don't understand....

FAIL in (instrument-function-call-test) (instrument_test.clj:109)
expected: #{[(System/currentTimeMillis) [3 1 1]] [(Thread/sleep 1000) [3 2]] [(- (bp (System/currentTimeMillis) {:coor [3 3 1]} (System/currentTimeMillis)) (bp start-time {:coor [3 3 2]} start-time)) [3 3]] [(System/currentTimeMillis) [3 3 1]] [(def test-fn (fn* ([] (bp (let* [start-time (bp (System/currentTimeMillis) {:coor [3 1 1]} (System/currentTimeMillis))] (bp (Thread/sleep 1000) {:coor [3 2]} (Thread/sleep 1000)) (bp (- (bp (System/currentTimeMillis) {:coor [3 3 1]} (System/currentTimeMillis)) (bp start-time {:coor [3 3 2]} start-time)) {:coor [3 3]} (- (System/currentTimeMillis) start-time))) {:coor [3]} (let [start-time (System/currentTimeMillis)] (Thread/sleep 1000) (- (System/currentTimeMillis) start-time)))))) []] [start-time [3 3 2]] [(let* [start-time (bp (System/currentTimeMillis) {:coor [3 1 1]} (System/currentTimeMillis))] (bp (Thread/sleep 1000) {:coor [3 2]} (Thread/sleep 1000)) (bp (- (bp (System/currentTimeMillis) {:coor [3 3 1]} (System/currentTimeMillis)) (bp start-time {:coor [3 3 2]} start-time)) {:coor [3 3]} (- (System/currentTimeMillis) start-time))) [3]]}
  actual: #{[(- (bp (. System currentTimeMillis) {:coor [3 3 1]} (System/currentTimeMillis)) (bp start-time {:coor [3 3 2]} start-time)) [3 3]] [(let* [start-time (bp (. System currentTimeMillis) {:coor [3 1 1]} (System/currentTimeMillis))] (bp (. Thread sleep 1000) {:coor [3 2]} (Thread/sleep 1000)) (bp (- (bp (. System currentTimeMillis) {:coor [3 3 1]} (System/currentTimeMillis)) (bp start-time {:coor [3 3 2]} start-time)) {:coor [3 3]} (- (System/currentTimeMillis) start-time))) [3]] [(. System currentTimeMillis) [3 1 1]] [(. Thread sleep 1000) [3 2]] [start-time [3 3 2]] [(. System currentTimeMillis) [3 3 1]] [(def test-fn (fn* ([] (bp (let* [start-time (bp (. System currentTimeMillis) {:coor [3 1 1]} (System/currentTimeMillis))] (bp (. Thread sleep 1000) {:coor [3 2]} (Thread/sleep 1000)) (bp (- (bp (. System currentTimeMillis) {:coor [3 3 1]} (System/currentTimeMillis)) (bp start-time {:coor [3 3 2]} start-time)) {:coor [3 3]} (- (System/currentTimeMillis) start-time))) {:coor [3]} (let [start-time (System/currentTimeMillis)] (Thread/sleep 1000) (- (System/currentTimeMillis) start-time)))))) []]}

FAIL in (inspect-var-integration-test) (inspect_test.clj:264)
rendering a var
expected: (= var-result (value (session/message {:op "eval", :inspect "true", :code "#'*assert*"})))
  actual: (not (= ("Class" ": " (:value "clojure.lang.Var" 0) (:newline) "Value: " (:value "true" 1) (:newline) (:newline) "--- Meta Information:" (:newline) "  " (:value ":ns" 2) " = " (:value "clojure.core" 3) (:newline) "  " (:value ":name" 4) " = " (:value "*assert*" 5) (:newline) (:newline) "--- Datafy:" (:newline) "  " "0" ". " (:value "true" 6) (:newline)) ("Class" ": " (:value "clojure.lang.Var" 0) (:newline) "Value: " (:value "true" 1) (:newline) (:newline) "--- Meta Information:" (:newline) "  " (:value ":added" 2) " = " (:value "\"1.0\"" 3) (:newline) "  " (:value ":ns" 4) " = " (:value "clojure.core" 5) (:newline) "  " (:value ":name" 6) " = " (:value "*assert*" 7) (:newline) "  " (:value ":doc" 8) " = " (:value "\"When set to logical false, 'assert' will omit assertion checks in\\n  compiled code. Defaults to true.\"" 9) (:newline) (:newline) "--- Datafy:" (:newline) "  " "0" ". " (:value "true" 10) (:newline))))

I didn't touch or I think interact with these ns's in any way, do you have any idea of what's happening?

[vemv]

Yes, feel free to disregard those - we can fix them before we cut a stable release.

Is there anything pending from your side?

[filipesilva]

Should I do any changes to the README, as the PR template mentions?

I'd like to test locally a bit to make sure everything is fine, but hitting a failure... when I run

# Install the project in your local ~/.m2 directory, using mranderson (recommended)
# The JVM flag is a temporary workaround.
export LEIN_JVM_OPTS="-Dmranderson.internal.no-parallelism=true"
PROJECT_VERSION=0.45.0-reload make install

it fails with this message:

...
prefixing #{"difflib"} in target/class-deps.jar with cidernrepl0450reload
deleting directories with class files in target/srcdeps...
   difflib  deleted
unzipping repackaged class-deps.jar into target/srcdeps
touch .inline-deps
rm -f .no-mranderson
touch .no-pedantic
lein with-profile -user,-dev,+1.11,+plugin.mranderson/config install
Error: Could not find or load main class clojure.main
Caused by: java.lang.ClassNotFoundException: clojure.main
Compilation failed: Subprocess failed (exit code: 1)
Error encountered performing task 'install' with profile(s): 'base,system,provided,1.11,config'
Compilation failed: Subprocess failed (exit code: 1)
make: *** [install] Error 1

Am I doing something wrong? If there's a better way to test locally let me know.

[vemv]

That looks like an occurrence of https://github.com/clojure-emacs/cider-nrepl/issues/848 - please try with a slightly older Lein?

[filipesilva]

Yeap that worked!

... but I'm at a bit of a loss of how to test it now. I mean CIDER won't have this operation, right? What should I do to test it in CIDER?

[filipesilva]

Well if there's nothing more that I can test at this point, I don't think there's anything pending on my side. Just pushed the README update.

[vemv]

Happy that you got it!

After a successful make install, you could fork cider https://docs.cider.mx/cider/contributing/hacking.html so that the new ops are used insted of the tools.namespace ones.

There should be a few defcustoms e.g. cider-refresh-op, cider-refresh-all-op.

Should be a small change overall.

It also should be good timing to discuss upstream whether a clear function op would make sense for clj-reload. Else we'd have to void that op somehow, to avoid surprises.

[filipesilva]

What's the difference between the clear op and the cider-refresh-all-op?

I tried to setup CIDER locally but get a native compile error while doing eldev build :autoloads. I tried something different though: I went into nrepl.clj, changed (def-wrapper wrap-refresh cider.nrepl.middleware.refresh/handle-refresh to use cider.nrepl.middleware.reload/handle-reload instead, update handle-reload to handle the refresh op, make install, then load it in a project manually instead of via jack-in.

I could see cider-ns-refresh working and logging messages Reloading successful, but the current refresh functionality logs this message anyway. There's a cider-ns-reload-all but that doesn't actually seem related to the cider-ns-refresh, at least from the doc. I don't think refresh-all or refresh-clear are currently exposed as CIDER commands.

This is the test I used:

(def foo 1)

(defonce bar 5)

(comment
  foo
  bar)

I saved, reloaded, and got eval'ed foo and bar in the comment block. Then altered their values, reloaded again, and saw foo update the value but bar remained. This is a different in cli-reload than in clojure.tools.namespace.repl: the former keeps defonce, but the latter does not. So I think this updated cider-ns-refresh is working with cli-reload. I double checked this test and with release version of CIDER the refresh does indeed update the value of the defonce var.

[filipesilva]

What are you thoughts on using the new operation from within CIDER by the way? I guessed that a new command would be added, cider-ns-reload would be added, but there's already a cider-ns-reload. Maybe cider-ns-clj-reload?

[vemv]

Hi Filipe!

What's the difference between the clear op and the cider-refresh-all-op?

In general, clear unloads all code, without trying to load it again. This is useful in case something temporarily broke - then one can clear, fix the code, load it by hand, ensure it works, then refresh to load the rest of the proect.

I don't think refresh-all or refresh-clear are currently exposed as CIDER commands.

Looks like they are: https://github.com/clojure-emacs/cider/blob/dc58ed1a411e6fe6f0350435aa4c56ec6edf59bf/cider-ns.el#L257-L261

What are you thoughts on using the new operation from within CIDER by the way? I guessed that a new command would be added

I'd prefer if the defuns/commands remained the same. Instead, the ops would be customizable via defcustoms.

Cheers - V

[filipesilva]

Ok so what's next? I don't think there's anything left for me to do in this PR, or is there?

[vemv]

I'd like to know if there's an intention from clj-reload's side to offer clear functionality.

In the meantime, WIP PR welcome for https://github.com/clojure-emacs/cider - let us know if it's working as intended, using the suggested defcustom design.

[filipesilva]

I see now all the refresh nrepl operations are within the same cider-ns-refresh, that's what I missed.

It also makes a lot more sense why you'd like clear to also exist, that way the semantics of the operation are maintained.

But I don't see an exposed unload operation in clj-reload, it's not part of the API I think.

Maybe we could leave that one as not yet implemented and the interface would still be the same?

[filipesilva]

@tonsky what are your thoughts about a clear/unload operation?

[tonsky]

In general, clear unloads all code, without trying to load it again. This is useful in case something temporarily broke - then one can clear, fix the code, load it by hand, ensure it works, then refresh to load the rest of the proect.

This? We can do this, sure. Send a PR or I’ll do it myself next week

[filipesilva]

@vemv early draft of the cider changes in https://github.com/clojure-emacs/cider/pull/3624, PTAL if it matches what you had in mind.

[tonsky]

Here you go https://github.com/tonsky/clj-reload/releases/tag/0.4.0

[vemv]

Thanks much, @tonsky 🎉!

@filipesilva , if you merge master in the build will become green.

Be sure to add clear, test it out locally, refresh docs and we're good to go.

[bbatsov]

Yeah, it'd be nice to explain both here and in CIDER's docs (later) why are there 2 instances of relatively similar functionality. I'm guessing if clj-reload works well down the road we can remove c.t.n. as I don't think we need both of them in the long run and I like that clj-reload is smaller in terms of scope.

[vemv]

tbh that would be one big breaking change :) it's easy to imagine a variety of commercial projects that have set up the t.n boilerplate, so using t.n would be a natural fit.

Ultimately, let's see how it all plays out over the years - I reckon that only one of both projects will remain actively maintained / used. It's pretty early to bet on either.

[bbatsov]

tbh that would be one big breaking change :) it's easy to imagine a variety of commercial projects that have set up the t.n boilerplate, so using t.n would be a natural fit.

Yeah, I'm well aware of this. I don't see this happening soon, but I also dislike functionality duplication. (it tends to be confusing for the end users and it's more maintenance work for us) Now CIDER will have 3 ways to do code reloading, so can understand my desire for this not to be a permanent situation. Anyways, that's not particularly important for this PR.

[filipesilva]

Added clear (unload in clj-reload) support, rebase, and incorporated @bbatsov's feedback.

Edit: also added a bit more about why you might want cld-reload to the cider docs in the other PR.

[vemv]

Great work, thanks!

FYI a changelog entry was missing - no biggie, adding it now

[cichli]

In general, clear unloads all code, without trying to load it again. This is useful in case something temporarily broke - then one can clear, fix the code, load it by hand, ensure it works, then refresh to load the rest of the proect.

clear just clears the state of the refresh tracker – see the doc for clojure.tools.namespace.repl/clear – it forces the next refresh to load everything, but doesn't unload anything itself. It's for recovering from corrupted states (normally caused by deleted files or circular dependencies). I think unloading is generally more useful and easier to understand, but we could maybe consider aliasing it to unload in the new middleware (sorry this reply is too late to suggest renaming it).

[bbatsov]

@cichli Might be good to file this as a ticket, so the suggestion to add an alias won't be forgotten.