managed: persist whether an override is a revision, so overrides can be added after restart

Author: tek

lib/internal/env.nix

@@ -65,7 +65,7 @@
   Internal error: A managed override for '${package}' is missing the attribute '${missing}'.
   '';
 
-  managedOverride = api: package: {version ? null, hash ? null, repo ? null, jailbreak ? null, local ? null}: let
+  managedOverride = api: package: {version ? null, hash ? null, repo ? null, jailbreak ? null, local ? null, ...}: let
     hackage = if repo == null then api.hackage else api.hackageConfGen (unknownHackage package) repo;
   in
   if version != null && hash != null

packages/hix/lib/Hix/Data/Overrides.hs

@@ -15,11 +15,35 @@ import Hix.Data.Version (SourceHash)
 import Hix.Managed.Cabal.Data.HackageRepo (HackageName (..))
 import Hix.Pretty (hpretty)
 
+data IsRevision =
+  IsRevision
+  |
+  IsNotRevision
+  deriving stock (Eq, Show)
+
+isRevision :: IsRevision -> Bool
+isRevision = \case
+  IsRevision -> True
+  IsNotRevision -> False
+
+toIsRevision :: Bool -> IsRevision
+toIsRevision = \case
+  True -> IsRevision
+  False -> IsNotRevision
+
+instance FromJSON IsRevision where
+  parseJSON v =
+    toIsRevision <$> parseJSON v
+
+instance EncodeNix IsRevision where
+  encodeNix = encodeNix . isRevision
+
 data Override =
   Override {
     version :: Version,
     hash :: SourceHash,
-    repo :: Maybe HackageName
+    repo :: Maybe HackageName,
+    revision :: Maybe IsRevision
   }
   |
   Jailbreak
@@ -30,7 +54,7 @@ data Override =
 instance EncodeNix Override where
   encodeNix = \case
     Override {..} ->
-      ExprAttrs (static <> foldMap (pure . assoc "repo") repo)
+      ExprAttrs (static <> foldMap (pure . assoc "repo") repo <> foldMap (pure . assoc "revision") revision)
       where
         static = [assoc "version" version, assoc "hash" hash]
 
@@ -43,7 +67,7 @@ instance EncodeNix Override where
 
 override :: Version -> SourceHash -> Override
 override version hash =
-  Override {repo = Nothing, ..}
+  Override {repo = Nothing, revision = Nothing, ..}
 
 instance FromJSON Override where
   parseJSON =
@@ -54,6 +78,7 @@ instance FromJSON Override where
         JsonParsec version <- o .: "version"
         hash <- o .: "hash"
         repo <- o .:? "repo"
+        revision <- o .:? "revision"
         pure Override {..}
 
       jailbreak o = do
@@ -68,13 +93,18 @@ instance FromJSON Override where
 
 instance Pretty Override where
   pretty = \case
-    Override {..} -> pretty version <+> brackets (pretty hash <> foldMap renderRepo repo)
+    Override {..} ->
+      pretty version <+> brackets (pretty hash <> foldMap renderRepo repo <> foldMap renderRevision revision)
     Jailbreak -> "jailbreak"
     Local -> "local"
     where
       renderRepo (HackageName name) =
         hcat [text ",", hpretty name]
 
+      renderRevision = \case
+        IsRevision -> ",rev"
+        IsNotRevision -> mempty
+
 -- | Overrides can be either for mutable (direct, nonlocal) deps, or for transitive deps, so they must use
 -- 'PackageName'.
 newtype Overrides =

packages/hix/lib/Hix/Managed/Build.hs

@@ -2,23 +2,23 @@ module Hix.Managed.Build where
 
 import Control.Monad (foldM)
 import qualified Data.Map.Strict as Map
+import qualified Data.Set as Set
 import qualified Data.Text as Text
 import Distribution.Pretty (Pretty)
 import Exon (exon)
 import Text.PrettyPrint (vcat)
 
+import Hix.Class.Map (nToMaybe)
 import qualified Hix.Color as Color
 import qualified Hix.Console
 import Hix.Console (color, colors)
 import Hix.Data.EnvName (EnvName)
 import Hix.Data.Monad (M)
-import Hix.Data.Overrides (Overrides)
-import qualified Hix.Data.PackageId
-import Hix.Data.PackageId (PackageId)
+import Hix.Data.Overrides (IsRevision (..), Override (..), Overrides)
+import Hix.Data.PackageId (PackageId (..))
 import Hix.Data.Version (Version, Versions)
 import Hix.Data.VersionBounds (VersionBounds)
 import qualified Hix.Log as Log
