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

Implement `--index-state` aka index freezing #3893

Merged
merged 4 commits into from Sep 24, 2016

Conversation

@hvr
Copy link
Member

hvr commented Sep 24, 2016

This implements a new flag --index-state (and its cabal.project/config-file
equivalent index-state: ...) which allows to change the source package index
state the solver uses to compute install-plans. This is particularly useful in
combination with freeze-files in order to also freeze the state the package
index was in at the time the install-plan was frozen.

This provides the core functionality, on which future enhancements can build
upon. See also description of #3604 for some possible enhancements.

@dcoutts
Copy link
Member

dcoutts left a comment

Ok, various things, but it's all looking pretty good.

cacheEntries :: [IndexCacheEntry]
}
data Cache = Cache
{ cacheHeadTs :: Timestamp

This comment has been minimized.

@dcoutts

dcoutts Sep 24, 2016

Member

Comment please, what is this? I'm guessing the TS of the final entry, ie the "current" time of the index overall? Help the later reader who lacks context.

writeIndexCache index cache
where
maxTs ents = maximum (nullTimestamp:map cacheEntryTimestamp ents)

This comment has been minimized.

@dcoutts

dcoutts Sep 24, 2016

Member

The performance / streaming characteristics of this are a bit subtle. You're making two passes over entries, one to write out the entries, one to calculate the maxTS. The order of the two passes is only determined here by he order of the Cache fields, and the corresponding behaviour of the Binary instance. I think this would be clearer to be more explicit, as:

let !maxTs = maximum (nullTimestamp:map cacheEntryTimestamp ents)
    cache = Cache {
              cacheEntries = entries,
              cacheHeadTs  = maxTs
            }
-- to avoid import cycles

-- | Effective 'IndexState' after applying 'filerCache'
data IndexStateInfo = IndexStateInfo

This comment has been minimized.

@dcoutts

dcoutts Sep 24, 2016

Member

IndexState or IndexStateInfo? Am I confused or are you? :-)

@@ -115,3 +119,27 @@ instance Text Timestamp where
-- missing/unknown/invalid
nullTimestamp :: Timestamp
nullTimestamp = TS minBound

----------------------------------------------------------------------------
-- defined here for now to avoid import cycles

This comment has been minimized.

@dcoutts
readPackageIndexCacheFile verbosity mkPkg index idxState = do
cache0 <- readIndexCache verbosity index
let (cache,isi) = filterCache idxState cache0
indexHnd <- openFile (indexFile index) ReadMode

This comment has been minimized.

@dcoutts

dcoutts Sep 24, 2016

Member

I'd swap the two lines above.

display (isiHeadTime isi))
return ()
IndexStateTime ts0 -> do
when (isiMaxTime isi /= ts0) $

This comment has been minimized.

@dcoutts

dcoutts Sep 24, 2016

Member

ahh, that's what it's for, checking.

@@ -1424,6 +1430,13 @@ installOptions showOrParseArgs =
installOnlyDeps (\v flags -> flags { installOnlyDeps = v })
(yesNoOpt showOrParseArgs)

, option [] ["index-state"]
"Operate on package index state rolled back to given unix-timestamp"

This comment has been minimized.

@dcoutts

dcoutts Sep 24, 2016

Member

"Use the source package index as it existed at a previous time." and then extend this message with examples of the time formats accepted.

, option [] ["index-state"]
"Operate on package index state rolled back to given unix-timestamp"
installIndexState (\v flags -> flags { installIndexState = v })
(reqArg "STATE" (readP_to_E ("Cannot parse index-state: "++)

This comment has been minimized.

@dcoutts

dcoutts Sep 24, 2016

Member

perhaps the parse error should also mention the formats.

sourcePkgDb <- getSourcePackages verbosity withRepoCtx
pkgConfigDB <- getPkgConfigDb verbosity progdb
let idxState = fromFlagOrDefault IndexUtils.IndexStateHead
. projectConfigIndexState $ projectConfigShared

This comment has been minimized.

@dcoutts

dcoutts Sep 24, 2016

Member

We already put this in the SolverSettings so we can get it from there.

(sourcePkgDb, repos) <-
liftIO $
withRepoCtx $ \repoctx -> do
sourcePkgDb <- IndexUtils.getSourcePackages verbosity repoctx
sourcePkgDb <- IndexUtils.getSourcePackagesWithState verbosity
repoctx idxState

This comment has been minimized.

@dcoutts

dcoutts Sep 24, 2016

Member

So in future we could do better here.

Suppose a project is using index-state: 123 and the user does cabal update somewhere else, so the source package db has changed, but the state of the source package db being used by this project has not (probably, unless the index was not just appended to but really changed -- e.g. as happened when we added the md5 checksums for all old packages).

So yes, ideally, normally we'd not force re-planning just because the index was appended to when it does not affect our project. We could do this by re-solving not on the change of the index but the change of the max timestamp or whatever it is from the index. That said, if we only used the timestamp then we would not notice if the content changed (as when the md5s were added), so it might be safer for index reading to return a checksum rather than just the final timestamp.

Just a thought for the future. It'd be a nice feature once we make index freezing the default workflow.

@ezyang
Copy link
Contributor

ezyang left a comment

Minor things.

-- 'IndexState' defined in "Distribution.Client.IndexUtils.Timestamp"
-- to avoid import cycles

-- | Effective 'IndexState' after applying 'filerCache'

This comment has been minimized.

@ezyang

ezyang Sep 24, 2016

Contributor

filterCache


-- | 'IndexState' specification to 'Cache'
--
-- Note: 'filerCache' is idempotent in the 'Cache' value

This comment has been minimized.

@ezyang

ezyang Sep 24, 2016

Contributor

filterCache

emptyStateInfo :: IndexStateInfo
emptyStateInfo = IndexStateInfo nullTimestamp nullTimestamp

-- | 'IndexState' specification to 'Cache'

This comment has been minimized.

@ezyang

ezyang Sep 24, 2016

Contributor

Maybe better: Filters a 'Cache' according to an 'IndexState' specification. Also returns 'IndexStateInfo' describing the resulting index cache.

@@ -1219,6 +1223,7 @@ data InstallFlags = InstallFlags {
installUpgradeDeps :: Flag Bool,
installOnly :: Flag Bool,
installOnlyDeps :: Flag Bool,
installIndexState :: Flag IndexState,

This comment has been minimized.

@ezyang

ezyang Sep 24, 2016

Contributor

Comment?

@hvr hvr force-pushed the hvr:pr/index-state-3rd-attempt branch from f4850f9 to 119aa61 Sep 24, 2016

@hvr hvr force-pushed the hvr:pr/index-state-3rd-attempt branch from 119aa61 to f56d950 Sep 24, 2016

@hvr hvr force-pushed the hvr:pr/index-state-3rd-attempt branch from f56d950 to abd2fa6 Sep 24, 2016

@hvr hvr added this to the 2.0 milestone Sep 24, 2016

@hvr hvr self-assigned this Sep 24, 2016

@hvr hvr force-pushed the hvr:pr/index-state-3rd-attempt branch from e123660 to afdc36b Sep 24, 2016

@hvr hvr changed the title Implement `--index-state` (3rd attempt) Implement `--index-state` aka index freezing Sep 24, 2016

@hvr hvr force-pushed the hvr:pr/index-state-3rd-attempt branch from afdc36b to 6f57c3b Sep 24, 2016

@hvr hvr merged commit 1535bd3 into haskell:master Sep 24, 2016

@hvr hvr deleted the hvr:pr/index-state-3rd-attempt branch Sep 25, 2016

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