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

Recompilation checking issue with multiple packages, --fast and TH #6248

Open
TeofilC opened this issue Sep 20, 2023 · 22 comments
Open

Recompilation checking issue with multiple packages, --fast and TH #6248

TeofilC opened this issue Sep 20, 2023 · 22 comments

Comments

@TeofilC
Copy link

TeofilC commented Sep 20, 2023

Summary

We noticed that some of our tests that used TH were sometimes not recompiling with stack. This led to stale test results.

I've managed to extract the following reproducer.

NB: this is reproducible with GHC 9.2.8 so it's not related to the recompilation changes in 9.4.

Steps to reproduce

Clone this repository https://github.com/TeofilC/stack-recomp-checking-issue.

Then run ./repro.sh

This does the following:

  1. Build everything
  2. Edits a file
  3. Only rebuilds the package that file belongs to
  4. Then tries to build the dependent package, which doesn't recompile <- the bug

Expected

I would expect the file to recompile even with --fast and cabal-install does this correctly, so it seems like this is a bug in stack rather than GHC/Cabal.

Actual

The file doesn't recompile.

I couldn't attach the verbose log as its too long, but should be easy to generate from the reproducer.

Stack version

stack --version
2.11.1 x86_64 hpack-0.35.2

Method of installation

nix

Platform

WSL2 x86_64

@TeofilC
Copy link
Author

TeofilC commented Sep 20, 2023

Ok interestingly with GHC 9.2.8 this is broken with both cabal-install and stack, but it seems to be fixed for cabal-install with GHC 9.4

@TeofilC
Copy link
Author

TeofilC commented Sep 20, 2023

The difference seems to be that with GHC 9.4 we have addDependentFile added for TH imports so maybe some logic around that is broken

@mpilgrem
Copy link
Member

@TeofilC, thanks for reporting. I hope to get to your reproducer over the next few days.

@mpilgrem
Copy link
Member

@TeofilC, I am confused. Looking at the steps in the repro.sh file (with added comments by me, and added step numbers):

[1] stack build --fast # build everything # with GHC option `-O0`
[2] sed -i 's/"someFunc"/"someFunc1"/g' lib-A/src/LibA.hs # Change module LibA to replace references to `someFunc` with `someFunc1`
[3] stack build lib-A --fast # just A! # with GHC option `-O0`
[4] stack build lib-B --fast # This will now fail, because module Lib has not been updated for the changes in module LibA
[5] sed -i 's/"someFunc1"/"someFunc"/g' lib-A/src/LibA.hs # Change module LibA again

Package lib-B has module Lib with code:

