Support cider.clj-reload/reload ops #3624

filipesilva · 2024-03-02T22:55:08Z

See clojure-emacs/cider-nrepl#850 for more.

Notes:

this ns only had one trivial test, I didn't add more
listing shows existing linting errors, I made sure I wasn't adding more

Replace this placeholder text with a summary of the changes in your PR.
The more detailed you are, the better.

Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

The commits are consistent with our contribution guidelines
You've added tests (if possible) to cover your change(s)
All tests are passing (eldev test)
All code passes the linter (eldev lint) which is based on elisp-lint and includes
- byte-compilation, checkdoc, check-declare, packaging metadata, indentation, and trailing whitespace checks.
You've updated the changelog (if adding/changing user-visible functionality)
You've updated the user manual (if adding/changing user-visible functionality)

Thanks!

If you're just starting out to hack on CIDER you might find this section of its
manual extremely useful.

filipesilva · 2024-03-02T22:55:44Z

@vemv just to get the ball rolling, is this roughly what you had in mind?

vemv

Looks reasonable to me - thanks!

cider-ns.el

filipesilva · 2024-03-03T09:32:31Z

Btw have you seen this eldev error? It happens on build and test:

[I] filipesilva@Filipes-MacBook-Pro ~/s/cider (clj-reload) [1]> eldev test
Native compiler error: (lambda (arg0 &rest arg1) (let ((f #'message)) (apply f arg0 arg1))), "Compiling /Users/filipesilva/.emacs.d/eln-cache/29.1-272591eb/subr--trampoline-6d657373616765_message_0.eln...
-macosx_version_min has been renamed to -macos_version_min
ld: library 'emutls_w' not found
libgccjit.so: error: error invoking gcc driver
Internal native compiler error: \"failed to compile\", \"/Users/filipesilva/.emacs.d/eln-cache/29.1-272591eb/subr--trampoline-6d657373616765_message_0.eln\", \"error invoking gcc driver\"

Error: native-ice (\"failed to compile\" \"/Users/filipesilva/.emacs.d/eln-cache/29.1-272591eb/subr--trampoline-6d657373616765_message_0.eln\" \"error invoking gcc driver\")
  mapbacktrace(#f(compiled-function (evald func args flags) #<bytecode -0xc883f0734512f81>))
  debug-early-backtrace()
  debug-early(error (native-ice \"failed to compile\" \"/Users/filipesilva/.emacs.d/eln-cache/29.1-272591eb/subr--trampoline-6d657373616765_message_0.eln\" \"error invoking gcc driver\"))
  comp--compile-ctxt-to-file(\"/Users/filipesilva/.emacs.d/eln-cache/29.1-272591eb/subr--trampoline-6d657373616765_message_0.eln\")
  comp-compile-ctxt-to-file(\"/Users/filipesilva/.emacs.d/eln-cache/29.1-272591eb/subr--trampoline-6d657373616765_message_0.eln\")
  comp-final1()
  load-with-code-conversion(\"/private/var/folders/hx/dl3d0pqn6615z78g83gc26300000gn/T/emacs-int-comp-subr--trampoline-6d657373616765_message_0-re9NpE.el\" \"/private/var/folders/hx/dl3d0pqn6615z78g83gc26300000gn/T/emacs-int-comp-subr--trampoline-6d657373616765_message_0-re9NpE.el\" nil t)
  command-line-1((\"-l\" \"/var/folders/hx/dl3d0pqn6615z78g83gc26300000gn/T/emacs-int-comp-subr--trampoline-6d657373616765_message_0-re9NpE.el\"))
  command-line()
  normal-top-level()
"

filipesilva · 2024-03-03T10:16:55Z

If anyone else comes across that error, reinstalling emacs from homebrew seemed to fix it.

filipesilva · 2024-03-03T15:13:24Z

@vemv was able to get a local dev env working, debugged the feature, and I believe this PR is ready to be reviewed. I left support for the clear operation, but will continue discussion of it in clojure-emacs/cider-nrepl#850 instead.

cider-ns.el

filipesilva · 2024-03-03T15:23:23Z

cider-ns.el

@@ -148,6 +156,9 @@ namespace-qualified function of zero arity."
       (reloading
        (log-echo (format "Reloading %s\n" reloading) 'font-lock-string-face))

+       (progress
+        (log-echo progress 'font-lock-string-face))


The progress key is a passthrough of the logging cli-reload outputs. It doesn't give you a list of files being reloaded per se, just logs as they are individually unloaded/reloaded. Examples:

cider-ns-refresh: Nothing to reload cider-ns-refresh: Reloading successful

cider-ns-refresh: Unloading scratch.scratch cider-ns-refresh: Loading scratch.scratch cider-ns-refresh: Reloading successful

This seems nice. However for large projects it might get excessively noisy?

I'd suggest a defcustom, default "no logging" (since one isn't debugging stuff by default)

I can see how this can get very spammy if you're looking at the *messages* buffer, and agree it should be good to prevent that spam.

I worry preventing it might be premature though... it is adding another customisation, and I imagine this feature might not have a lot of users for a while. Perhaps it could prove unnecessary.

I defer to your opinion though. I don't look at *messages* a lot. Maybe spam there is a big issue.

WRT the default of no logging, there's some tension there. Reloading, especially on larger projects, can take a while. Seeing the progress is helpful, and tools.namespace not showing the progress is actually to its detriment.

So larger projects, which are the ones where the progress report would be more spammy, are also the ones that care more about the current progress.

CIDER generally uses spinner for progress indication needs.

(it's an item that popped up in a recent PR)

It could be added at some point. Please do not feel pressured to do so, but it's what I'd expect before the "spammy" default :)

I spinner would be quite easy to add indeed. I agree that the extra logging should probably be optional (and maybe displayed in a standalone buffer one can quickly discard). Not a big deal at this stage, though.

Btw, there's already cider-ns-refresh-show-log-buffer, which I guess it makes sure to reuse if possible.

And because we're already using log-echo it seems we don't have to worry much about spamming Messages here:

(log-echo (message &optional face) (log message face) (unless cider-ns-refresh-show-log-buffer (let ((message-truncate-lines t)) (message "cider-ns-refresh: %s" message))))

I'm sorry, I'm not sure what the end result is here. Is it that I should add a spinner, or hat I shouldn't do anything since cider-ns-refresh-show-log-buffer already exists?

It seems to me you don't need to do anything, as those logs will end in the dedicated buffer (when enabled).

We can discuss the need for adding a spinner down the road.

vemv

LGTM!

Please let's wait till the clear op is there.

That also gives you the opportunity to QA the feature for a few days - I tend to suggest that as it can easily uncover the need for fixes/refinements.

Cheers - V

cider-ns.el

vemv · 2024-03-03T18:36:33Z

cider-ns.el

@@ -148,6 +156,9 @@ namespace-qualified function of zero arity."
       (reloading
        (log-echo (format "Reloading %s\n" reloading) 'font-lock-string-face))

+       (progress
+        (log-echo progress 'font-lock-string-face))


This seems nice. However for large projects it might get excessively noisy?

I'd suggest a defcustom, default "no logging" (since one isn't debugging stuff by default)

bbatsov · 2024-03-04T22:24:52Z

cider-ns.el

@@ -186,6 +197,19 @@ Its behavior is controlled by `cider-ns-save-files-on-refresh' and
                          (file-in-directory-p buffer-file-name dir))
                        dirs)))))))

+(defun cider-ns-refresh--refresh-op (op-name)


I'd name this just cider-ns--refresh-op. Although I'm not fully sold on the current approach, as I find it weird to use one set of ops as baseline instead of something more generic. Also - the "refresh" name comes from a function with this name in c.t.n..

I get that the current approach will likely result in the least amount of code changes, but I still want to think about this a bit more.

I now like cider-ns--reload-op even more, as I guess we'll move away from the "refresh" naming at some point. So maybe you can name the clj-reload op names (sans the namespace) the "default" here and calculate the "refresh" ops in terms of them. Something like (cider-ns--reload-op 'reload).

I think I effected the changes as you indicated, but please let me know if understood it wrong.

Yep, it did exactly what I had in mind.

cider-ns.el

bbatsov · 2024-03-04T22:29:53Z

doc/modules/ROOT/pages/usage/misc_features.adoc

@@ -130,6 +130,13 @@ and `cider-ns-reload-all` commands can be used instead. These commands
 invoke Clojure's `+(require ... :reload)+` and `+(require
 ... :reload-all)+` commands at the REPL.

+You can also use https://github.com/tonsky/clj-reload[clj-reload] instead:


Hmm, won't there be any configuration differences that should be noted here? The way it's written one would expect that changing the "refresh" tool will be mostly transparent action and I'm not sure that's really the case.

Yes, please indicate that one should generally (reload/init {:dirs dirs}) and that cider won't do that for users

(I'm aware that that became optional, but that was a very recent change and easily a suboptimal one, based on t.n experience)

I'm happy to describe how users should add the init first but I don't see why that would come up as a difference. I don't see any direct mention of the initialisation procedure for tools.namespace either, although it seems to be implied, but also not necessary.

Another difference is that cider-ns-refresh-before-fn and cider-ns-refresh-after-fn are not used anymore, but cli-reload supports unload hooks on individual namespaces.

I don't see any direct mention of the initialisation procedure for tools.namespace either,

I added

cider/cider-ns.el

Lines 258 to 260 in 23540fd

(defun cider-ns-refresh (&optional mode)

"Reload modified and unloaded namespaces, using the Reloaded Workflow.

Uses the configured 'refresh dirs' \(defaults to the classpath dirs).

this week

Another difference is that cider-ns-refresh-before-fn and cider-ns-refresh-after-fn are not used anymore, but cli-reload supports unload hooks on individual namespaces.

We should pay attention to this. resetting a system is a fundamental step when working when any commercial app.

I'd suggest that it's discussed + tackled in a follow-up PR?

cider-ns.el

vemv · 2024-03-05T04:54:59Z

@filipesilva : following #3625 you may rebase master in to get cider-nrepl with your changes in 🎉

vemv · 2024-03-06T04:27:52Z

...and following #3627 you should should see jumping to the culprit file/line on errors.

(No backend changes were needed at all)

Please let us know if it works here as well.

See clojure-emacs/cider-nrepl#850 for more.

filipesilva · 2024-03-09T14:03:48Z

@vemv @bbatsov I've pushed changes that address your review items. The review items that were trivial to incorporate I marked as resolved to better keep track of things.

bbatsov · 2024-03-09T14:05:28Z

cider-ns.el

                 (const :tag "clj-reload https://github.com/tonsky/clj-reload" clj-reload))
-  :package-version '(cider . "1.13.1"))
+  :package-version '(cider . "1.40.0"))


Whoops, fixed!

vemv · 2024-03-09T14:26:30Z

@filipesilva : a very common .dir-locals.el is some variation of:

((nil . ((cider-ns-refresh-before-fn . "integrant.repl/suspend")
         (cider-ns-refresh-after-fn  . "integrant.repl/resume")
,,,

So that reloaded changes can actually affect an app.

I'd be OK with merging this asap but please confirm if you'd intend to further refine the solution

filipesilva · 2024-03-09T15:16:26Z

@vemv I'm not really thinking of adding that extra functionality, no. That looks like it should be part of base functionality for clj-reload, not something that needs CIDER to work. It might already work just with unload hooks, but I'm not sure. @tonsky what are your thoughts on this?

filipesilva · 2024-03-09T15:17:23Z

Meanwhile I found a bug where reload-all isn't working because I passed the wrong option in cider-nrepl. I'm working on fixing it and making the tests more robust.

filipesilva · 2024-03-09T15:44:33Z

clojure-emacs/cider-nrepl#854 is the fix for the reload-all op.

tonsky · 2024-03-09T21:43:13Z

@vemv I'm not really thinking of adding that extra functionality, no. That looks like it should be part of base functionality for clj-reload, not something that needs CIDER to work. It might already work just with unload hooks, but I'm not sure. @tonsky what are your thoughts on this?

I think this is just calling a function immediately before clj-reload.core/reload and immediately after. What kind of support are you thinking? Can clj-reload really do something here?

vemv · 2024-03-09T22:06:22Z

Thanks!

While t.n supports an :after fn, in cider-nrepl, (somewhat surprisingly) we invoke the functions ourselves.

So yeah no upstream changes are required.

vemv · 2024-03-10T19:24:22Z

I'm squash-merging this separately as we speak.

Thanks much for your work @filipesilva !

vemv · 2024-03-10T19:27:57Z

It's in master now

c4fa1a8

A melpa snapshot will be automatically created later today.

We'll cut a stable release soon enough.

Cheers - V

filipesilva · 2024-03-12T14:40:20Z

Awesome, thank you so much!

filipesilva · 2024-03-24T23:59:28Z

Just following up on the "reloaded changes can actually affect an app" case.

I can verify I can add this fn in a given ns:

(defn before-ns-unload []
  (println "stopping processes")

(defn after-ns-reload []
  (println "restarting processes")

And it is running before/after the reload, both when I call reload manually and when I call it via cider. I just used it to restart a server in an app of mine I've moved to clj-reload. So clj-reload can handle this use case via its hooks.

vemv · 2024-03-25T08:08:25Z

Thanks!

Anyway, cider-ns-refresh-before-fn / cider-ns-refresh-after-fn are now supported, so it should work as well.

cichli · 2024-05-10T21:56:05Z

While t.n supports an :after fn, in cider-nrepl, (somewhat surprisingly) we invoke the functions ourselves.

The middleware interleaves responses such as {:status :ok} / {:status :invoking-after} / {:status :invoked-after} with the execution of the refresh and invoking :after – if we used the t.n option, we wouldn't be able to do that.

filipesilva mentioned this pull request Mar 2, 2024

Support clj-reload workflow clojure-emacs/cider-nrepl#850

Merged

5 tasks

vemv reviewed Mar 3, 2024

View reviewed changes