Dev ghc session test #39

alanz · 2017-12-14T20:29:12Z

Tests now pass.

Please review

DanielG · 2017-12-20T14:21:01Z

src/CabalHelper/Runtime/Main.hs

@@ -70,6 +70,11 @@ import Distribution.Simple.LocalBuildInfo
  , withComponentsLBI
  , withLibLBI
  , withExeLBI
+
+#if CH_MIN_VERSION_Cabal(2,0,0)


CPP'ed import go at the end of the import list, grouped with the others.

DanielG · 2017-12-20T14:22:10Z

src/CabalHelper/Runtime/Main.hs

@@ -218,6 +238,7 @@ usage = do
     ++"  | ghc-lang-options [--with-inplace]\n"
     ++"  | package-db-stack\n"
     ++"  | entrypoints\n"
+     ++"  | needsbuildoutput\n"


sed -i 's/needsbuildoutput/needs-build-output/g'

DanielG · 2017-12-20T14:23:42Z

src/CabalHelper/Runtime/Main.hs

    libopts =
+      -- AZ:TODO: we already have the clbi, use it rather


DanielG · 2017-12-20T14:24:40Z

src/CabalHelper/Runtime/Main.hs

          in
-            cleanRecursiveOpts (CLib lib) libbi libclbi
+                      -- ghcOptInputModules = toNubListR $ allLibModules lib clbi,


Does this comment convey any important information?

DanielG · 2017-12-20T14:25:20Z

src/CabalHelper/Runtime/Main.hs

+             , componentEntrypoints c))
+  return $ Map.fromList $ map snd includeDirs
+
+type SubDeps = ([UnitId], [FilePath], [(OpenUnitId, ModuleRenaming)], ChEntrypoint)


I'd prefer if this was a full blown datatype with record field names.

DanielG · 2017-12-20T14:35:06Z

tests/GhcSession.hs

@@ -30,9 +31,10 @@ main = do
  args <- getArgs
  topdir <- getCurrentDirectory
  res <- mapM (setup topdir test) $ case args of
-    [] -> [ ("tests/exelib"   , parseVer "1.10")
+    [] -> [
+            ("tests/exelib"   , parseVer "1.10")


Don't change the indent style for no good reason please.

DanielG · 2017-12-20T14:37:43Z

tests/GhcSession.hs

+        exists <- doesDirectoryExist packageDir
+        let opts' = if exists
+              then ("-package-db " ++ packageDir) : "-Werror" : opts
+              else "-Werror" : opts


This whole packageDir business shouldn absolutely not be here, if it has to be something in Main.hs is still broken. Remember this test is supposed to simulate what ghc-mod will have to do and the whole point of cabal-helper is to abstract away cabal specific hacks like that.

Can you explain why this is necessary?

It is a matter of timing.

If a component has ProduceBuildOutput set, then invoking cabal build (at the ghc-mod or higher level) will result in the packageDir being created.

But at the point we invoke cabal-helper the build has not yet happened.

So one alternative is to invoke cabal-helper more than once, if ProduceBuildOutput is set, and let it check for existence of that package db dir and add it to the options if needed.

But that does seem more complicated than requiring the cabal-helper client to do it.

@DanielG please give your preference.

DanielG · 2017-12-20T14:38:22Z

tests/GhcSession.hs

+              else "-Werror" : opts
+
+        -- let opts' = "-Werror" : opts
+        -- let opts' = "-v 3" : "-Werror" : opts


DanielG · 2017-12-20T14:45:30Z

tests/GhcSession.hs

@@ -113,11 +132,15 @@ compileModule ep opts = do

    handleSourceError (\e -> GHC.printException e >> return False) $ do

+    let target = case nb of
+          ProduceBuildOutput -> HscNothing -- AZ: what should this be?


You know, I saw your conversation with ezyang and I didn't realize backpack packages with holes won't work with GHCi/HscInterpreted when we started this.

The way I see it we have two choices here either we tell ghc-mod which hscTarget it has to use and have it operate in a degraded capacity or we tell it to ignore such backpack packages altogether the latter would mean users couldn't even syntax check modules from such components.

The former would need investigating which target to use whereas the latter is more or less zero effort. I guess we can deduce that we need to change target/ignore a component based on needsBuildOutput for now so we can defer the decision until we get to integrating these changes into ghc-mod.

DanielG · 2017-12-20T14:48:42Z

src/CabalHelper/Runtime/Main.hs

+combineEp (Just (ChLibEntrypoint es1 os1 ss1)) (ChLibEntrypoint es2 os2 ss2) = Just (ChLibEntrypoint (nub $ es2++es1) (nub $ os2++os1) (nub $ ss2++ss1))
+combineEp (Just (ChLibEntrypoint es1 os1 ss1)) (ChExeEntrypoint  mi os2)     = Just (ChExeEntrypoint mi (nub $ os2++es1++os1++ss1))
+combineEp (Just (ChExeEntrypoint   _ os1))     (ChLibEntrypoint es2 os2 ss2) = Just (ChLibEntrypoint es2 (nub $ os2++os1) ss2)
+combineEp (Just (ChExeEntrypoint   _ os1))     (ChExeEntrypoint  mi os2)     = Just (ChExeEntrypoint mi  (nub $ os2++os1))


Not sure what to make of this function. Half of it should really be a Semigroup instance for ChEntrypoint and the other half (the Maybe) stuff is probably just mplus or some such.

I agree it is messy, but have no good ideas on how to improve it. Working code alternatives welcomed.

Something like this:

import Data.Semigroup instance Semigroup ChEntrypoint where ChLibEntrypoint es1 os1 ss1 <> ChLibEntrypoint es2 os2 ss2 = ChLibEntrypoint (nub $ es1 ++ es2) (nub $ os1 ++ os2) (nub $ ss1 ++ ss2) -- etc. combineEp Nothing ep = Just ep combineEp maybeEp1 ep2 = (ep2 <>) <$> maybeEp1

I didn't check whether the order of the lists actually matters (I still don't understand the rest of the package well enough).

I think the Semigroup instance can't be made much easier.

EDIT: Semigroup definition, Nothing case

Looking at this again I think a Semigroup instance just isn't appropriate here, it seems to me you're trying to use ChEntrypoint as a container for intermediate data for which it is just not fit for. Especially the conversions between ChExe <-> ChLib make this obvious and really don't fit what a Semigroup instance for this type should do in my mind (though it could probably be law abiding anyways). Previously I thought all the cases were just of the form ChXEntrypoint, ChXEntrypoint -> ChXEntrypoint which is why I suggested Semigroup.

Does recursiveIncludeDirs really need to return a ChXEntrypoint? It didn't need to previously, and if it doesn't need to, the function could be rewritten in a much simpler way using some sort of fold I think. (Btw, in case of a successful Map.lookup u includeDirs, the function returns an "accumulated" ChXEntrypoint holding all the intermediate paths. Not sure whether this is intended.)

Still need clarity on best way of adding the local package db when it is needed and exists.

Which includes inserting the inplace directory for package includes

Subject to DanielG/cabal-helper#39

turion · 2018-01-08T20:56:34Z

src/CabalHelper/Runtime/Main.hs

+                                  , ChEntrypoint)
+recursiveIncludeDirs includeDirs unit = go ([],[],Nothing) [unit]
+  where
+    go (afp,aci,Nothing  ) [] = (afp,aci,error "recursiveIncludeDirs:no ChEntrypoint")


This is a lazy error, i.e. if you only inspect afp and aci, the error won't be thrown. I don't know whether this is the intended behaviour.

turion · 2018-01-08T21:09:58Z

src/CabalHelper/Runtime/Main.hs

@@ -373,13 +398,33 @@ main = do
      return $ Just $ ChResponsePkgDbs $ map pkgDb $ withPackageDB lbi

    "entrypoints":[] -> do
+#if CH_MIN_VERSION_Cabal(2,0,0)
+      includeDirMap <- recursiveDepInfo lbi v distdir
+      eps <- componentsMap lbi v distdir $ \c clbi _bi -> do


We could remove the do here since both cases are return.

Indeed, we can also pull the returns out of the two cases.

Except componentsMap is in IO, so requires a return.

Yeah you're right, but the two returns can be merged into one like: return $ case ... of .... since both cases just deal with pure values.

alanz added 10 commits December 3, 2017 21:50

Add recursive otherModules and exposedModules to the "entrypoints"

0508962

Saner way to merge entrypoints

486eb9c

Add missing tests to sdist, check for Cabal 2.0 for CFlib

d4d2264

Add CPP for earlier cabal versions

91e4296

WIP on backpack support

f11b496

Bring in needsBuildOuput query, for indefinite modules

d1214f4

Progress on when to build and when not

5e40beb

Tests pass for backpack

0cf6848

Remove extraneous imports

ddd7cb2

Clean up

371646b

alanz requested a review from DanielG December 14, 2017 20:29

DanielG requested changes Dec 20, 2017

View reviewed changes

alanz added 3 commits December 21, 2017 22:31

WIP on addressing review issues

8002b81

Still need clarity on best way of adding the local package db when it is needed and exists.

When a component uses an internal backpack module, use cabal opts

50a1131

Which includes inserting the inplace directory for package includes

Move import into CPP zone

cf2a820

alanz added a commit to alanz/ghc-mod that referenced this pull request Jan 6, 2018

Update for newer cabal-hel

d3d920a

Subject to DanielG/cabal-helper#39

turion reviewed Jan 8, 2018

View reviewed changes

DanielG merged commit cf2a820 into DanielG:dev/ghc-session-test Jan 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dev ghc session test #39

Dev ghc session test #39

alanz commented Dec 14, 2017

DanielG Dec 20, 2017

DanielG Dec 20, 2017

DanielG Dec 20, 2017

DanielG Dec 20, 2017

DanielG Dec 20, 2017

DanielG Dec 20, 2017

DanielG Dec 20, 2017

alanz Dec 21, 2017

DanielG Dec 20, 2017

DanielG Dec 20, 2017

DanielG Dec 20, 2017

alanz Dec 21, 2017

turion Jan 8, 2018 •

edited

Loading

DanielG Jan 9, 2018

turion Jan 9, 2018

turion Jan 8, 2018

turion Jan 8, 2018

DanielG Jan 9, 2018

alanz Jan 9, 2018

DanielG Jan 9, 2018

		libopts =
		-- AZ:TODO: we already have the clbi, use it rather

Dev ghc session test #39

Dev ghc session test #39

Conversation

alanz commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turion Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turion Jan 8, 2018 •

edited

Loading