{-# LANGUAGE TemplateHaskell #-}
module Lib
    ( someFunc
    ) where

import qualified LibA

import Language.Haskell.TH.Syntax

someFunc :: String
someFunc = $( runIO (print LibA.someFunc) >> lift LibA.someFunc )

On Windows, step [4] fails as expected - because LibA no longer exports a function someFunc - with:

❯ stack build lib-B
lib-B> build (lib)
Preprocessing library for lib-B-0.1.0.0..
Building library for lib-B-0.1.0.0..
[1 of 2] Compiling Lib [LibA changed]

src\Lib.hs:11:28: error:
    • Not in scope: ‘LibA.someFunc’
      Perhaps you meant one of these:
        ‘Lib.someFunc’ (line 11), ‘LibA.someFunc1’ (imported from LibA)
      Module ‘LibA’ does not export ‘someFunc’.
    • In the untyped splice:
        $(runIO (print LibA.someFunc) >> lift LibA.someFunc)
   |
11 | someFunc = $( runIO (print LibA.someFunc) >> lift LibA.someFunc )
   |                            ^^^^^^^^^^^^^

src\Lib.hs:11:51: error:
    • Not in scope: ‘LibA.someFunc’
      Perhaps you meant one of these:
        ‘Lib.someFunc’ (line 11), ‘LibA.someFunc1’ (imported from LibA)
      Module ‘LibA’ does not export ‘someFunc’.
    • In the untyped splice:
        $(runIO (print LibA.someFunc) >> lift LibA.someFunc)
   |
11 | someFunc = $( runIO (print LibA.someFunc) >> lift LibA.someFunc )
   |                                                   ^^^^^^^^^^^^^

Error: [S-7282]
       Stack failed to execute the build plan.

       While executing the build plan, Stack encountered the error:

       [S-7011]
       While building package lib-B-0.1.0.0 (scroll up to its section to see the error) using:
       C:\sr\setup-exe-cache\x86_64-windows\Cabal-simple_9p6GVs8J_3.6.3.0_ghc-9.2.8.exe --verbose=1 --builddir=.stack-work\dist\8a54c84f build lib:lib-B --ghc-options " -fdiagnostics-color=always"
       Process exited with code: ExitFailure 1

@mpilgrem
Copy link
Member

I don't understand the relevance of --fast here. All it does is specify GHC's option -O0 - see https://docs.haskellstack.org/en/stable/build_command/#-fast-flag.

@TeofilC
Copy link
Author

TeofilC commented Sep 20, 2023

Oh I think the quoting works differently on Windows?
The aim of that sed command is to replace the /string/ "someFunc" with "someFunc1" rather than the name of the function. Admittedly this is a bit confusing sorry for that.

I don't understand the relevance of --fast here. All it does is specify GHC's option -O0 - see https://docs.haskellstack.org/en/stable/build_command/#-fast-flag.

Indeed the passing -O0 would be equivalent

@mpilgrem
Copy link
Member

@TeofilC, I see. I misunderstood because I am not familar with sed. I'll try again.

@mpilgrem
Copy link
Member

mpilgrem commented Sep 20, 2023

@TeofilC, so, if the steps are (correcting my understanding):

[1] stack build --fast # build everything # with GHC option `-O0`
[2] sed -i 's/"someFunc"/"someFunc1"/g' lib-A/src/LibA.hs # Change module LibA to replace the string in function `someFunc`
[3] stack build lib-A --fast # just A! # with GHC option `-O0`
[4] stack build lib-B --fast # Build B with GHC option `-O0`
[5] sed -i 's/"someFunc1"/"someFunc"/g' lib-A/src/LibA.hs # Change module LibA again

On Windows, this works fine. Step [4] is:

❯ stack build lib-B
lib-B> build (lib)
Preprocessing library for lib-B-0.1.0.0..
Building library for lib-B-0.1.0.0..
[1 of 2] Compiling Lib [LibA changed]
lib-B> copy/register
Installing library in D:\Users\mike\Code\Haskell\stack-recomp-checking-issue\.stack-work\install\c55ac0a2\lib\x86_64-windows-ghc-9.2.8\lib-B-0.1.0.0-H6rHC3vmm6DChm3TRQxBL6
Registering library for lib-B-0.1.0.0..

What are you experiencing, that differs? (EDIT: I've tried it with and without --fast, it makes no difference.)

@TeofilC
Copy link
Author

TeofilC commented Sep 20, 2023

Yes that's right. Though [5] is only necessary to get us back to the inital state.

I'm expecting [4] to lead to lib-B recompiling, since the TH splice uses LibA.someFunc.
If you then make use of lib-B you will find that it contains the old value of LibA.someFunc

@TeofilC
Copy link
Author

TeofilC commented Sep 20, 2023

On rereading your output I see that it is recompiling for you. It doesn't do that for me on Linux

@mpilgrem
Copy link
Member

mpilgrem commented Sep 20, 2023

Actually, I think the --fast flag is having some effect. Will explain shortly.

EDIT:

I've added an executable to package lib-B:

module Main (main) where

import Lib

main :: IO ()
main = putStrLn someFunc
executables:
  bTest:
    source-dirs: app
    main: Main.hs
    dependencies:
    - lib-B

With --fast, the final steps are:

❯ stack build lib-B --fast
lib-B> build (lib + exe)
Preprocessing library for lib-B-0.1.0.0..
Building library for lib-B-0.1.0.0..
Preprocessing executable 'bTest' for lib-B-0.1.0.0..
Building executable 'bTest' for lib-B-0.1.0.0..
Linking .stack-work\dist\8a54c84f\build\bTest\bTest.exe ...
lib-B> copy/register
Installing library in D:\Users\mike\Code\Haskell\stack-recomp-checking-issue\.stack-work\install\c55ac0a2\lib\x86_64-windows-ghc-9.2.8\lib-B-0.1.0.0-H6rHC3vmm6DChm3TRQxBL6
Installing executable bTest in D:\Users\mike\Code\Haskell\stack-recomp-checking-issue\.stack-work\install\c55ac0a2\bin
Registering library for lib-B-0.1.0.0..

❯ stack exec bTest
someFunc

Without --fast the final steps are:

❯ stack build lib-B
lib-B> build (lib + exe)
Preprocessing library for lib-B-0.1.0.0..
Building library for lib-B-0.1.0.0..
[1 of 2] Compiling Lib [LibA changed]
Preprocessing executable 'bTest' for lib-B-0.1.0.0..
Building executable 'bTest' for lib-B-0.1.0.0..
[1 of 2] Compiling Main [Lib changed]
Linking .stack-work\dist\8a54c84f\build\bTest\bTest.exe ...
lib-B> copy/register
Installing library in D:\Users\mike\Code\Haskell\stack-recomp-checking-issue\.stack-work\install\c55ac0a2\lib\x86_64-windows-ghc-9.2.8\lib-B-0.1.0.0-H6rHC3vmm6DChm3TRQxBL6
Installing executable bTest in D:\Users\mike\Code\Haskell\stack-recomp-checking-issue\.stack-work\install\c55ac0a2\bin
Registering library for lib-B-0.1.0.0..

❯ stack exec bTest
someFunc1

This behaviour reminds me of something, somewhere else. I'll see if I can put my finger on it.

@TeofilC
Copy link
Author

TeofilC commented Sep 20, 2023

I think I might have broken this reproducer when minimising. When trying it again with GHC 9.4.6 I don't seem to get this behaviour. I'll take another look at it

@TeofilC
Copy link
Author

TeofilC commented Sep 20, 2023

The executable stuff sounds like #6193

@mpilgrem
Copy link
Member

mpilgrem commented Sep 20, 2023

@TeofilC
Copy link
Author

TeofilC commented Sep 20, 2023

After further digging I've managed to refine a reproducer that actually works.
I've pushed it to the repository.

The key thing is that the test suite doesn't get unregistered by [3] since that build invocation doesn't get passed --test.

While this does concern an executable, I think this is a distinct issue since the logs show we don't even compile the module after the change, so it's not merely a linking thing. And I can reproduce it on GHC 9.2.8.

@mpilgrem
Copy link
Member

I see the problem. After 'dirty' A is re-built, B does not recognise that A is still 'dirty' compared to the last time that B was built.

@mpilgrem
Copy link
Member

I don't think this is anything to do with Template Haskell or --fast. I get the same behaviour with simply:

lib-A:

module LibA ( someFuncA ) where

someFuncA :: IO ()
someFuncA = putStrLn "someFuncA"

lib-B:

module LibB ( someFuncB ) where

import LibA

someFuncB = do
  someFuncA
  putStrLn "someFuncB"

@mpilgrem
Copy link
Member

mpilgrem commented Sep 20, 2023

Maybe related: #2339
Perhaps of interest: #3130
Looks very similar to this: #2904 (closed) - has something that was fixed become unfixed?

@mpilgrem
Copy link
Member

What appears to distinguish this issue from closed #2904 (closed, fixed and still fixed) is that it is an executable in package B (including a test-suite) that depends on package A when there is no library in package B that depends on package A. So a route to fixing it may be to examine the 'cure' for #2904, which was #3047 (Aggressive unregister).

@mpilgrem
Copy link
Member

My current hypothesis is that this is because, it seems:
(a) a Cabal package without a library stanza does not result in an installed package (ghc-pkg list); and
(b) a Cabal package with a library stanza results in an installed package but its depends: entry does not include the dependencies of the stanzas other than the library stanza (ghc-pkg describe).

I am not sure if that is a Stack bug or a Cabal bug, put perhaps this Cabal issue is relevant: haskell/cabal#8434.

However, Stack.Build.ConstructPlan.mkUnregisterLocal is working off a list of dumped installed packages (ghc-pkg dump) and depending on the depends: field (extract):

-- | Determine which packages to unregister based on the given tasks and
-- already registered local packages.
mkUnregisterLocal ::
     Map PackageName Task
     -- ^ Tasks
  -> Map PackageName Text
     -- ^ Reasons why packages are dirty and must be rebuilt
  -> [DumpPackage]
     -- ^ Local package database dump
  -> Bool
     -- ^ If true, we're doing a special initialBuildSteps build - don't
     -- unregister target packages.
  -> Map GhcPkgId (PackageIdentifier, Text)
mkUnregisterLocal tasks dirtyReason localDumpPkgs initialBuildSteps =
  -- We'll take multiple passes through the local packages. This will allow us
  -- to detect that a package should be unregistered, as well as all packages
  -- directly or transitively depending on it.
  loop Map.empty localDumpPkgs
...
  go :: DumpPackage -> State UnregisterState ()
  go dp = do
    us <- get
    case maybeUnregisterReason (usToUnregister us) ident mParentLibId deps of
      ...
   where
    gid = dpGhcPkgId dp
    ident = dpPackageIdent dp
    mParentLibId = dpParentLibIdent dp
    deps = dpDepends dp -- <<<< If this is missing the dependency, it can't get unregistered

@mpilgrem
Copy link
Member

@TeofilC, I think I know what the cause of the problem is: an installed GHC package does not capture non-library stanza dependencies.

That means that there is a workaround, for now, albeit an 'ugly' one: either (a) include a 'dummy' library stanza in the package with a build-depends: that references the local dependencies of the non-library stanza(s) or (b) if there is a library stanza in the package, ensure it also includes 'dummy' references to the dependencies unique to the non-library stanzas.

@TeofilC
Copy link
Author

TeofilC commented Sep 22, 2023

Thanks Ill try that!

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

No branches or pull requests

2 participants