-import Hix.Managed.Data.NixOutput (PackageDerivation (..))
 import Hix.Managed.Build.Solve (solveMutation)
 import qualified Hix.Managed.Cabal.Changes
 import Hix.Managed.Cabal.Config (isNonReinstallableDep, isReinstallableId)
@@ -32,6 +32,7 @@ import qualified Hix.Managed.Data.Mutation
 import Hix.Managed.Data.Mutation (BuildMutation (BuildMutation), DepMutation, MutationResult (..))
 import qualified Hix.Managed.Data.MutationState
 import Hix.Managed.Data.MutationState (MutationState (MutationState), updateBoundsWith)
+import Hix.Managed.Data.NixOutput (PackageDerivation (..))
 import Hix.Managed.Data.Query (Query (Query))
 import qualified Hix.Managed.Data.QueryDep
 import Hix.Managed.Data.QueryDep (QueryDep)
@@ -107,12 +108,12 @@ buildVersions ::
   Bool ->
   Versions ->
   [PackageId] ->
-  M (Overrides, Set PackageId, BuildStatus)
+  M (Overrides, BuildStatus)
 buildVersions builder context description allowRevisions versions overrideVersions = do
   logBuildInputs context.env description reinstallable
-  (result, (overrides, revisions)) <- builder.buildTargets allowRevisions versions reinstallable
+  (result, overrides) <- builder.buildTargets allowRevisions versions reinstallable
   logBuildResult description result
-  pure (overrides, revisions, buildStatus result)
+  pure (overrides, buildStatus result)
   where
     reinstallable = filter isReinstallableId overrideVersions
 
@@ -121,29 +122,32 @@ buildConstraints ::
   EnvContext ->
   Text ->
   Bool ->
-  Set PackageId ->
+  Overrides ->
   SolverState ->
-  M (Maybe (Versions, Overrides, Set PackageId, BuildStatus))
-buildConstraints builder context description allowRevisions prevRevisions state =
+  M (Maybe (Versions, Overrides, BuildStatus))
+buildConstraints builder context description allowRevisions prevOverrides state =
   solveMutation builder.cabal context.deps prevRevisions state >>= traverse \ changes -> do
-    (overrides, revisions, status) <-
+    (overrides, status) <-
       buildVersions builder context description allowRevisions changes.versions changes.overrides
-    pure (changes.versions, overrides, prevRevisions <> revisions, status)
+    pure (changes.versions, overrides, status)
+  where
+    prevRevisions =
+      Set.fromList $ nToMaybe prevOverrides \cases
+        name Override {version, revision = Just IsRevision} -> Just PackageId {..}
+        _ _ -> Nothing
 
 buildMutation ::
   EnvBuilder ->
   EnvContext ->
   MutationState ->
-  Set PackageId ->
   BuildMutation ->
-  M (Maybe (MutationState, Set PackageId))
-buildMutation builder context state prevRevisions BuildMutation {description, solverState, updateBound} =
-  result <$> buildConstraints builder context description True prevRevisions solverState
+  M (Maybe MutationState)
+buildMutation builder context state BuildMutation {description, solverState, updateBound} =
+  result <$> buildConstraints builder context description True state.overrides solverState
   where
     result = \case
-      Just (versions, overrides, revisions, status) -> do
-        new <- justSuccess (updateMutationState updateBound versions overrides state) status
-        pure (new, revisions)
+      Just (versions, overrides, status) ->
+        justSuccess (updateMutationState updateBound versions overrides state) status
       Nothing -> Nothing
 
 logMutationResult ::
@@ -177,7 +181,7 @@ validateMutation envBuilder context handlers stageState mutation = do
       then pure MutationKeep
       else handlers.process stageState.ext mutation build
 
-    build = buildMutation envBuilder context stageState.state stageState.revisions
+    build = buildMutation envBuilder context stageState.state
 
 convergeMutations ::
   Pretty a =>

packages/hix/lib/Hix/Managed/Build/Mutation.hs

@@ -56,18 +56,18 @@ updateConstraints impl candidate state =
 -- TODO If we'd use the @retract@ field from @DepMutation@ and the target bound here, we could probably use a universal
 -- bounds updater without leaking implementation...investigate.
 buildCandidate ::
-  (BuildMutation -> M (Maybe (MutationState, Set PackageId))) ->
+  (BuildMutation -> M (Maybe MutationState)) ->
   (Version -> VersionBounds -> VersionBounds) ->
   (MutableId -> PackageId -> MutationConstraints -> MutationConstraints) ->
   SolverState ->
   MutableDep ->
   Version ->
