[WIP] Lock files#4550
Conversation
qrilka
left a comment
qrilka
left a comment
There was a problem hiding this comment.
I think we need to discuss some details in a call
| let cfInfo = CFIHash cabalHash (Just cabalSize) | ||
| packageIdRev = PackageIdentifierRevision name version cfInfo | ||
| pprintExtra (name, (version, BlobKey _ _)) = | ||
| let packageIdRev = PackageIdentifierRevision name version CFILatest |
There was a problem hiding this comment.
could you explain this change? Why do we use CFILatest when we have a particular blob key?
There was a problem hiding this comment.
This change is for the suggestions part. Without this, if a package is not in the snapshot - The stack build tool will recommend it to be added like this:
* Recommended action: try adding the following to your extra-deps in /home/sibi/github/tldr-hs2/stack.yaml:
string-quote-0.0.1@sha256:7d91a0ba1be44b2443497c92f2f027cd4580453b893f8b5ebf061e1d85befaf3
With the above change, it will recommend it like this:
* Recommended action: try adding the following to your extra-deps in /home/sibi/github/tldr-hs2/stack.yaml:
string-quote-0.0.1
The reason I made that change was because of Michael's comment in the issue:
... the original stack.yaml can be even more lightweight than it is today, e.g. including the
@rev:0information there will no longer be necessary at all.
There was a problem hiding this comment.
But why not change the suggestion properly instead? This change above complicates the logic a bit an gives no hints about reasons
| , isLockFileOutdated | ||
| , loadPackageLockFile | ||
| , loadSnapshotLayerLockFile | ||
| ) |
There was a problem hiding this comment.
do we need such a large multiline import list? Why not just everything from Stack.Lock? Also it doesn't look great when not aligned with other imports in the module
| cachedCompletePackageLocation :: (HasPantryConfig env, HasLogFunc env, HasProcessContext env) => Map RawPackageLocation PackageLocation | ||
| -> RawPackageLocation | ||
| -> RIO env PackageLocation | ||
| cachedCompletePackageLocation cachePackages rp@(RPLImmutable rpli) = do |
There was a problem hiding this comment.
minor naming suggestion: what about s/cachePackages/cachedLocations/?
| case item of | ||
| Nothing -> do | ||
| pl <- completePackageLocation rpli | ||
| pure $ PLImmutable pl |
There was a problem hiding this comment.
maybe in 1 line as PLImmutable <$> completePackageLocation rpli?
| (\x -> "Lock file package removed: " <> display x) | ||
| (chRemoved change) | ||
| mapM_ logDebug addedStr | ||
| mapM_ logDebug deletedStr |
There was a problem hiding this comment.
what about
forM_ (chRemoved change) $ \p ->
logDebug $ "Lock file package removed: " <> display p
Imo it's much more readable - e.g. 2 lines instead of 5
|
|
||
| loadSnapshotFile :: | ||
| Path Abs File -> Path Abs Dir -> IO [RawPackageLocationImmutable] | ||
| loadSnapshotFile path rootDir = do |
There was a problem hiding this comment.
I still see the same custom parsing, do I miss something?
| value | ||
|
|
||
| loadPackageLockFile :: Path Abs File -> IO LockFile | ||
| loadPackageLockFile lockFile = do |
| => Map RawPackageLocationImmutable PackageLocationImmutable | ||
| -> RawPackageLocationImmutable | ||
| -> RIO env PackageLocationImmutable | ||
| cachedSnapshotCompletePackageLocation cachePackages rpli = do |
There was a problem hiding this comment.
duplicated in cachedCompletePackageLocation?
| if lockFileOutdated | ||
| then do | ||
| logDebug "Lock file is outdated or isn't present" | ||
| generatePackageLockFile stackYamlFP |
There was a problem hiding this comment.
in generatePackageLockFile you complete locations when it's needed, why can't we just reuse that information and not to go through FS to get it?
There was a problem hiding this comment.
and not to go through FS
We go through the FS to read the lock file to see if we can avoid doing completePackageLocation and rather reuse the information from the lock file. Is there a reason you don't want to go through FS ?
There was a problem hiding this comment.
I see no reasons not to reuse already completed locations without going through FS
|
Closing in favor of #4746 |
4 participants
Note: Documentation fixes for https://docs.haskellstack.org/en/stable/ should target the "stable" branch, not master.
Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!