Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feat: V2.2 Support configuration of hash function for source checksums #181

Merged
merged 15 commits into from
May 15, 2024
Merged

Feat: V2.2 Support configuration of hash function for source checksums #181

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift click to select a range
0814cbc
Feat: V2.2 Support configuration of hash function for source checksums
arnauorriols Apr 25, 2024
4d5c0c1
Add benches
arnauorriols Apr 26, 2024
f4e5bce
fix lint
arnauorriols Apr 26, 2024
fad4b2f
default to no-checksum
arnauorriols May 7, 2024
9e7cb95
xxhash
arnauorriols May 7, 2024
75b3686
Add xxhash bench
arnauorriols May 8, 2024
13570df
Add twox-hash, and make all checksums optional features
arnauorriols May 10, 2024
ab1f997
warning message
arnauorriols May 10, 2024
83c4508
Merge branch 'main' into support-crc32
arnauorriols May 10, 2024
62cc0c5
Restore Cargo.lock to main
arnauorriols May 10, 2024
32cfcce
Enable all features in tests
arnauorriols May 10, 2024
030f61f
Merge branch 'main' into support-crc32
arnauorriols May 14, 2024
fa49cb5
fix build
arnauorriols May 14, 2024
1890e5b
Remove crc32 and xxhash, keep xxhash3
arnauorriols May 15, 2024
e56f8f5
do not run benchmark in cargo test
arnauorriols May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
270 changes: 270 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 32,10 @@ path = "src/examples/builder.rs"
name = "eszip_viewer"
path = "src/examples/viewer.rs"

[[bench]]
name = "source_hash_function"
harness = false

[dependencies]
anyhow = "1"
base64 = "0.21.0"
Expand All @@ -46,6 50,8 @@ serde_json = "1"
sha2 = "0.10.1"
thiserror = "1.0.30"
url = "2.2.2"
crc32fast = "1"
Copy link
Member Author

@arnauorriols arnauorriols Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Question to reviewer: do we want to add the different hashes as optional dependencies? FWIW, the current design will degrade to no checksuming (instead of failing the parsing) if the configured hash function is not available

Copy link
Member

Choose a reason for hiding this comment

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

This seems fine if they are both already deps of CLI.

Copy link
Member

Choose a reason for hiding this comment

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

I think so. For example, in Deno we have no need for this dep atm

Copy link
Member Author

@arnauorriols arnauorriols May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've made them all optional, and kept the "sha256" feature as default feature for backwards compatibility.

xxhash-rust = {version = "0.8", features = ["xxh3"]}
Copy link
Member

Choose a reason for hiding this comment

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

In Deno we use twox-hash. This seems to have a BSL license.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added twox-hash as xxhash feature, and kept xxhash-rust as xxhash3 feature.

I've confirmed twox-hash's xxhash3 implementation is outdated and not producing the same hash as xxhash-rust's. In my machine twox-hash's xxhash64 is faster than xxhash-rust's xxhash3, but different infrastructure might have different results. For this reason, I suggest to keep both.


[dev-dependencies]
import_map = { workspace = true }
Expand All @@ -54,3 60,4 @@ tokio = { version = "1", features = ["macros", "rt"] }
reqwest = { version = "0.11.23", features = ["rustls-tls"] }
jsonc-parser = { version = "0.23.0", features = ["serde"] }
insta = "1.34.0"
criterion = { version = "0.5", features = ["async_tokio"] }
Loading
Loading