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

Locking triomphe is the wrong approach to preserving MSRV #440

Open
aumetra opened this issue Jul 12, 2024 · 10 comments · May be fixed by #456
Open

Locking triomphe is the wrong approach to preserving MSRV #440

aumetra opened this issue Jul 12, 2024 · 10 comments · May be fixed by #456
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@aumetra
Copy link

aumetra commented Jul 12, 2024

The recent release locked the triomphe crate version to >=0.1.3, <0.1.12 which kinda works(?) but isn't a good solution.
IMO it's fine if, by default, the MSRV breaks if there is a simple way of getting the MSRV to compile (in this case, by adding =0.1.11 to the manifest of projects that absolutely need the old Rust version).

There have been prior examples where a version lock like this has broken compilation for many downstream users, such as with the *-dalek crates, where zeroize used to be locked (1, 2, 3, 4).

MSRV should, in my opinion at least, not block new versions if there is a simple fix to getting that Rust version running.

@tatsuya6502
Copy link
Member

tatsuya6502 commented Jul 13, 2024

Thank you for your comment. We know it is not an ideal approach to lock dependency in older versions , but we had no other options.

  • [email protected] requires Rust 1.76.0 (2024-02-08) or newer, which conflicts with our MSRV policy:
    • It will keep a rolling MSRV policy of at least 6 months.

    • As of today, our policy allows Rust 1.75.0 (2023-12-28).
  • It seems [email protected] conflicted at least two downstream moka user's MSRV. (Pin triomphe to keep the MSRV #426)
    • Raising moka's MSRV does not solve their problems.

Locking dependency in older versions may become a problem in the future, but no downstream user has reported yet. Meanwhile, some downstream users already reported problems with a newer version of the same dependency.

Anyway. To prevent the potential future problem, I will remove triomphe from moka's dependency. moka uses a small portion of triomphe code base, which seems very stable. And triomphe is a fork of std::sync::Arc. I will copy necessary part of triomphe or std::sync::Arc code into moka, and modify it to meet our needs. (Their OSS licenses allow it)

@tatsuya6502 tatsuya6502 added the enhancement New feature or request label Jul 13, 2024
@tatsuya6502 tatsuya6502 self-assigned this Jul 13, 2024
@tatsuya6502 tatsuya6502 added this to the v0.12.9 milestone Jul 13, 2024
@aumetra
Copy link
Author

aumetra commented Aug 25, 2024

Is there any reason why those downstream users can't pin triomphe themselves?
I don't see why they would need moka to pin versions for them (and potentially breaking downstream uses of moka because of this requirement) if they can just fix it themselves.

It also seems like this approach of making users pin crate versions themselves isn't foreign to moka crates, looking at the .ci_extras directory over on the mini-moka repo (https://github.com/moka-rs/mini-moka/blob/main/.ci_extras/pin-crate-vers-msrv.sh)

@tatsuya6502
Copy link
Member

tatsuya6502 commented Aug 27, 2024

Is there any reason why those downstream users can't pin triomphe themselves?

No. Anybody can pin trionmphe version in their Cargo.lock by using cargo update -p PACKAGE --precise VERSION. However, Cargo ignores Cargo.lock provided by dependencies, so if they distribute library crates, pinning in Cargo.lock has no effect for their downstream users.

I don't see why they would need moka to pin versions for them

The main purpose to pin triomphe in moka is to keep our MSRV, not to keep their MSRVs. We needed it for ourselves in the first place.

Our policy ensures moka to compile with Rust versions at least 6 months old. [email protected]'s MSRV is Rust 1.76.0 (2024-02-08), and at the time v0.1.12 was released, Rust 1.76.0 was only 3 moths old. If moka depended on [email protected], it meant its MSRV was broken.

As for dashmap in mini-moka, I did not pin it in Cargo.toml but pinned it in Cargo.lock only for our CI. This is because the MSRV of the latest dashmap (5.x) did not conflict with mini-moka's 6-month MSRV policy. [email protected]'s MSRV is Rust 1.65.0 2022-11-03. So I was thinking if mini-moka user reports mini-moka's MSRV Rust 1.61.0 2022-05-19 is broken, I would simply raise the MSRV to Rust 1.65.0.

@tatsuya6502
Copy link
Member

As I said before, I agree with you that locking triomphe in Cargo.toml is a wrong approach.

We know it is not an ideal approach to lock dependency in older versions , but we had no other options.

Now it is August 2024 and Rust 1.76.0 is 6 months old. We now have following options:

  1. Raise moka's MSRV from 1.65.0 to 1.76.0 and unpin triomphe version in Cargo.toml.
  2. Remove triomphe from moka's dependency. Take necessary codes from triomphe into moka.

I will take 2. because moka uses only a small portion of triomphe code base, which seems very stable.

After writing my first comment on July 13th, 2024, I did some experiments to take necessary triomphe code into moka. I was able to solve compile errors, but I was thinking to clean the code up further. Then I had no time to make any progress. I am hoping I will have some time in mid September to finish it.

moka-triomphe

@aumetra
Copy link
Author

aumetra commented Aug 27, 2024

Ah, don't worry. I didn't mean to annoy you, but if you need help with integrating the vendored code, I'd be happy to help out.

@GnomedDev
Copy link

I don't think vendoring is the correct decision in any situation. How about talking to upstream about lowering their MSRV and committing your cleanups upstream?

@aumetra
Copy link
Author

aumetra commented Sep 8, 2024

Seems like their MSRV bump was to make the behaviour more consistent/correct. I'm pretty sure they wouldn't reintroduce those kinds of things (which is a, quote, "longrunning potential soundness issue that got fixed with a new API") "just" to support an around half a year old compiler version.

Issue commenting about this: Manishearth/triomphe#93 (comment)
MSRV bump PR: Manishearth/triomphe#93

@GnomedDev
Copy link

Okay, so even if this library vendored triomphe with a lower MSRV there would be soundness problems. I personally don't see any issue with the current situation of MSRV 1.76 and using upstream triomphe.

@tatsuya6502
Copy link
Member

Thanks for sharing your concerns.

When we locked the triomphe version to an older one, I reviewed relevant codes and concluded that both moka and mini-moka were not affected by the soundness problem. I believe it would occur only when storing a zero sized type in triomphe::Arc and we do not store such a type.

triomphe started as a fork of std::sync::Arc to remove the weak references from Arc. We only want that version of Arc without weak references to reduce memory overhead. We use a few number of Arcs per cached entry and the type of the weak reference counter is usize (8 bytes in 64 bit systems).

Now triomphe has many added features that we do not need, so I thought this would be a good time to move from triomphe to a simple fork of std::sync::Arc.

I do not have time right now, but I am still hoping I will have some time to work on it in coming weekend. So please let me try it.

@GnomedDev
Copy link

Rust will dead code eliminate any features not used, and forking will just mean you no longer get any fixes and maintenance... alongside leading to duplicate code being compiled if someone does bring triomphe in to their dependency tree somehow. This just seems like a very bad idea all around to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants