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

localPkgDescr is frequently out-of-date #3606

Open
ezyang opened this issue Jul 23, 2016 · 4 comments
Open

localPkgDescr is frequently out-of-date #3606

ezyang opened this issue Jul 23, 2016 · 4 comments
Labels
Cabal: other old-milestone: ⊥ Moved from https://github.com/haskell/cabal/milestone/5 type: refactor
Milestone

Comments

@ezyang
Copy link
Contributor

ezyang commented Jul 23, 2016

LocalBuildInfo has a localPkgDescr, so you might think that it would be OK to read out the PackageDescription from this rather than a PackageDescription that is passed into all of the main actions (e.g., build takes a PackageDescription as WELL as a LocalBuildInfo.) Well, that would be poorly assumed; a buildinfo hook can update the PackageDescription with extra information after the LocalBuildInfo has been built... meaning that the stored localPkgDescr may very well be out of date.

So I wrote 30836c4 to make sure that we actually update localPkgDescr ourselves. BUT LITTLE DID I REALIZE that there are scads of code in the ecosystem which also updatePackageDescription; GHC is one of them! And in fact I spent most of this morning tracking down a bug whereby GHC's build system was silently dropping an extra library, because Cabal had been using the wrong PackageDescription.

What's the moral of the story?

  1. DO NOT, I repeat, DO NOT use localPkgDescr. You CANNOT assume that it is up-to-date because client code may not have updated it while modifying the package description. I suppose something we could do which would make uses of localPkgDescr more likely to work is, wherever we have a function that takes both a PackageDescription and a LocalBuildInfo, override the localPkgDescr right there.
  2. It really was a mistake to not apply hooked build info only once, at configure step. If we had done this, then it would not be necessary to repeatedly reapply the updates. Can we do this? I bet it will break client code too.

So there are three paths we can take.

  1. We can paper over this bug by updating LocalBuildInfo with an explicitly passed in PackageDescription so that the two are consistent.
  2. Going further, expunge all references to localPkgDescr in Cabal, such that it is ONLY used to reconstitute a PackageDescription at the very beginning. This could be done in combination with (1).
  3. Apply hooked build info at configure time, drop post-facto updates. Will probably break Setup code which is written to reapply the hooked build info at build/whatever time.
@23Skidoo
Copy link
Member

/cc @dcoutts

ezyang added a commit to ezyang/cabal that referenced this issue Jul 23, 2016
@ttuegel
Copy link
Member

ttuegel commented Jul 23, 2016

Apply hooked build info at configure time, drop post-facto updates. Will probably break Setup code which is written to reapply the hooked build info at build/whatever time.

FWIW, I think this is the best (easiest to grok) option. It may break things, but at least they will be broken towards sensible.

ezyang referenced this issue Jul 23, 2016
This implements the flag `--allow-older` which is the analogous to
`--allow-newer` acting on lower bounds.
@ezyang
Copy link
Contributor Author

ezyang commented Jul 23, 2016

I just realized that this may not just double-apply, it may not apply at all.

Here's a Setup script from haskell-mpfr-0.1 which would be broken:

  mpfrPreBuild args flags = do
    preBuild simpleUserHooks args flags
    distDir <- getBuildDist flags
    mpfrDist <- getMpfrDist
    makeMpfr mpfrDist
····
    let modified = emptyBuildInfo { extraLibs = ["mpfrPIC"]
                                  , extraLibDirs = [distDir </> "libtmp"]
                                  , includeDirs = [mpfrDist </> "include"]
                                  }

    return (Just modified, snd emptyHookedBuildInfo)

The modified hooked build info is only applied at build-time, not at configure time.

qtah-qt5-0.1.0 similarly only adds the library directory at build time:

qtahHooks :: UserHooks
qtahHooks = simpleUserHooks
  { hookedPrograms = [generatorProgram]
  , postConf = \_ cf _ lbi -> do libDir <- lookupQtahCppLibDir lbi
                                 storeQtahCppLibDir libDir
                                 generateSources cf lbi libDir
  , preBuild = \_ _ -> addLibDir
  , preTest = \_ _ -> addLibDir
  , preCopy = \_ _ -> addLibDir  -- Not sure if necessary, but doesn't hurt.
  , copyHook = \pd lbi uh cf -> do let verbosity = fromFlagOrDefault normal $ copyVerbosity cf
                                   doInstall verbosity pd lbi
                                   copyHook simpleUserHooks pd lbi uh cf
  , preInst = \_ _ -> addLibDir  -- Not sure if necessary, but doesn't hurt.
  , instHook = \pd lbi uh if' -> do let verbosity = fromFlagOrDefault normal $ installVerbosity if'
                                    doInstall verbosity pd lbi
                                    instHook simpleUserHooks pd lbi uh if'
  , preReg = \_ _ -> addLibDir  -- Necessary.
  , cleanHook = \pd z uh cf -> do doClean cf
                                  cleanHook simpleUserHooks pd z uh cf
  } 

another from llvm-data-interop-0.3.0

main = defaultMainWithHooks autoconfUserHooks { preBuild = extBuild }

extBuild args flags = do
  -- Use the Paths module exposed by one of the dependencies to find
  -- the directory containing the enumeration header.  Add that
  -- directory to the search path for this package.
  enumHeaderDir <- getDataFileName "src/c++"
  (Just bi, rest) <- (preBuild autoconfUserHooks) args flags
  let incdirs = includeDirs bi
  return (Just bi { includeDirs = enumHeaderDir : incdirs }, rest)

Fortunately there are only a few dozen packages using preBuild hooks, it would seem.

Also it's embarrassing anyway because I broke HookedBuildInfo with a different type (maybe we should revert.)

@ezyang
Copy link
Contributor Author

ezyang commented Jul 23, 2016

I attempted (3) but I'm bailing, whoever decides to rewrite the hook interface should deal.

@ezyang ezyang added this to the _|_ milestone Sep 6, 2016
@andreabedini andreabedini added the old-milestone: ⊥ Moved from https://github.com/haskell/cabal/milestone/5 label Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cabal: other old-milestone: ⊥ Moved from https://github.com/haskell/cabal/milestone/5 type: refactor
Projects
None yet
Development

No branches or pull requests

4 participants