Skip to content

Lock files attempt 2#4746

Merged
snoyberg merged 15 commits intomasterfrom
lock-files-2
Apr 30, 2019
Merged

Lock files attempt 2#4746
snoyberg merged 15 commits intomasterfrom
lock-files-2

Conversation

@qrilka
Copy link
Copy Markdown
Contributor

@qrilka qrilka commented Apr 16, 2019

Please include the following checklist in your PR:

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • The documentation has been updated, if necessary.

Tested manually and with local run of integration tests
Has some code and resolves #4550
Resolves #4288

@qrilka qrilka requested review from psibi and snoyberg April 16, 2019 12:16
@qrilka
Copy link
Copy Markdown
Contributor Author

qrilka commented Apr 16, 2019

One note about the implementation: it persist all dependency locations and that means that stack.yaml.lock is quite large because it contains all snapshot packages so loading it takes noticeable time.

@qrilka qrilka changed the title Lock files 2 Lock files attempt 2 Apr 16, 2019
@snoyberg
Copy link
Copy Markdown
Contributor

I haven't looked at the code yet, but based on your comment @qrilka: do you think it makes more sense to take a different approach here? Two things I can think of:

  • Don't include all packages. This seems like it will make it quite difficult to come up with a SourceMapHash
  • Use a more efficient format, either as the primary format or as an optimized format for lookup. In the latter, I mean keeping the .lock file as a YAML file, but keeping a .lock.bin file next to it (or elsewhere) in a more efficient binary format, or maybe even storing in the SQLite database

@qrilka
Copy link
Copy Markdown
Contributor Author

qrilka commented Apr 17, 2019

@snoyberg I think I need to check the current timings to get a bit more clear understanding of the problem. But in any case it looks odd to add an optimization which makes Stack slower.

@qrilka qrilka changed the title Lock files attempt 2 [WIP] Lock files attempt 2 Apr 17, 2019
@qrilka
Copy link
Copy Markdown
Contributor Author

qrilka commented Apr 23, 2019

@snoyberg on Monday I've tried to optimize sourcemap-based lockfiles and I had some improvements but still if we parse almost the same what we have in a snapshot it takes some time. So in 40bf6f0 I've simplified everything and in that commit lock file contains only non-trivial location completions (e.g. snapshot locations won't get stored there) - it simplifies logic quite a lot and also it gives some speed improvements (up to 20% compared to optimized sourcemap-based locks).
I think we should go this simpler and a bit faster way.
One visible difference with the previous version is that we don't check snapshots but I don't see how that could cause any troubles.
If we choose this simple variant then lock_files.md needs to be rewritten.

@qrilka
Copy link
Copy Markdown
Contributor Author

qrilka commented Apr 23, 2019

BTW regarding timings I have this gist - https://gist.github.com/qrilka/d5e220f3955c8d8ea70ad40f720f5f12

@snoyberg snoyberg mentioned this pull request Apr 23, 2019
2 tasks
@qrilka qrilka changed the title [WIP] Lock files attempt 2 Lock files attempt 2 Apr 23, 2019
@qrilka qrilka changed the title Lock files attempt 2 [WIP] Lock files attempt 2 Apr 23, 2019
@qrilka
Copy link
Copy Markdown
Contributor Author

qrilka commented Apr 23, 2019

@snoyberg 1 of integration test failed, so I need to fix that before it will be ready for a proper review

@qrilka
Copy link
Copy Markdown
Contributor Author

qrilka commented Apr 23, 2019

the problem with 1438-configure-options looks to be unrelated to this change so this PR is ready to be reviewed

@qrilka qrilka changed the title [WIP] Lock files attempt 2 Lock files attempt 2 Apr 23, 2019
Copy link
Copy Markdown
Contributor

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really solid, and I'm happy with the results of testing it so far! One request: I didn't notice a test for removing a package from extra-deps and checking that it was removed from the lock file as well. If I missed it: ignore me. If I didn't miss it: can you add such a test?

{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE OverloadedStrings #-}

module Stack.LockSpec where
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very happy to see this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to @psibi :)

Comment thread subs/pantry/src/Pantry.hs
logDebug $ "Parsing cabal file for " <> display loc
bs <- loadCabalFileBytes loc
let foundCabalKey = BlobKey (SHA256.hashBytes bs) (FileSize (fromIntegral (B.length bs)))
let foundCabalKey = bsToBlobKey bs
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good helper function

@qrilka qrilka requested a review from snoyberg April 30, 2019 07:07
@qrilka
Copy link
Copy Markdown
Contributor Author

qrilka commented Apr 30, 2019

@snoyberg please take another look, especially into the doc

Comment thread doc/lock_files.md Outdated
This should minimize the number of changes to packages incurred.
When loading a Stack project all completed package or snapshot locations
(even when they were completed using information from a lock file) get
collected to form a new lock file.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to form a new lock file

Can you clarify this a bit? I'm guessing the logic is "form a new lock file in memory and compare against the one on disk, writing if there are any differences." Is that the case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll add this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@snoyberg snoyberg merged commit 8b824ef into master Apr 30, 2019
@snoyberg snoyberg deleted the lock-files-2 branch April 30, 2019 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Write pantry hashes into a .lock file instead of emitting warnings

2 participants