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

home: reduce MSRV to 1.64 #13270

Closed
wants to merge 1 commit into from
Closed

home: reduce MSRV to 1.64 #13270

wants to merge 1 commit into from

Conversation

djc
Copy link
Contributor

@djc djc commented Jan 9, 2024

In #12654 the MSRV for home was set to #12654. The actual crate still works fine with 1.64 (except for some warnings about the unrecognized lints key). Though I read the discussion in the original PR (and so I recognize this PR might be controversial), I would like to suggest that for a crate as popular as home (with 9.1M recent downloads) -- and as small/low on change -- IMO it would be nice to be a little more conservative and at the very least avoid bumping the MSRV proactively unless there is a need to do so.

(As a library maintainer of a bunch of very popular libraries, I am pretty unhappy with a N - 2 sliding window approach to MSRV -- asking people to justify an MSRV bump is a good thing IMO especially if the bump is aggressive -- or if you want to do a sliding window, at least be more conservative about the window; at least N - 4, preferably more like N - 6 or N - 8. Most of the libraries I maintain are currently in the 1.61 - 1.63 range.)

@rustbot
Copy link
Collaborator

rustbot commented Jan 9, 2024

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-environment-variables Area: environment variables A-home Area: the `home` crate S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2024
@djc
Copy link
Contributor Author

djc commented Jan 9, 2024

(If there is a need to do any legwork to make this work better in CI, I can probably do that work if the idea of this PR is accepted.)

@epage
Copy link
Contributor

epage commented Jan 9, 2024

Could you describe the problem you are trying to solve.

For example,

  • Make dependency updating painless until MSRV-aware resolver is out
    • This means we specifically need a wider MSRV policy than you, accounting for justification-required MSRV updating which is a variable policy making it difficult to target
  • Needing a specific change: what?
  • Reducing risk for if there is a bug, feature, or security fix we need

@djc
Copy link
Contributor Author

djc commented Jan 12, 2024

The problem I'm trying to solve is that I have a downstream library for which I'd like to maintain a less aggressive MSRV. In general, it seems wasteful to restrict a popular library to a higher MSRV than what it really needs, since this in turn restricts all downstream libraries to a higher MSRV than what is actually needed.

@epage
Copy link
Contributor

epage commented Jan 12, 2024

Our choice of MSRV does not affect the MSRV of dependents except in their risk profile to catastrophic bugs or vulnerabilities.

@djc
Copy link
Contributor Author

djc commented Jan 12, 2024

Our choice of MSRV does not affect the MSRV of dependents except in their risk profile to catastrophic bugs or vulnerabilities.

Because MSRV-aware cargo update will just prevent dependents from getting newer versions of the library? That really seems more like a workaround rather than something to rely on.

@weihanglo
Copy link
Member

Although people should use other alternative than home for getting home path, admittedly it is a widely-used package that I've seen such a msrv complaints from several channels.

Looking back the PR #12381 proposing MSRV for all member packages, we've listed some concerns aware #12381 (comment). To me, I am fine lowering the MSRV for home specifically, as it is not even released with the standard Rust release. This is documented:

This library is shared between cargo and rustup and is used for finding their home directories.
This is not directly depended upon with a path dependency; cargo uses the version from crates.io.
It is intended to be versioned and published independently of Rust's release system.
Whenever a change needs to be made, bump the version in Cargo.toml and cargo publish it manually, and then update cargo's Cargo.toml to depend on the new version.

In #13167 we opt out home from semver-checks against beta/stable Rust release, so should be fine adding more special treatments.

@epage
Copy link
Contributor

epage commented Jan 17, 2024

Keep in mind a lower MSRV for home affects cargo development. We have a PR up where we can't use the latest features in some packages because of our N-2 packages. We also share deps but home only depends on windows-sys.

@weihanglo
Copy link
Member

We have a PR up where we can't use the latest features in some packages because of our N-2 packages

#13254 (comment) as an example of this.

@taiki-e
Copy link
Member

taiki-e commented Jan 17, 2024

We have a PR up where we can't use the latest features in some packages because of our N-2 packages

#13254 (comment) as an example of this.

As I said in #13254 (comment), as far as I know, it is possible to avoid the problem while maintaining a low MSRV, at least for this problem.

@bors
Copy link
Collaborator

bors commented Jan 19, 2024

☔ The latest upstream changes (presumably #13324) made this pull request unmergeable. Please resolve the merge conflicts.

@djc
Copy link
Contributor Author

djc commented Jan 19, 2024

Keep in mind a lower MSRV for home affects cargo development. We have a PR up where we can't use the latest features in some packages because of our N-2 packages. We also share deps but home only depends on windows-sys.

Maybe we should move home out of this repo to avoid it making Cargo development harder? I'd volunteer to help out as a maintainer.

@epage
Copy link
Contributor

epage commented Jan 20, 2024

Maybe we should move home out of this repo to avoid it making Cargo development harder? I'd volunteer to help out as a maintainer.

As there isn't any functionality being blocked from getting the users with older MSRVs, I feel like that time would be better spent on getting this back into the standard library.

I will go ahead and propose we talk about this in the cargo team meeting.

@epage epage added the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Jan 20, 2024
@epage
Copy link
Contributor

epage commented Jan 23, 2024

We talked about this in the cargo team meeting. We understand that home is used by a lot of people. However, it is also a "mature" crate for their purposes and the latest versions exist mainly for logistical reasons. From this, we need to weigh out the cost of applying a longer MSRV. We are skittish about applying a lazy MSRV update policy, especially because of the burden users place on maintainers to justify an MSRV change. An MSRV policy also has affects on all packages within the repo. cargo hack exists but excluding private packages only helps with some of the problems. Overall, the long term solution for this is to get the core function back into the standard library so long as T-libs-api has interest in that (which they seemed to last we saw).

In an effort to balance the different needs, we are willing to extend the N-2 policy to N-4 as a bridge for users while an ACP is being worked on. This offer is only good so long as the ACP is being actively driven to a resolution or else this turns from a bridge to a general policy.

@djc
Copy link
Contributor Author

djc commented Jan 23, 2024

What are the logistical reasons? Was the idea of moving this into a separate repository discussed at all? If others (like, me) would volunteer to help maintain and shoulder the burden to justify MSRV changes and the rest of the packages within this repo would no longer be affected, that seems to address all of the concerns listed here. I am also able and willing to do the legwork to set this up in a separate repository (whether within rust-lang or somewhere else), bringing along the Git history so far.

Getting this into std is a good idea but will take another six months. I honestly don't see only upside in getting this into a separate repo where a more broadly accessible MSRV can be supported.

@epage
Copy link
Contributor

epage commented Jan 23, 2024

The logistical reasons are that we've automated when a package must be released based on if there are any changes to the directory because we've had too many times where we incorrectly publish packages from the cargo repo.

We briefly talked about splitting the workspace or moving it to a separate repo but both of those take work and short term efforts and we felt it better to focus on the long term effort, given what seems like a low user impact from this situation.

@ehuss ehuss removed the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Jan 23, 2024
@djc
Copy link
Contributor Author

djc commented Jan 25, 2024

Well, that's disappointing.

@djc djc closed this Jan 25, 2024
@epage
Copy link
Contributor

epage commented May 29, 2024

FYI rust-lang/libs-team#372

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-environment-variables Area: environment variables A-home Area: the `home` crate S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants