Add reload #26

thumphries · 2017-02-09T00:43:53Z

Howdy maintainers,

I've been using hint for dynamic reloading of web applications, where filesystem events or other signals trigger a reload and restart. The implicit reset in loadModules made this quite slow, so I wrote this GHCi-style incremental reload function.

Let me know if there are any changes required, or if there are safety concerns that might prevent it being merged. It seems to be roughly equivalent to GHCi's reload command.

Thanks for building and maintaining this library, it saved me a lot of GHC API pain.

mvdan

Overall sounds like a good idea, but would like to have someone else's input too.

cc @gelisam

mvdan · 2017-02-09T09:49:50Z

src/Hint/Context.hs

-                    doLoad fs `catchIE` (\e -> reset >> throwM e)
+                    targets <- mapM (\f->runGhc2 GHC.guessTarget f Nothing) fs
+                    runGhc1 GHC.setTargets targets
+                    doLoad GHC.LoadAllTargets `catchIE` (\e -> reset >> throwM e)


This line is exactly what reload runs too. Deduplicate code?

mvdan · 2017-02-09T09:50:12Z

src/Language/Haskell/Interpreter.hs

    -- ** Context handling
     ModuleName, isModuleInterpreted,
     loadModules, getLoadedModules, setTopLevelModules,
+     reload,


Please put it in the reset line like the other file.

mvdan · 2017-02-09T09:52:26Z

src/Hint/Context.hs

-                   --
-                   runGhc1 GHC.setTargets targets
-                   res <- runGhc1 GHC.load GHC.LoadAllTargets
+reload :: MonadInterpreter m => m ()


Since this will be exported, it will need documentation. See how it is done for reset.

mvdan · 2017-02-09T10:01:50Z

Also please use a slightly more detailed commit message, like Add reload, similar to ghci's :reload or Add reload to Language.Haskell.Interpreter.

thumphries · 2017-02-10T03:14:27Z

Thanks for the feedback.

I've found I need to call setTopLevelModules after calling reload. Maybe this should perform getContext / setContext also, to behave a little more like GHCi. What do you think?

mvdan

I've found I need to call setTopLevelModules after calling reload. Maybe this should perform getContext / setContext also, to behave a little more like GHCi. What do you think?

setTopLevelModules seems to call setContext, so I would think not.

In any case, if we're wrong about this, I'm not too worried as we could fix it later without changing the exposed API.

mvdan · 2017-02-10T09:40:25Z

src/Hint/Context.hs

-                   res <- runGhc1 GHC.load GHC.LoadAllTargets
+                    targets <- mapM (\f->runGhc2 GHC.guessTarget f Nothing) fs
+                    runGhc1 GHC.setTargets targets
+                    catchReset (doLoad GHC.LoadAllTargets)


I meant that this line could just call reload, unless I'm missing something. Then, catchReset would be unnecessary.

Fixed. Apologies for my density!

mvdan · 2017-02-11T17:58:14Z

Thanks for the updates. Code looks good, but since I'm not familiar with ghci's :reload I'm gonna wait for someone who is to have a look before merging and releasing this.

cc @jcpetruzza @gelisam

gelisam · 2017-02-11T17:59:52Z

I'm not familiar with it either, sorry.

jcpetruzza · 2017-02-11T18:06:34Z

Could I suggest adding a unit test for this function? It will make it easier to convince people that this actually works as intended, and it will make the life of @mvdan infinitely easier the next time the ghc api changes and reload needs to be fixed 😄

thumphries · 2017-02-13T02:50:22Z

In any case, if we're wrong about this, I'm not too worried as we could fix it later without changing the exposed API.

Sounds good. I expect to be using this functionality for some internal developer tooling pretty shortly, so we'll know pretty quickly if it misbehaves or (more likely) is annoying to use.

Code looks good, but since I'm not familiar with ghci's :reload I'm gonna wait for someone who is to have a look before merging and releasing this.

In the absence of this person, I'll try to convince you of my reasoning.

here is the implementation from ghci-ng. It does a LoadAllTargets and then restores the context from the GHCi state.

It also reverts CAFs after every load/reload. I noticed Hint has a few comments wondering if we should do that or not, I certainly don't know the answer :) though it seems the consequences mostly affect magic constants like stdin stdout.

The other step in afterLoad, restoring tick arrays, seems to refer to the internal state used by the debugger. We should be ok ignoring this, likewise the Safe Haskell stuff.

Could I suggest adding a unit test for this function?

I'll add some tests similar to those for loadModules, though they should behave identically with this implementation. The only difference is reload is incremental, so it is much faster and suitable for use in a development loop.

