Skip to content

Conversation

@sftse
Copy link
Contributor

@sftse sftse commented Sep 9, 2025

When we fixed #62 we unfortunately acquired a rather large dependency tree.

With some test shenanigans, we can downgrade icu_casemap to a dev-dependency, so downstream consumers do not have to foot the bill. We can even remove it entirely, since we generate a small table now, but then need to manually add it to dependencies again if we want to regenerate the table for newer Unicode standards.

@sftse
Copy link
Contributor Author

sftse commented Sep 9, 2025

This takes build time from 6.3s on my machine down to 0.7s.

Generate the uppercase exceptions once upfront.
@sftse sftse force-pushed the dependency-reduction branch from d84eac7 to d48a87a Compare September 10, 2025 11:21
@sftse
Copy link
Contributor Author

sftse commented Sep 10, 2025

Given how rare it should be to regenerate the exceptional cases, if ever, given how old the format is, I've removed it entirely from the dependencies.

@jmcnamara
Copy link

jmcnamara commented Sep 12, 2025

It should be possible to avoid the once_cell dependency as well by using std::sync::OnceLock. It is stable since Rust v1.70.0.

$ git diff
diff --git a/Cargo.toml b/Cargo.toml
index 75dcc2d..db09613 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -11,7 +11,6 @@ edition = "2018"
 
 [dependencies]
 fnv = "1.0"
-once_cell = "1.21.3"
 uuid = "1"
 
 [dev-dependencies]
diff --git a/src/internal/path.rs b/src/internal/path.rs
index 753a709..bc230a8 100644
--- a/src/internal/path.rs
+++ b/src/internal/path.rs
@@ -2,8 +2,7 @@ use std::cmp::Ordering;
 use std::collections::HashMap;
 use std::io;
 use std::path::{Component, Path, PathBuf};
-
-use once_cell::sync::Lazy;
+use std::sync::OnceLock;
 
 // ========================================================================= //
 
@@ -20,7 +19,6 @@ impl CaseMapper {
 }
 
 const MAX_NAME_LEN: usize = 31;
-static CASE_MAPPER: Lazy<CaseMapper> = Lazy::new(CaseMapper::new);
 
 // ========================================================================= //
 
@@ -28,12 +26,15 @@ static CASE_MAPPER: Lazy<CaseMapper> = Lazy::new(CaseMapper::new);
 /// using simple capitalization and the ability to add exceptions.
 /// Used when two directory entry names need to be compared.
 fn cfb_uppercase_char(c: char) -> char {
+    static CASE_MAPPER: OnceLock<CaseMapper> = OnceLock::new();
+    let case_mapper = CASE_MAPPER.get_or_init(CaseMapper::new);
+
     // TODO: Edge cases can be added that appear
     // in the table from Appendix A, <3> Section 2.6.4
 
     // Base case, just do a simple uppercase
     // equivalent to icu_casemap::CaseMapper::new().simple_uppercase(c)
-    CASE_MAPPER.simple_uppercase(c)
+    case_mapper.simple_uppercase(c)
 }
 
 /// Compares two directory entry names according to CFB ordering, which is
@@ -222,6 +223,8 @@ mod tests {
     #[ignore = "add icu_casemap to dependencies to regenerate exceptional uppercase chars"]
     #[test]
     fn uppercase_generation() {
+        use crate::internal::path::CaseMapper;
+
         struct AsArray(Vec<(char, char)>);
 
         impl std::fmt::Display for AsArray {
@@ -241,7 +244,7 @@ mod tests {
             }
         }
 
-        let case_mapper = &super::CASE_MAPPER;
+        let case_mapper = CaseMapper::new();
         // uncomment line to regenerate exceptions
         // let case_mapper = icu_casemap::CaseMapper::new();

@jmcnamara
Copy link

jmcnamara commented Sep 12, 2025

+1 to this PR. It would be great to use rust-cfb in Calamine but the current dependency chain is slightly off-putting.

See also: tafia/calamine#551

@sftse
Copy link
Contributor Author

sftse commented Sep 12, 2025

I was not explicitly able to determine the current MSRV, so the once_cell is to be a bit more conservative. The uuid dependency states MSRV of 1.63 and once_cell has MSRV 1.65.

If 1.70 is fine we can apply the changes you suggest, thanks.

@jmcnamara
Copy link

jmcnamara commented Sep 12, 2025

I was not explicitly able to determine the current MSRV, so the once_cell is to be a bit more conservative. The uuid dependency states MSRV of 1.63 and once_cell has MSRV 1.65.

That is fair.

However the actual MSRV required is higher.

According to cargo-msrv the MSRV on main is v1.81.0 (probably due to the icu-casemap dependencies):

$ cargo-msrv find

...

Result:
   Considered (min … max):   Rust 1.31.1 … Rust 1.89.0
   Search method:            bisect
   MSRV:                     1.81.0
   Target:                   x86_64-apple-darwin

On your branch it goes back a bit further to MSRV v1.74.1:

$ git checkout dependency-reduction


$ cargo-msrv find

...


Result:
   Considered (min … max):   Rust 1.31.1 … Rust 1.89.0
   Search method:            bisect
   MSRV:                     1.74.1
   Target:                   x86_64-apple-darwin

As a manual check I downgraded manually to rustc v1.65.0 and rust-cfb doesn't compile with that version.

It is probably worth specifying the MSRV in Cargo.toml as part of this patchset or in another PR.

@sftse sftse force-pushed the dependency-reduction branch from 6a74aea to 5e4f8b7 Compare September 13, 2025 11:15
@sftse
Copy link
Contributor Author

sftse commented Sep 13, 2025

Thanks for the investigation and conclusive evidence, the suggestions have been included.

@mdsteele mdsteele merged commit 67342d2 into mdsteele:master Sep 19, 2025
4 checks passed
@mdsteele
Copy link
Owner

Thanks!

@sftse
Copy link
Contributor Author

sftse commented Oct 14, 2025

Any chance this can be released?

@sftse sftse deleted the dependency-reduction branch October 14, 2025 12:58
@mdsteele
Copy link
Owner

Published as v0.12.0.

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.

3 participants