danielsz / system

Reloaded components à la carte
608 stars 77 forks source link

Boot/tools.namespace integration doesn't work out of the box #79

Closed gtrak closed 8 years ago

gtrak commented 8 years ago

The boot task doesn't set up tools.namespace unloading properly, instead it just reloads the affected namespaces manually using data from ns-tracker and require :reload. If I tried to use tools.namespace.repl/refresh manually, my system would be referencing non-existent vars and it would break further eval.

I worked around it locally in my build.boot script with this combo, but this seems like an issue with both the library and documentation.

(defn system-init
  "Has to be dynamic because tns unloads vars"
  []
  ((resolve 'myapp/system)
   {:port 7979}))

(defn setup-tns
  "the 'system' boot task doesn't automatically unload namespaces, so we configure 
tools.namespace here according to https://github.com/boot-clj/boot/wiki/Repl-reloading"
  []
  (apply tns-repl/set-refresh-dirs (get-env :directories)))

(deftask dev
  "Run a non-autoreload system with a REPL"
  []
  (setup-tns)
  (reloaded/set-init! system-init)
  (repl))

This is also a problem with the system task, which calls reloaded.repl itself, which transitively calls tools.namespace. I have to set up tools.namespace dirs and use a dynamic resolve in my init function.


(deftask dev-watch
  "Run a restartable system with a REPL"
  []
  (setup-tns)
  (comp
   (watch :verbose true)
   ;; :files vector will control reloading of the system state
   (system :sys system-init :auto true :files ["handler.clj" "html.clj"])
   ;; Can't launch REPL inline because it'll break watch
   (repl :server true)))

I think the tools.namespace setup can happen in the task, which shouldn't use ns-tracker, and the dynamic resolution hack should be documented somewhere, probably reloaded.repl.

gtrak commented 8 years ago

Just to try to make the motivation more clear, the use-case I'm working towards is to have both an auto-refreshing dev environment for newcomers to the codebase, and a manual environment for my own personal preference. That manual environment cannot use tools.namespace directly without this fix. Alternatively, it would need to duplicate the ns-tracker code in the 'system' boot task, which I also got working, but that wouldn't support the 'unloading' feature of tools.namespace, which would force users to have a JVM restart when things like protocols and multimethods change.

danielsz commented 8 years ago

Thank you, @gtrak, these are indeed exactly the problems with the current boot task.

Your use case sounds perfectly reasonable, so I'm grateful you stated it in clear terms, and I'll try to accommodate it the best I can.

I've had plans for some time now to solve everything with a completely revised task. I'm nurturing somewhat ambitious goals with the interactive experience I want to endow system, and this is part of the problem. There is a branch in which I have endless piles of experiments.

Hearing about your use case and looking at your workaround will probably help me focus. It also motivates me to do incremental fixes that I can actually merge.

More to come soon, I promise.

danielsz commented 8 years ago

Hi @gtrak,

So I bit the bullet and rewrote the whole thing.

I'll start with the good news. I got rid of ns-tracker and reloaded-repl dependencies. The code does what the design, described in the docs, promised to do: refresh and/or restart. You don't need the workarounds anymore. Also, you get the dual manual mode in the REPL and automatic workflow from the boot task with zero configuration.

Now for the not so good news: unloading does not work, and I don't know why. system.reload is a line by line equivalent implementation to clojure.tools.namespace.reload , bar the call to remove-ns, which seems to not work. I would love more eyeballs on this one, and a in-depth explanation of what's going on.

I will upload a snapshot later this week. Please do a lein install for now and get in touch if you can.

Note: This is breaking in the sense that reloaded.repl is now system.repl.

gtrak commented 8 years ago

Nice! I'll give it a try and see if I can figure out what's up with it.

arichiardi commented 8 years ago

I'll be testing if too ;)

danielsz commented 8 years ago

That is so awesome! Thank you @gtrak and @arichiardi!

gtrak commented 8 years ago

@danielsz, so I skimmed through the code, one thing stands out as a possible simplification. Why did you prefer to use the tools.namespace tracker+boot-machinery instead of giving tools.namespace a reference to boot's dirs in get-env? That seemed to work in my testing.

danielsz commented 8 years ago

@gtrak Good point. I've done this first and I've found that it doesn't work.

We need to agree on a repo where we can load/unload the same code and compare, talk about specifics, etc. This one is probably a good place to start (it's simple and ships with system): https://github.com/danielsz/system/tree/master/examples/boot

Steps to follow: 1, Run boot dev 2, Type in a terminal: curl http://localhost:3000/test (It will say: Example.) 3, In line 11 of https://github.com/danielsz/system/blob/master/examples/boot/src/example/handler.clj, change "Example." to "Another Example." Save the file. System should restart everything and the expected response of the curl command this time should be "Another example."

Please try it with your implementation. In my tests, I've traced the problem to remove-ns. When that is being called, new code doesn't get loaded. That is why I went the longer route, used the low-level machinery of tools.namespace, commented out remove-ns, etc.

danielsz commented 8 years ago

Similar findings on remove-ns:

And a comment on remove-ns by the man who wrote it:

So, only namespaces that are not referenced by anything else can be safely removed - it's a very special-purpose operation. Rich

danielsz commented 8 years ago

The challenges ahead:

danielsz commented 8 years ago

Good news. I finished the redesign and everything seems to work, including unloading active code. It is an extensive overhaul, and I will test locally for a couple of days. I've pushed the changes, so if you'd like to try it out, pull in the latest changes, lein install and you're good to go.

Please let me know how you like it...

arichiardi commented 8 years ago

I will test it in a project of mine as well,it is so nice in boot that you can just take the task and voilà :smile:

arichiardi commented 8 years ago

@danielsz I had a look at the code, it looks pretty, I would only push fs-prev-state, modified-namespaces and dirs in the system task (with a let wrapping with-pre-wrap).

EDIT: I now noticed the ns-tracker that needs to be executed inside the task, so I would probably compose: better doing it and see what happens!

danielsz commented 8 years ago

@arichiardi I'm not sure I understand. Everything is inside the system task. The with-pre-wrap macro is not used here because of the data-readers bindings.
May I ask you if you had to change anything in your project to have it work?

arichiardi commented 8 years ago

No change no, but I am trying to have it with mount so I am modifying it a bit, please disregard the comment above :smile:

arichiardi commented 8 years ago

I might not understand how this work, but is there a separate thread where system runs its checks? Because in boot-reload there are explicitly running things in a pod and I understand that the computation never ends, maybe ns-tracker creates a separate thread?

danielsz commented 8 years ago

@arichiardi So the way it works is the following:

The system task runs in the Boot pipeline, and reacts to fileset changes. At every change, we refresh the code, which depending on what has changed can mean:

Additionally, namespaces accept metadata by which they can be marked and spared by remove-ns.

No threads, no pods, pure Boot pipeline stuff.

I'm preparing a blog post in which I will elaborate on the topic.

arichiardi commented 8 years ago

Ok now I understand, thanks for explaining that! The consequence of this is basically that you cannot chain it with a repl task.

danielsz commented 8 years ago

It composes with any Boot task, including the repl task. In the examples directory, you will find a demo project that has a Boot pipeline combining the two.

arichiardi commented 8 years ago

Ok interesting, I still cannot see how it can observe changes once it left the task, I mean who triggers the reacts to fileset changes part but I will probably need to do another pass to the code :smile:

danielsz commented 8 years ago

Ah, right, I should probably mention that it is designed to work in conjunction with Boot's built-in watch task. That is the trigger.

arichiardi commented 8 years ago

Ooooooooh ahahh now it is clear yes ;)

danielsz commented 8 years ago

Here is some background information on the new implementation and lessons learnt:

http://danielsz.github.io/2016/05/06/Ghosts-in-the-machine

danielsz commented 8 years ago

system 0.3.0-SNAPSHOT is deployed.

gtrak commented 8 years ago

works great! :+1: