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

Only apply MSRV to diplomat-runtime #678

Merged
merged 2 commits into from
Sep 6, 2024

Conversation

robertbastian
Copy link
Collaborator

@Manishearth
Copy link
Contributor

core/macro as well, right?

I'm open to having the core MSRV only apply in ast mode though so far aside from potential error handling libraries like annotate-snippets I've not really wanted more add deps for hir.

@robertbastian
Copy link
Collaborator Author

Macro yes, core I'm not sure. I previously suggested to move HIR to tool because it's only used by the tool.

@Manishearth
Copy link
Contributor

I think for now I'd like to mark those three libraries as having an MSRV, and we can potentially move HIR if we really want it to not use the MSRV. I'm not really in favor of doing that move, though. But I feel comfortable not needing to use cutting edge Rust features in hir.

@Manishearth
Copy link
Contributor

Manishearth commented Sep 5, 2024

If anything, the main cutting edge Rust feature I need is one that will be there in ast, which is token tree span info, which hasn't stabilized yet.

@sffc
Copy link
Contributor

sffc commented Sep 5, 2024

I have no strong opinion here except that I want to stop diplomat updates from causing pain when vendoring into the ICU4X repo. My CI that checks all projects is the most conservative solution. I might prefer that we change ICU4X's MSRV CI to check only certain projects first before making this change here in Diplomat.

@Manishearth
Copy link
Contributor

I'd like to treat these projects as separate for these discussions. From Diplomat's POV I want us to check the three non-tool crates for its MSRV, and we should land that ASAP, or back out the unreviewed MSRV change landed to main. If ICU4X wishes to improve its CI, it absolutely should, and Diplomat should not be constrained by purely internal ICU4X CI changes in that way.

@sffc
Copy link
Contributor

sffc commented Sep 6, 2024

If ICU4X wishes to improve its CI, it absolutely should, and Diplomat should not be constrained by purely internal ICU4X CI changes in that way.

I don't think I agree with this; I would rather hold back bleeding edge Rust features from Diplomat developers if it makes ICU4X integration easier, since most Diplomat features are still being developed for ICU4X. But I don't feel strongly and if you want to merge this PR, having MSRV CI even for a subset of crates is better than nothing.

@Manishearth
Copy link
Contributor

if it makes ICU4X integration easier

ICU4X should fix its CI then. I am quite happy privileging ICU4X as a client in Diplomat in cases where it actually matters, and we do this in countless different ways already. I do not wish to set a precedent for privileging ICU4X for things that fundamentally do not matter that much and are not a Diplomat-side issue.

I would rather hold back bleeding edge Rust features from Diplomat developers

It's not even about bleeding edge Rust features. It's about Diplomat developers being constrained into gardening a large CLI app dependency tree to follow an MSRV, which is a constant cost and often nontrivial. Our dependencies do not currently all have an MSRV policy, which makes this best effort.

To be clear: the cost is present regardless of what new code we add to Diplomat.

@Manishearth Manishearth merged commit 0dfb267 into rust-diplomat:main Sep 6, 2024
17 checks passed
@sffc
Copy link
Contributor

sffc commented Sep 6, 2024

ICU4X should fix its CI then.

Yes, I agree! ICU4X should fix its CI. But, I think the order of operations should be "ICU4X fixes its CI" followed by "Diplomat relaxes its MSRV strictness", not the other way around. Without that, we are likely to have conflicts again, just as we did in unicode-org/icu4x#5482. Maybe with slower development, we will be fine for the time being.

It's not even about bleeding edge Rust features. It's about Diplomat developers being constrained into gardening a large CLI app dependency tree to follow an MSRV, which is a constant cost and often nontrivial. Our dependencies do not currently all have an MSRV policy, which makes this best effort.

To be clear: the cost is present regardless of what new code we add to Diplomat.

Sure, and I would rather have the cost be upfront, rather than discovered only when an ICU4X team member opens a PR to get something done and then has to wait over a weekend while we decide how to resolve CI problems.

@Manishearth
Copy link
Contributor

Without that, we are likely to have conflicts again, just as we did in unicode-org/icu4x#5482. Maybe with slower development, we will be fine for the time being.

I'm trying to maintain a separation between the "we" that is Diplomat and the "we" that is ICU4X here. ICU4X having such conflicts is not really an issue for Diplomat. ICU4X has multiple workarounds to handle that: waiting on an update, using a branch, or fixing CI. However, ICU4X, a client, not being able to make releases, is an issue that Diplomat can and should care about.

I'm trying to maintain this separation because we are now getting more contributions and more clients (which is already benefiting ICU4X's needs!), and while I'm happy privileging ICU4X as our primary client I think maintaining a separation is important for this to continue healthily. Certainly I would be more skeptical of participating in a project where such decisions are made to ease the life of a different project I was less involved in. Treating ICU4X as an important client, but still maintaining a line between the two teams, is the way to go.

Part of what I'm worried about here is sending the wrong signal, basically.

Sure, and I would rather have the cost be upfront

From my framework of having this be two separate projects, this isn't making the cost upfront, it is moving the cost to a different team.

rather than discovered only when an ICU4X team member opens a PR to get something done and then has to wait over a weekend while we decide how to resolve CI problems

As I said, there are so many temporary ways to get around this, like using a branch, or whatever. It is rare that a PR in ICU4X must land in a short period of time anyway.

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