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

Diplomat MSRV? #672

Open
sffc opened this issue Sep 3, 2024 · 9 comments
Open

Diplomat MSRV? #672

sffc opened this issue Sep 3, 2024 · 9 comments

Comments

@sffc
Copy link
Contributor

sffc commented Sep 3, 2024

Multiple times, Diplomat has updated and it has been tricky to integrate into ICU4X due to the MSRV of dependencies.

Diplomat should, at the very least, be explicit about its MSRV. Currently it is 1.71, for which I added a new CI check.

https://github.com/rust-diplomat/diplomat/actions/runs/10689442469/job/29631556841

Additionally, Diplomat should ideally share an MSRV with ICU4X given ICU4X's important role as a major client of Diplomat.

@Manishearth
Copy link
Contributor

Manishearth commented Sep 3, 2024

I'm happy with diplomat_core/diplomat/diplomat_runtime having an MSRV (and that MSRV being constrained by ICU4X needs). I'm not really in favor of a CLI tool like diplomat-tool having an MSRV.

This is because CLI tools tend to require a much less conservative dependency tree (and this will continue as we add more backends, more customizeability, and things like better UX), and we have historically not tried to keep diplomat-tool low-dependency.

ICU4X MSRV CI breaking because of diplomat-tool's deps is purely a tooling problem on ICU4X's side and has nothing to do with the actual reason ICU4X has an MSRV in the first place.

@Manishearth
Copy link
Contributor

In ICU4X's case we can decouple msrv from the all-features tooling by adding an all-features metadata key that allows it to skip specific crates.

@sffc
Copy link
Contributor Author

sffc commented Sep 3, 2024

Well, if ICU4X has an MSRV, then all of ICU4X's library crate dependencies should be able to have that MSRV. I would include icu4x-datagen in that. Dependencies that are only repository tooling (cargo-make, dhat, ...) might be an exception.

I didn't take a super close look at exactly where pulldown-cmark was coming from.

@Manishearth
Copy link
Contributor

Manishearth commented Sep 3, 2024

Well, if ICU4X has an MSRV, then all of ICU4X's library crate dependencies should be able to have that MSRV.

Yes, diplomat-tool is not a library crate dependency. It is an internal repository-only-tooling dependency of tools/make/diplomat-gen 1. Both clap and pulldown-cmark are deps of diplomat-tool.

I would include icu4x-datagen in that

I'm not 100% sure I agree2, however I don't think it's that relevant: I think in icu4x-datagen we have been trying to be careful about deps, and have been trying to keep them low with some success. I'm comfortable with restricting its MSRV.

I don't want such limitations on diplomat-tool, especially as it grows more backends. We've had a bunch of plans for better UX (especially around error reporting) on diplomat-tool that may involve pulling in more deps, and I'd like to reserve the right to do that in the future.

Large deptrees are the natural state of Rust CLI tooling in a way that isn't the case for Rust libraries, and whilst icu4x-datagen is in a position to keep a lid on it, I'm not sure I want diplomat-tool to also restrict itself that way.

Footnotes

  1. In theory we could make diplomat-tool have Cargo features so that clap is unnecessary for those using it "as a library". Wouldn't fix the pulldown_cmark issue though.

  2. Do the reasons we restrict ICU4X lib MSRV apply to icu4x-datagen as well? Perhaps. I'm not sure they do.

@sffc
Copy link
Contributor Author

sffc commented Sep 4, 2024

I include icu4x-datagen because people often include it as a dependency to be invoked during their build: it's something users of icu4x often need to run. diplomat-tool is something users of diplomat need to run, so it should I think conform to Diplomat's MSRV policy, but it's not generally something users of icu4x need to run, but only because we publish our header files in the repo.

@Manishearth
Copy link
Contributor

I include icu4x-datagen because people often include it as a dependency to be invoked during their build: it's something users of icu4x often need to run

Right, I agree, but I'm not sure that the subset of users with MSRV constraints on their libraries are likely to have the same constraints on icu4x-datagen use (potentially because they don't use a build script).

I'm not saying such a thing isn't possible, I'm just not sure (but also that it's not super relevant to this discussion, since icu4x-datagen currently has no troubles following MSRV).

diplomat-tool is something users of diplomat need to run, so it should I think conform to Diplomat's MSRV policy

No, I think the ontology here is subtler, diplomat-tool is used by users of diplomat, but diplomat_core, diplomat, and diplomat_runtime are depended upon by users of users of diplomat (like ICU4X). I think the two can and probably should have different MSRV policies (probably with diplomat-tool not having an MSRV at all, just supporting stable).

Basically, I don't want the arbitrary constraint of "Diplomat has a single MSRV policy" to tie diplomat-tool's MSRV to ICU4X. It's quite reasonable for the other three Diplomat crates to follow in ICU4X's MSRV footsteps and adopt roughly the same MSRV cadence. But I don't see reason for diplomat-tool to follow that as well.

@robertbastian
Copy link
Collaborator

I'm with Manish here, this is an ICU4X tooling problem.

@Manishearth
Copy link
Contributor

Concrete proposal: diplomat_core, macro, and runtime follow the same MSRV policy as ICU4X ("update for free when > 6 versions, needs justification for > 4 versions, disallowed < 4"), with the additional caveat that by default they are also constrained by ICU4X's MSRV as well. Of course, reasons to update diplomat MSRV may also be valid reasons to update ICU4X MSRV: diplomat may provide justification to the ICU4X team and update both at the same time.

diplomat-tool follows no MSRV policy. I am okay with documenting its MSRV though I don't really think that's valuable since diplomat-tool doesn't do weird unsafe Rust stuff so the primary drivers of diplomat-tool's MSRV are its dependencies (and people can lockfile constrain as needed)

@Manishearth
Copy link
Contributor

I made a PR for the policy above in #683 , and I've tweaked CI to match.

I've left the door open for other clients to ask us to suit their needs as well.

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

3 participants