-  M (Maybe (MutableId, SolverState, MutationState, Set PackageId))
+  M (Maybe (MutableId, SolverState, MutationState))
 buildCandidate build updateStateBound updateConstraintBound solverState package version = do
   Log.debug [exon|Mutation constraints for #{showP candidate}: #{showP mutationSolverState.constraints}|]
   fmap result <$> build (candidateMutation mutationSolverState candidate updateStateBound)
   where
-    result (newState, revisions) = (candidate, newSolverState newState, newState, revisions)
+    result newState = (candidate, newSolverState newState, newState)
 
     candidate = MutableId {name = package, version}
 

packages/hix/lib/Hix/Managed/Build/Single.hs

@@ -1,6 +1,7 @@
 module Hix.Managed.Build.Single where
 
 import Hix.Data.Monad (M)
+import Hix.Data.Overrides (Overrides)
 import Hix.Data.VersionBounds (exactVersion)
 import Hix.Managed.Build (buildConstraints)
 import Hix.Managed.Cabal.Data.SolverState (solverState)
@@ -18,10 +19,11 @@ buildVersions ::
   EnvContext ->
   Text ->
   MutableVersions ->
+  Maybe Overrides ->
   M BuildStatus
-buildVersions builder context description versions =
-  buildConstraints builder context description False [] solver <&> \case
-    Just (_, _, _, status) -> status
+buildVersions builder context description versions prevOverrides =
+  buildConstraints builder context description False (fold prevOverrides) solver <&> \case
+    Just (_, _, status) -> status
     Nothing -> Failure
   where
     solver = solverState context.solverBounds context.deps (fromVersions exactVersion versions) def

packages/hix/lib/Hix/Managed/Build/Solve.hs

@@ -9,7 +9,7 @@ import Text.PrettyPrint (hang, ($$), (<+>))
 
 import Hix.Class.Map (nRestrictKeys)
 import Hix.Data.Monad (M)
-import Hix.Data.PackageId (PackageId (PackageId))
+import Hix.Data.PackageId (PackageId)
 import Hix.Data.Version (packageIdVersions)
 import qualified Hix.Log as Log
 import qualified Hix.Managed.Cabal.Changes
@@ -28,6 +28,18 @@ logNonReinstallable :: NonEmpty PackageId -> M ()
 logNonReinstallable ids =
   Log.verbose [exon|NOTE: Cabal solver suggested new versions for non-reinstallable packages: #{showPL ids}|]
 
+-- | Forcing revisions means that any package that has a revision in the Hackage snapshot will be treated as an
+-- override, i.e. it will be built from Hackage despite having the same version as the one installed in nixpkgs.
+--
+-- The benefit of doing this is that often nixpkgs will be outdated in comparison with Hackage, and therefore have
+-- tighter dependency bounds.
+-- When Cabal resolves a plan based on revised bounds in packages whose versions match nixpkgs, but not their revisions,
+-- the nix build will fail with bounds errors, requiring a restart with revisions.
+--
+-- On the other hand, in many situations (like lower bound mutations), this is entirely irrelevant; in others, the
+-- original bounds might just work for the current build; and most often nixpkgs actually has the latest revision, which
+-- we cannot observe at this point.
+-- Therefore, this feature is disabled until it can be refined.
 checkRevision ::
   Bool ->
   CabalHandlers ->
@@ -62,13 +74,13 @@ processSolverPlan forceRevisions cabal deps prevRevisions SolverPlan {..} = do
   where
     projectDeps = nRestrictKeys mutablePIds versions
     versions = packageIdVersions (overrides ++ installed)
-    overrides = filter notLocal (changes ++ forcedRevisions ++ reusedRevisions)
+    overrides = changes ++ forcedRevisions ++ reusedRevisions
+    -- If a package has been selected for revision during a prior build, add it to the overrides despite its matching
+    -- version.
+    -- This simply ensures that the revision procedure can be skipped in this build, since the same version will likely
+    -- cause the same dependency bounds error that triggered the revision.
     (reusedRevisions, installed) = partition (flip Set.member prevRevisions) noForcedRevisions
     (noForcedRevisions, forcedRevisions) = partitionEithers (checkRevision forceRevisions cabal <$> matching)
-    -- notLocal PackageId {name} = not (isLocalPackage deps.local name)
-    -- TODO I assumed that targets hadn't been part of EnvDeps.local for a long time, so this shouldn't be effective
-    -- anymore, but verify anyway!
-    notLocal PackageId {} = True
     mutablePIds = Set.fromList (depName <$> Set.toList deps.mutable)
 
 -- TODO probably best to store the revisions in the SolverState

packages/hix/lib/Hix/Managed/Build/Target.hs

@@ -1,7 +1,6 @@
 module Hix.Managed.Build.Target where
 
 import Control.Monad.Trans.State.Strict (StateT (runStateT))
-import qualified Data.Map.Strict as Map
 import Exon (exon)
 import Path (Abs, Dir, Path)
 
@@ -26,7 +25,7 @@ import Hix.Managed.Data.Targets (Targets, firstMTargets)
 import Hix.Managed.Handlers.AvailableVersions (AvailableVersionsHandlers (..))
 import Hix.Managed.Handlers.SourceHash (SourceHashHandlers)
 import Hix.Managed.Handlers.StateFile (StateFileHandlers)
-import Hix.Managed.Overrides (packageOverride, packageOverrides)
+import Hix.Managed.Overrides (packageOverrideRegular, packageOverrides, packageRevision)
 import Hix.Managed.StateFile (writeBuildStateFor, writeSolverStateFor)
 
 data BuilderResources =
@@ -76,7 +75,7 @@ suggestRevision resources _ pkg = \cases
   Nothing (BoundsError _)
     | Just package <- failedPackageId pkg
     -> do
-      override <- packageOverride resources.hackage [] package
+      override <- packageRevision resources.hackage [] package
       pure (Just RetryPackage {package, ..})
   _ _ -> pure Nothing
 
@@ -92,9 +91,13 @@ suggestNothing _ _ _ _ =
 latestVersionFor :: BuilderResources -> PackageName -> M (Maybe RetryPackage)
 latestVersionFor resources target =
   resources.versions.latest target >>= traverse \ latest -> do
-    override <- packageOverride resources.hackage [] PackageId {name = target, version = latest}
+    override <- packageOverrideRegular resources.hackage [] PackageId {name = target, version = latest}
     pure RetryPackage {package = PackageId {name = target, version = override.version}, ..}
 
+-- | This might seem wrong at first glance – it immediately jailbreaks the entire package even though a newer revision
+-- might relax just the right bounds and leave the rest intact.
+-- However, at this point bounds are entirely useless, since a) we already incorporated proper bounds in our plan by
+-- running the solver, and b) nix cannot select between different versions anyway.
 suggestJailbreakAndLatestVersion ::
   BuilderResources ->
   FailureCounts ->
@@ -130,11 +133,11 @@ buildTargets ::
   Bool ->
   Versions ->
   [PackageId] ->
-  M (BuildResult, (Overrides, Set PackageId))
+  M (BuildResult, Overrides)
 buildTargets builder allowRevisions _ overrideVersions = do
   overrides <- packageOverrides builder.global.hackage builder.localUnavailable overrideVersions
   let build target = buildAdaptive (buildWithOverrides builder.global builder.env target) suggest
       s0 = (overrides, [])
-  second (second Map.keysSet) <$> runStateT (firstMTargets (BuildSuccess []) buildUnsuccessful build builder.targets) s0
+  second fst <$> runStateT (firstMTargets (BuildSuccess []) buildUnsuccessful build builder.targets) s0
   where
     suggest = if allowRevisions then suggestRevision builder.global else suggestNothing

packages/hix/lib/Hix/Managed/Data/Mutation.hs

@@ -4,7 +4,6 @@ import Distribution.Pretty (Pretty (pretty))
 import qualified Text.PrettyPrint as PrettyPrint
 import Text.PrettyPrint (parens, (<+>))
 
-import Hix.Data.PackageId (PackageId)
 import Hix.Data.Version (Version)
 import Hix.Data.VersionBounds (VersionBounds)
 import Hix.Managed.Cabal.Data.SolverState (SolverState)
@@ -41,7 +40,6 @@ data MutationResult s =
     candidate :: MutableId,
     changed :: Bool,
     state :: MutationState,
-    revisions :: Set PackageId,
     ext :: s
   }
   |

packages/hix/lib/Hix/Managed/Data/StageState.hs

@@ -94,12 +94,11 @@ data StageState a s =
     success :: Map MutableDep BuildSuccess,
     failed :: [DepMutation a],
     state :: MutationState,
-    revisions :: Set PackageId,
     iterations :: Word,
     ext :: s
   }
   deriving stock (Eq, Show)
 
 initStageState :: Initial MutationState -> s -> StageState a s
 initStageState (Initial state) ext =
-  StageState {success = [], failed = [], revisions = [], iterations = 0, ..}
+  StageState {success = [], failed = [], iterations = 0, ..}