Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating static files never triggers recompilation #94

Closed
snoyberg opened this issue Jun 28, 2013 · 8 comments
Closed

Updating static files never triggers recompilation #94

snoyberg opened this issue Jun 28, 2013 · 8 comments

Comments

@snoyberg
Copy link
Member

Consider the following code:

{-# LANGUAGE OverloadedStrings #-}

module Main where

import IdeSession
import Data.Monoid (mconcat)
import qualified Data.ByteString.Lazy.Char8 as L8

main :: IO ()
main = do
    sess <- initSession defaultSessionConfig
                { configDir             = "."
                }

    let cb = \_ -> return ()
        update = flip (updateSession sess) cb

    let mainContents = L8.pack $ unlines
            [ "{-# LANGUAGE TemplateHaskell #-}"
            , "import Text.Hamlet"
            , "import Text.Blaze.Html.Renderer.String"
            , "main = putStrLn $ renderHtml $(shamletFile \"foo.hamlet\")"
            ]

    update $ mconcat
        [ updateModule "Main.hs" mainContents
        , updateDataFile "foo.hamlet" "<p invalid"
        ]

    -- Error message expected, invalid data file
    getSourceErrors sess >>= print

    update $ updateDataFile "foo.hamlet" "<p>Valid"

    -- No error message expected, but got one anyway
    getSourceErrors sess >>= print

    update $ updateModule "Main.hs" $ mainContents `L8.append` "\n\n-- Trigger a recompile"
    -- No more error message
    getSourceErrors sess >>= print

    update $ updateDataFile "foo.hamlet" "<p invalid"

    -- Error message expected, but recompilation is not triggered.
    getSourceErrors sess >>= print

I've added an invalid Hamlet file, and a source file which references this Hamlet file. As expected, compilation fails. However, updating the data file with a valid Hamlet file does not trigger any recompilation, and the only way to get the change noticed is to actually change the source file. Going in the reverse direction, updating the Hamlet file with something invalid does not trigger recompilation either.

Note that Hamlet uses qAddDependentFile (http://hackage.haskell.org/packages/archive/template-haskell/2.7.0.0/doc/html/Language-Haskell-TH-Syntax.html#v:qAddDependentFile) internally, so under normal circumstances GHC is able to detect that a recompilation is necessary.

@snoyberg
Copy link
Member Author

Note: this is not a high priority, urgent issue, but I would like to get some feedback on what's going on here and how difficult it would be to fix.

@edsko
Copy link
Collaborator

edsko commented Jun 28, 2013

We keep a flag internally whether we should invoke ghc at the next call to updateSession, and this flag is set whenever you change a source file, but not when you change a data file. So the easy fix to this would be to set this flag on updateDataFile too. The better fix would be to hook in to qAddDependentFile, but that's probably overkill. A third option would be to add an extra argument to updateDataFile to specify whether this should trigger recompilation or not.

@snoyberg
Copy link
Member Author

Let's go for the easy fix: just setting the flag on updateDataFile. If you can provide a commit on top of the temporary-pkgdir-hack branch, we can start testing this and see if it solves our problem (while avoiding any negative side-effects).

@ghost ghost assigned Mikolaj Jun 28, 2013
Mikolaj added a commit that referenced this issue Jun 28, 2013
Only verified that it does not break the current tests. Specific tests
will be added to the other branches later on.
@Mikolaj
Copy link
Collaborator

Mikolaj commented Jun 28, 2013

Pinging @snoyberg and @jwiegley, just in case. :)

@snoyberg
Copy link
Member Author

Perfect, thanks. And yes, the ping was very good, we don't get notifications when commits have been made.

@jwiegley Can you update our ide-backend on master to use this change?

@jwiegley
Copy link

@snoyberg Doing so now.

Mikolaj added a commit that referenced this issue Jun 29, 2013
Only verified that it does not break the current tests. Specific tests
will be added to the other branches later on.
Mikolaj added a commit that referenced this issue Jun 29, 2013
@snoyberg
Copy link
Member Author

This is now on our staging system, and does indeed solve our problem. Thank you for the fast turnaround @edsko and @Mikolaj. Closing.

@Mikolaj
Copy link
Collaborator

Mikolaj commented Jun 29, 2013

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants