Avoid loading of package caches during build plan construction

[sjakobi]

via IORef trickery

[sjakobi]

I've been looking how to do this properly with MonadBaseControl but failed. My impression was that you'd always have to evaluate getPackageCaches before you could pass it to inner.

[mgsloan]

I don't see what this buys us. getPackageCaches already does caching. It does feel a little ugly to have an IORef in the Config datatype, but it seems like all bets are already off there due to the configEnvOverride type.

Perhaps that caching isn't working in practice (could happen from initializing multiple Config, but I don't think that's happening)?

[sjakobi]

Does this make it any clearer?

-- | Access the package caches from 'IO'.
withPackageCaches
    :: (MonadIO m, MonadReader env m, HasConfig env, MonadLogger m, HasHttpManager env, MonadBaseControl IO m, MonadCatch m)
    => (IO (Map PackageIdentifier (PackageIndex, PackageCache)) -> m a)
    -> m a
withPackageCaches inner = do
    getCaches <- toIO getPackageCaches
    inner getCaches

toIO :: (MonadIO m, MonadBaseControl IO m) => m a -> m (IO a)
toIO m = do
    -- TODO: Use monad base control properly.
    runInBase <- liftBaseWith $ \run -> return (void . run)
    return $ do
        i <- newIORef (error "Impossible evaluation in toIO")
        runInBase $ do
            x <- m
            liftIO $ writeIORef i x
        readIORef i

It looks rather illegal to me.

Alas, we want to access (and update!) the package caches and index during build plan construction, but deserialize the caches only when necessary. The functions in Stack.Build.ConstructPlan are all basically in IO.

I can only speculate why this hasn't been solved differently:

• Changing the types in Stack.Build.ConstructPlan was too much work
• The functions that construct the build plan have to be fast, so we can't just layer all of getPackageCaches' monad transformers on top. (I really have no experience with performance with monad transformers, but that's what I've heard)

Some ideas how to improve the situation:

For the case where we want to make nice error messages (addPackageDeps) I think we could just pass the constraints back to constructPlan and load the package caches there.

Regarding loadPackage I don't really understand yet what installPackage and tellExecutablesUpstream actually do.
For a stack install of stack there are two calls to loadPackage: One for path-io and one for hpack.

Maybe we could make a simpler alternative to loadPackage, without the update logic. When that function doesn't find what we want, we just return an error to constructPlan, let constructPlan update the index and start afresh. At that point the time to construct another build plan doesn't matter anymore.

Or we just add the monad transformers.

[mgsloan]

Ah, I think what confused me is that withPackageCaches could be getPackageCachesIO :: ... => m (IO PackageCaches). There is no reason for it to bracket, and I thought this had something to do with scoping out caching.

The changes to ConstructPlan look reasonable, but I haven't thought too hard about it.

For a stack install of stack there are two calls to loadPackage: One for path-io and one for hpack.

It surprised me that we'd need the package index if the appropriate path-io and hpack versions are installed.

[sjakobi]

There is no reason for it to bracket, and I thought this had something to do with scoping out caching.

Good point, I've updated the PR!

It surprised me that we'd need the package index if the appropriate path-io and hpack versions are installed.

Looks like we should take a closer look! 😄

[mgsloan]

LGTM, thanks! Yes, probably a close look at that case should be taken before closing out #1892

[sjakobi]

Thanks for reviewing!