feature(pkg): improve checksum handling #7696
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rgrinberg
force-pushed
the
ps/rr/feature_pkg___improve_checksum_handling
branch
from
01431e4
to
5a35cd0
Compare
Alizter
reviewed
May 8, 2023
rgrinberg
force-pushed
the
ps/rr/feature_pkg___improve_checksum_handling
branch
from
5a35cd0
to
3262875
Compare
Alizter
reviewed
May 8, 2023
| | Some (loc, _) -> | ||
| User_error.raise ~loc | ||
| [ Pp.text "Invalid checksum, got" | ||
| ; Dune_pkg.Checksum.pp actual_checksum |
There was a problem hiding this comment.
Suggested change
| ; Dune_pkg.Checksum.pp actual_checksum | |
| ; Checksum.pp actual_checksum |
Alizter
approved these changes
May 8, 2023
|
According to the Dune monorepo benchmark, this PR introduces a regression in file-watching mode: Or maybe it's just noise? We should start looking at the benchmarks! |
|
The particular benchmark where there's a "regression" oscillates between ~80 and ~100 seconds. Also, this PR apparently speeds up another (hardly plausible) test by 16%. Definitely seems like noise given that this PR doesn't touch these code paths at all. |
|
Yeah, that seems right. Nevertheless, I'd love to see that table copied here by a bot. |
rgrinberg
force-pushed
the
ps/rr/feature_pkg___improve_checksum_handling
branch
from
3262875
to
83ae2cc
Compare
Leonidas-from-XIV
approved these changes
May 9, 2023
| @@ -998,37 +1007,53 @@ module Fetch = struct | |||
|
|
|||
| let is_useful_to ~distribute:_ ~memoize = memoize | |||
|
|
|||
| let encode_loc f (loc, x) = | |||
| Dune_lang.List | |||
| (* TODO use something better for locs here *) | |||
There was a problem hiding this comment.
Do we have some common loc representation in Sexp? Like (loc (file f.ml) (line 42))?
rgrinberg
force-pushed
the
ps/rr/feature_pkg___improve_checksum_handling
branch
from
83ae2cc
to
69818c8
Compare
* Add locations to various checksum errors * Add locations to checksum parsing errors Signed-off-by: Rudi Grinberg <me@rgrinberg.com> <!-- ps-id: 594bfdad-fa2d-493e-b259-bc7486124b39 -->
rgrinberg
force-pushed
the
ps/rr/feature_pkg___improve_checksum_handling
branch
from
69818c8
to
4e068bd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Rudi Grinberg me@rgrinberg.com