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.

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

Ship pre-built library by default? #192

Open
orlp opened this issue Jan 23, 2025 · 3 comments
Open

Ship pre-built library by default? #192

orlp opened this issue Jan 23, 2025 · 3 comments

Comments

@orlp
Copy link

orlp commented Jan 23, 2025

As far as I can see, since the timezone database is vendored into the released crate anyway, if you don't set CHRONO_TZ_TIMEZONE_FILTER, every user will spend time and energy compiling the following set of packages to get exactly the same result as everyone else:

[build-dependencies]
└── chrono-tz-build v0.4.0
    ├── parse-zoneinfo v0.3.1
    │   └── regex v1.11.1
    │       ├── aho-corasick v1.1.3
    │       │   └── memchr v2.7.4
    │       ├── memchr v2.7.4
    │       ├── regex-automata v0.4.9
    │       │   ├── aho-corasick v1.1.3 (*)
    │       │   ├── memchr v2.7.4
    │       │   └── regex-syntax v0.8.5
    │       └── regex-syntax v0.8.5
    └── phf_codegen v0.11.2
        ├── phf_generator v0.11.2
        │   ├── phf_shared v0.11.2
        │   │   └── siphasher v0.3.11
        │   └── rand v0.8.5
        │       └── rand_core v0.6.4
        │           └── getrandom v0.2.15
        │               ├── cfg-if v1.0.0
        │               └── libc v0.2.169
        └── phf_shared v0.11.2 (*)

This seems like a lot of wasted time and energy.

I suggest making CHRONO_TZ_TIMEZONE_FILTER locked behind an optional feature, and just use a pre-baked chrono-tz version by default so that none of the build-dependencies need to be built.

@djc
Copy link
Member

djc commented Jan 24, 2025

I agree that there is a lot of wasted time and energy, though having two separate ways to build the library has its own maintenance overhead. Would you be interested in proposing a PR to improve on the status quo along the lines you suggested?

@orlp
Copy link
Author

orlp commented Jan 24, 2025

@djc So something along the lines of this?

#![allow(non_camel_case_types, clippy::unreadable_literal)]

#[cfg(feature = "timezone-filter")]
include!(concat!(env!("OUT_DIR"), "/timezones.rs"));

#[cfg(not(feature = "timezone-filter"))]
include!("prebuilt_timezones.rs");

I'm not sure by the way whether this would be considered a breaking change or not. It means that without setting the timezone-filter feature you would suddenly not get a filtered set of timezones anymore making your binary bigger, but I believe anyone depending on chrono-tz would continue to work as you'll only get more functionality than you asked for.

@orlp
Copy link
Author

orlp commented Jan 24, 2025

I also believe we need to pin a specific version of phf, because any bumps to its version could potentially invalidate the pre-built mapping otherwise.

Arguably this is already broken in chrono-tz today. I don't believe there is anything in-place to ensure the version of phf matches that of phf_codegen exactly, which is not a problem today but if 0.12 comes out it could become a problem if embedded in a program that pins phf to 0.11 but nothing pinning phf_codegen or vice versa.

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

No branches or pull requests

2 participants