thumphries · 2017-02-13T06:07:51Z

unit-tests/run-unit-tests.hs

+                             --
+                             liftIO $ writeFile mod_file_a mod_a_v2
+                             liftIO $ writeFile mod_file_b mod_b
+                             set [searchPath := ["."]]


I'm not quite sure why this is necessary, will need to spend some time debugging. It breaks if you move it to the top, which isn't good!

mvdan · 2017-02-13T09:52:00Z

Thanks for adding the test!

will need to spend some time debugging

Let me know when you're happy with the PR.

mvdan

Just to remind myself and everyone that this isn't ready to merge, I'm marking it as "changes requested".

thumphries · 2017-03-02T15:40:58Z

Thanks, I'll get to this pretty soon.

Just restating the reason for block, for future archaeologists: something odd is happening when working on modules in . (pwd) - modules are able to load, but they don't reload. Explicitly setting . as the search path makes things work again. All modules outside . seem to reload correctly.

mvdan · 2017-06-12T10:54:52Z

Friendly ping - any news? I'm not in a rush, but I'm not a fan of leaving pull requests open for a long time either.

Perhaps we should move this to an issue after the summer if it's still not ready to merge by then.

thumphries · 2017-06-12T12:40:39Z

Sorry for the delay! The project I needed this for has been postponed for a while. If I haven't addressed it the next time you come around to it I'm happy for it to be turned into an issue. While this mostly works as-is, I feel the quirks of GHC's module resolution should be better understood and documented before this is made available to users. It can be quite surprising. I was able to build large commercial applications and do incremental reloading with the current implementation, but you need to be holding it right, as my testing woes above suggest.

…

On 12 Jun. 2017, at 7:54 pm, Daniel Martí ***@***.***> wrote: Friendly ping - any news? I'm not in a rush, but I'm not a fan of leaving pull requests open for a long time either. Perhaps we should move this to an issue after the summer if it's still not ready to merge by then. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

mvdan · 2018-02-28T14:42:59Z

It has been a full year without activity, so I'm going to close this pull request for now. Please feel free to reopen it if you do find time to work on it again.

Or, if you think there's useful information for others here, but you're not planning on using it yourself soon, feel free to move the thread to an issue as well.

mvdan reviewed Feb 9, 2017

View reviewed changes

thumphries force-pushed the topic/reload branch from 26bb948 to 80ac231 Compare February 10, 2017 03:11

mvdan reviewed Feb 10, 2017

View reviewed changes

Add reload, similar to ghci's :reload

edddb9c

thumphries force-pushed the topic/reload branch from 80ac231 to edddb9c Compare February 11, 2017 03:29

thumphries added 2 commits February 13, 2017 15:57

Add failing unit test

09aec17

This fixes it for some reason

43a3cfd

thumphries commented Feb 13, 2017

View reviewed changes

mvdan suggested changes Mar 2, 2017

View reviewed changes

charleso mentioned this pull request Mar 9, 2017

Haskell quick dev mode haskell-mafia/loom#66

Open

4 tasks

mvdan closed this Feb 28, 2018

Add reload #26

Add reload #26

Uh oh!

Conversation

thumphries commented Feb 9, 2017

Uh oh!

mvdan left a comment

Choose a reason for hiding this comment

Uh oh!

mvdan Feb 9, 2017

Choose a reason for hiding this comment

Uh oh!

mvdan Feb 9, 2017

Choose a reason for hiding this comment

Uh oh!

mvdan Feb 9, 2017

Choose a reason for hiding this comment

Uh oh!

mvdan commented Feb 9, 2017

Uh oh!

thumphries commented Feb 10, 2017

Uh oh!

mvdan left a comment

Choose a reason for hiding this comment

Uh oh!

mvdan Feb 10, 2017

Choose a reason for hiding this comment

Uh oh!

thumphries Feb 11, 2017

Choose a reason for hiding this comment

Uh oh!

mvdan commented Feb 11, 2017

Uh oh!

gelisam commented Feb 11, 2017

Uh oh!

jcpetruzza commented Feb 11, 2017

Uh oh!

thumphries commented Feb 13, 2017

Uh oh!

thumphries Feb 13, 2017

Choose a reason for hiding this comment

Uh oh!

mvdan commented Feb 13, 2017

Uh oh!

mvdan left a comment

Choose a reason for hiding this comment

Uh oh!

thumphries commented Mar 2, 2017

Uh oh!

mvdan commented Jun 12, 2017

Uh oh!

thumphries commented Jun 12, 2017 via email

Uh oh!

mvdan commented Feb 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants