Detect unlisted modules and TH dependent files (#32,#105)

[borsboom]

This seems to basically work. The approach it takes is to have GHC dump the interfaces (using -ddump-hi -ddump-to-file) while it builds, and then it parses the dumped interfaces when checking for changes (before the next build) to detect any unlisted modules and TH dependencies.

I'd like input on some of these before moving further.

• If there are unlisted/TH dependencies, the second time you build after a clean (without changing any files in the interim) it thinks those files are new and so it runs the build anyway. That's because the dumped HI files didn't exist for the first build so it didn't record the state of the unlisted files. Is this a corner case worth fixing? To do so, it'd have to do a pass looking over the .dump-hi files immediately after the build to find the unlisted files and record their state.
• It reads all the .dump-hi files before every build. That seems to be pretty fast, but caching would be faster. Worth doing? (this and the previous item probably make sense to do together)
• Currently there are a number list->Set->list conversions. To fix that, it'd mean changing a bunch of stuff in the module to use Sets instead of lists. Any objection to doing so?
• I noticed nub is used a bunch in the existing code. I think converting the code to use Sets would eliminate many of those nubs, but any remaining should probably become nubOrds (from extra) instead. Agreed?
• Stack.Ghci will now get the unlisted modules in ghciPkgModules (previously it only got the listed ones). Is that the correct behaviour?
• The Stack.Iface module Dan added isn't used at all (this code draws inspiration from it, though). Should I just remove that?

ping @chrisdone @snoyberg

[borsboom]

An idea mentioned in #105 (comment) is to display warnings for any files not listed from the .cabal file. For modules, this should be straightforward, but for TH dependencies I don't think there's a good way to know whether the file should be listed or not. For example, using gitrev makes a module depend on .git/packed-refs and .git/index only if building in a git repo, and these obviously should not be be listed in extra-source-files.

[snoyberg]

Was this discovered by this code itself?

[borsboom]

Yup!

[snoyberg]

If there are unlisted/TH dependencies, the second time you build after a clean (without changing any files in the interim) it thinks those files are new and so it runs the build anyway

Actually, I think your current solution is the right one, it avoids an ugly race condition where a TH-loaded file is edited after the build is started.

That seems to be pretty fast, but caching would be faster. Worth doing?

Not yet, let's wait until we see it cause trouble in practice

Currently there are a number list->Set->list conversions. To fix that, it'd mean changing a bunch of stuff in the module to use Sets instead of lists. Any objection to doing so?

None at all, but let's wait till this is merged to master and then do the cleanup on master itself.

I noticed nub is used a bunch in the existing code. I think converting the code to use Sets would eliminate many of those nubs, but any remaining should probably become nubOrds (from extra) instead. Agreed?

Absolutely

Stack.Ghci will now get the unlisted modules in ghciPkgModules (previously it only got the listed ones). Is that the correct behaviour?

I'll need to defer to @chrisdone on that

The Stack.Iface module Dan added isn't used at all (this code draws inspiration from it, though). Should I just remove that?

Yes, and may as well include it in this branch (though doing on master after merge is also fine).

---

Overall: I like the approach, and the code is shorter than I was hoping for, which is great. Having a few integration tests around this would be nice, but I'm OK merging this to master now. Please do so when you're ready. I'm doing some testing on Windows and will shout if there's a problem, but I doubt that will be the case.

[snoyberg]

In testing (at least on Windows): rebuilding did not occur after a change to a TH-included file. I used embedFile to include foo.txt, built, and then modified that file with no rebuild triggered.

[borsboom]

There seem to be worse issues on Windows. Just saw this, seems to be an error from GHC itself while writing the .dump-hi file:

$ stack build  --copy-bins
stack-0.1.2.7-192640abed725b4dd084095d9f476aa3: unregistering (local file changes)
stack-0.1.2.7: build
Building stack-0.1.2.7...
Preprocessing library stack-0.1.2.7...
[40 of 64] Compiling Stack.Package    ( src\Stack\Package.hs, .stack-work\dist\i386-windows\Cabal-1.18.1.5\build\Stack\Package.o )
[41 of 64] Compiling Stack.Build.Types ( src\Stack\Build\Types.hs, .stack-work\dist\i386-windows\Cabal-1.18.1.5\build\Stack\Build\Types.o ) [Stack.Package changed]
[42 of 64] Compiling Stack.GhcPkg     ( src\Stack\GhcPkg.hs, .stack-work\dist\i386-windows\Cabal-1.18.1.5\build\Stack\GhcPkg.o ) [Stack.Build.Types changed]
.stack-work\dist\i386-windows\Cabal-1.18.1.5\build\src/Stack\GhcPkg.dump-hi: commitBuffer: invalid argument (invalid character)

--  While building package stack-0.1.2.7 using:
      C:\\Users\\emanu\\AppData\\Local\\Programs\\stack\\i386-windows\\ghc-7.8.4\\bin\\runhaskell.exe -package=Cabal-1.18.1.5 -clear-package-db -global-package-db -package-db=C:\Users\emanu\AppData\Roaming\stack\snapshots\i386-windows\lts-2.17\7.8.4\pkgdb\ C:\Users\emanu\AppData\Local\Temp\stack1620\Setup.hs --builddir=.stack-work\dist\i386-windows\Cabal-1.18.1.5\ build --ghc-options -hpcdir .stack-work\dist\i386-windows\Cabal-1.18.1.5\hpc\.hpc\ -ddump-hi -ddump-to-file
    Process exited with code: ExitFailure 1

[snoyberg]

That's interesting, I did not trigger that bug in my testing.

[borsboom]

@snoyberg TH dependency changes should now work on Windows (was a newline issue).

For the "invalid character" problem, have you tried building stack itself on Windows using this version of stack? I've tried to reproduce on Linux using LC_ALL=C, but wasn't able to. I tried the chcp 65001 trick that works for Hakyll on Windows, but that didn't help.

[borsboom]

Probably triggered by this bit of output it's trying to write to the .dump-hi (note the α in (GHC.Types.IO α -> m α)):

  $wunregisterGhcPkgId :: Control.Monad.IO.Class.MonadIO m
                          -> Control.Monad.Logger.MonadLogger m
                          -> Control.Monad.Catch.MonadCatch m
                          -> Control.Applicative.Applicative GHC.Types.IO
                          -> Control.Applicative.Applicative m
                          -> GHC.Base.Monad GHC.Types.IO
                          -> GHC.Base.Monad m
                          -> (GHC.Types.IO α -> m α)
                          -> System.Process.Read.EnvOverride
                          -> Path.Internal.Path Path.Abs Path.Dir
                          -> Stack.Types.GhcPkgId.GhcPkgId
                          -> m ()

[snoyberg]

I can confirm that the original bug I reported is resolved. I'll play a bit more and see if I can reproduce the other problem.

[snoyberg]

If you set your LANG environment variable on Windows, does that solve the problem?

[borsboom]

No, doesn't seem to make any difference.

[snoyberg]

Do you have any interesting environment variables set by any chance?

[borsboom]

I merged the Windows code page fix to master and then rebased this PR onto it. Solves the "invalid character" for me.

[chrisdone]

I'm still gonna review this when I get round to it, btw.