Don't use once_cell

[Kijewski]

once_cell upgraded the MSRV to 1.56. This breaks the use of iana-time-zone transitively even though we only use it for Android targets.

This PR replaces once_cell by using static mut + std::sync::Once. once_cell is more or less only a safe wrapper around both, but not actually needed. We already do the same in our Windows targets.

Cf. matklad/once_cell#201

[astraw]

This looks good to me. Can you merge this and publish it? Otherwise I will do it but likely won't be able to get to it until tomorrow.

[Kijewski]

Thank you for reviewing! Released as 0.1.49.

[lopopolo]

I disagree with merging this.

If consumers of iana-time-zone require working on a significantly outdated compiler, they can choose to pin an older version of once_cell in their Cargo.lock using cargo update -p once_cell --precise.

static mut is incredibly unsafe and I would prefer we don't use it.

[lopopolo]

CStr::from_bytes_with_nul_unchecked is only usable in const contexts since Rust 1.59.0, so this PR actually raises the MSRV from where once_cell would have bumped it to.

https://doc.rust-lang.org/nightly/std/ffi/struct.CStr.html#method.from_bytes_with_nul_unchecked

[Kijewski]

Actually, that would be in line with our MSRV policy not to care about tier 2 targets. :) The MSRV for tier 1 targets was still reduced.

But yes, rising it for Android was an accident. Do you want to open a PR to fix that?

[lopopolo]

I would rather open a PR to revert this change and have 0.1.49 yanked on crates.io

[Kijewski]

static mut is easy to get wrong, yes. But in here we only write to the variable once, and never read it before written to, as ensure by the use of std::sync::Once.

[lopopolo]

This PR raises MSRV on android to 1.59.0 regardless due to its use of CStr::from_bytes_with_nul_unchecked in a const context.

[lopopolo]

Additionally, per the README, the MSRV is only 1.31 for Tier 1 platforms. Android is a tier 2 platform. It seems acceptable to me to require a compiler newer than one year old on a Tier 2 platform.

[Kijewski]

The problem is that once_cell gets downloaded and evaluated not only for Android, but e.g. Linux too.

Regardless what's in their src/*, the the single line edition = "2021" in their Cargo.toml breaks everything that uses iana-time-zone for any target on older cargo versions.

[lopopolo]

If consumers of iana-time-zone require working on a significantly outdated compiler, they can choose to pin an older version of once_cell in their Cargo.lock using cargo update -p once_cell --precise.

Why is this not an acceptable workaround? We already test with -Zminimal-versions in CI to ensure this use case is supported.

[Kijewski]

Because it affects users that should not be affected at all, unless they are direct users of once_cell.

I'm happy to learn if the use of std::sync::Once + static mut in here is dangerous, not least because we would need to fix it in our windows implemention, too.

[lopopolo]

If a Rust user is not willing or able to use the latest compiler, they should expect to do surgery on their Cargo.lock file.

There exists combinations of iana-time-zone and once cell that work on old rustc. I do not believe we need to commit for all versions of our dependencies.

[Kijewski]

I still don't see what the problem is with not using the library.

Using sync::Once::call_once() to populate a static mut variable is the very example how to use Once. The only thing I did differently from the example is that I did not add an get_cached_val() function, but inlined its code.

Once_cell is great. Otherwise it would not be on track to be included in the standard library. But in here it is simply not needed, even without my original reasoning that it breaks our stated MSRV for tier 1 platforms.

[Kijewski]

@djc, @esheppa, do you have an opinion on that matter? Chrono is by far the biggest user of this library, so I guess you are interested in the change, whether you like it or not, too. :)

[matklad]

I think this is the right change: its worth it to just cut the number of deps. The only thing I'd do differently is pulling the statics inside the function, so that it's clear no one else accesses it.

And yeah, it's a shame that Cargo gets broken by a dep which shouldn't even apply to the target.

[esheppa]

@Kijewski and @lopopolo - what about using a thread_local! and RefCell like we use here. It may have more runtime cost or be slightly less efficient, but it should still work in this case. Another benefit here is that you'd be able to use CStr::from_bytes_with_nul_unchecked() in a non-const context if desired (when initializing the thread local)

[Kijewski]

The only thing I'd do differently is pulling the statics inside the function, so that it's clear no one else accesses it.

@matklad, yes, that's a good idea! I opened #62 to do that, and fix that I raised the MSRV on Android by using CStr::from_bytes_with_nul_unchecked() in a const context.

[Kijewski]

@esheppa, AndroidSystemProperties is sync + send, so I don't think that it would be better to use thread_local!. The problem is, that calling AndroidSystemProperties::new() is extremely slow. I think most threads in rust are long lived, so that the call might not happen that often, but I guess we should not depend on it.

[esheppa]

As a further thought - is it possible that calling AndroidSystemProperties::new() will return a different timezone on subsequent calls? Should this be effectively cached for the lifetime of the application, or is there an alternative caching strategy that would allow changes in timezone to propagate?

[Kijewski]

AndroidSystemProperties is like std::env, if it were an object. We still call get_from_cstr() on every invocation, so if the user is e.g. flying on a plane and updating their time zone during the flight, the information will always be current.

[djc]

I think this is great, and even better with #62. Thanks for addressing the MSRV hazard!

@lopopolo as a point of feedback, I think you were very insistent (to the extent that it feels almost combative to me, writing things like "I would rather open a PR to revert this change and have 0.1.49 yanked on crates.io" is really not a great way to interact IMO) in this issue that this is not the right fix, but (especially given that you recently advocated for getting rid of unnecessary dependencies in other places) I didn't see any clear rationale for why you dislike this change so much.

[lopopolo]

Thanks for the feedback @djc.

There have been several changes to this crate that replace safe abstractions provided by foundational dependencies with bespoke unsafe code. #35 is another such change.

My personal preference is to not be bound by MSRV and use battle tested abstractions to obviate the need for writing unsafe code.

I understand there are other stakeholders here that have different needs.

[djc]

Sure, that makes some amount of sense. As an application author, I also prefer using safe abstractions provided by foundational dependencies. However, as a library maintainer I think it can definitely be reasonable to allow a wide audience to more easily gain access to the code, and I feel adding small amounts of unsafe code with easily verifiable invariants is a decent trade-off. In the end once_cell probably also contains unsafe code.

As Rust gains more popularity it might become a more reasonable trade-off to require stalwart users/packagers to use a more recent compiler, and I think moves like this from once_cell (and potentially libc in the future?) might accelerate that future. Still, as a library maintainer I think it's reasonable to accept some not too high cost in order to make it easier for downstream users to consume my code.

[lopopolo]

I would rather open a PR to revert this change and have 0.1.49 yanked on crates.io

For what it is worth, I have commit rights on this repository and do believe it is reasonable to discuss whether changes should be reverted or yanked. That being said, I do agree with you that this particular comment was unnecessarily combative.

[lopopolo]

Still, as a library maintainer I think it's reasonable to accept some not too high cost in order to make it easier for downstream users to consume my code.

Yes that's correct and is something I've come around to since posting my initial comments.

@matklad's proposal and #62 do make me much more comfortable with this change. Thank you folks.

Particularly with chrono as a reverse dependency, it is important to be sensitive to the deployment practicalities of real users when making choices around MSRV.

[djc]

For what it is worth, I have commit rights on this repository and do believe it is reasonable to discuss whether changes should be reverted or yanked. That being said, I do agree with you that this particular comment was unnecessarily combative.

Ah, I was not aware that you have commit rights on this repo. I definitely agree it is reasonable to discuss whether changes should be reverted or yanked, I was merely objecting with the way that comment came across.

Thanks for being open to my feedback and reconsidering your position (to some extent), much appreciated!

[astraw]

From my side, I am grateful to you all for pitching in here!

It is comforting to have all you of you looking at what is ultimately a pretty limited sized bit of code. It is somewhat surprising how many potential footguns there are here!

[djc]

Thank you, too! I think the ecosystem would be clearly less well off if this platform-specific complexity was duplicated all over crates like chrono.

[matklad]

The problem is that once_cell gets downloaded and evaluated not only for Android, but e.g. Linux too.

This still bugs me. It feels like a cargo bug -- perhaps it should warn on edition mismatch or something, instead of outright erroring. If someone could look into that (eg, search cargo's issue tracker for exisgin bug to this end) that might be useful.

I'd do it, but I happen to change another Cargo bug at this point already :)