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

Workaround for type inference breakage in the time crate #14452

Closed
wants to merge 1 commit into from

Conversation

kornelski
Copy link
Contributor

The time crate is widely used, and the compatibility issue rust-lang/rust#127343 affects many crates. This may have long-lasting consequences, as the broken versions of the time crate are locked in lockfiles.

If @jhpratt doesn't mind this, I propose here breaking a taboo, and slightly modify the already published crates to fix the incompatibility.

With this patching-patch the affected time crates build again with Rust 1.80. It's an ad-hoc bit of code in tar unpacker, because hopefully Cargo won't need a proper infrastructure for this.

@rustbot
Copy link
Collaborator

rustbot commented Aug 25, 2024

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-registries Area: registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 25, 2024
Copy link
Member

@jhpratt jhpratt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While certainly unconventional, this does work around the problem about as much as possible.

Without checking the version bounds (I presume you did), the patch itself is acceptable to me. Whether this should be done is presumably up to the relevant team, so I'll defer to them on that.

Edit: For precedent, I think that if this is ever done in the future, it should only ever be done with explicit permission from someone with publish permissions on crates.io.

@steffahn
Copy link
Member

steffahn commented Aug 26, 2024

I wonder what this does in case you already have the relevant time package(s) uncompressed in your .cargo/registry/src/…

Which (if it doesn’t work properly) is not only a transitional issue; people may also in the future run an old rust version from time to time, and that might always end up being the time that time is uncompressed, right?

@tgross35
Copy link

Repeating a suggestion from the possible mitigations in the compiler: it may be good to add a // FIXME(2025-Q2): ... as a reminder that this should be revisited at some point in the future. It might get dropped at that point or it might be extended based on how much usage of the old version still exists, but just something to note this isn't intended to be a permanent hack in Cargo.

@ruuda
Copy link
Contributor

ruuda commented Aug 26, 2024

I can’t judge if NixOS/Nixpkgs is a large enough ecosystem for the Rust project to take it into account, and how that relates to other ecosystems affected by the 1.180 breakage. I also don’t know enough about the details of how Cargo unpacks crates or how exactly cargoLock / cargoVendorDir works in Nixpkgs. But I do want to flag potential side affects that this change might have.

I know that Rust packages in Nixpkgs involve creating a vendor directory, and every Rust application in Nixpkgs contains a checksum of its Cargo dependencies to ensure it’s reproducible and the sources don’t change. (To ensure reproducibility, Nix builds applications in a sandbox without network access, so Cargo can’t download dependencies itself.) Given the fallout 1.80 has already caused for Nixpkgs, I at least want to flag this. If this change would invalidate most cargoSha256/cargoHashes stored in Nixpkgs then this change would effectively double the churn that has already started. From what I can tell from a quick glance, Nix downloads and unpacks the crates itself, so it would not benefit from this workaround but at least it would not be harmful.

If this change is going to be merged into Cargo, I can help to test its impact on Nixpkgs.

@alyssais
Copy link

For Nixpkgs, I think the best thing would be to have an environment variable to turn this off, given we've already mostly handled the breakage anyway. I imagine an opt out for Cargo modifying crate sources is desirable for other reasons as well.

@kornelski
Copy link
Contributor Author

This is applied only when unpacking. For users with the crate already in unpacked, I'd need to add some extra check.

Perhaps patching could copy the whole crate to another directory in case tools check the integrity of the original dir, but I'll need to check if that works with the rest of the build pipeline.

@epage epage marked this pull request as draft August 26, 2024 14:39
@epage
Copy link
Contributor

epage commented Aug 26, 2024

@kornelski big changes like this are generally best handled in issues, rather than PRs. We prefer PR discussions to focus on the implementation once a solution has been found.

When moving this to an appropriate place to discuss, I think an important element to the discussion would be where is the appropriate place to put this hack. For example, is there something the compiler could be doing? Cargo and rustc both have package-specific hacks and it would be good to identify where we should be improving this.

Longer term, if we had #3177 and a way for registries to suggest patches (see also https://blog.rust-lang.org/inside-rust/2024/03/26/this-development-cycle-in-cargo-1.78.html#why-is-this-yanked), we could generalize a concept like this.

@GoldsteinE
Copy link

Is this a big change? I see it as a hotfix for a mistake that broke substantial parts of the ecosystem. While discussion on how these hotfixes should be done in general is no doubt useful, trying to properly come up with a general solution before merging this would delay this hotfix in particular indefinitely. As its usefuleness declines with time (as people come up with their own workarounds), I think it should either be merged somewhat quickly or just cancelled, since by the time a proper general decision could be made, it would no longer be relevant.

@emilazy
Copy link

emilazy commented Aug 26, 2024

It’s pretty big, in my opinion. It silently alters the source tree of a huge number of historical crate dependency graphs from what their locked hashes say they should be. It seems like a dangerous precedent to set and the compiler would be the more appropriate way to hack around this, if a hack is desired. I’m biased because it’s bad for Nixpkgs, but I don’t think we’d be the only ecosystem with serious concerns about this.

(Ideally the compiler would have deployed a workaround before it hit a stable release…)

@weihanglo
Copy link
Member

Is this a big change?

Yes.

At code level, any change in Cargo might affect some other workflows we didn't notice, like #14452 (comment) Nixpkgs one potentially. Would it break projects using [patch] or source replacements? Would it put cargo-vendored projects in a mixed state? Even a hotfix seems pretty valid, we still need to carefully review and plan.

At policy level, how do we evaluate to accept a hard-coded fix? By thumb-up counts in this issue? By prevalence of the crate? I don't think we should accept a pull request without clearly establishing a boundary. There might be more requests like this one if we don't discuss.

@epage
Copy link
Contributor

epage commented Aug 26, 2024

While discussion on how these hotfixes should be done in general is no doubt useful, trying to properly come up with a general solution before merging this would delay this hotfix in particular indefinitely.

To be clear, I was not saying this had to be blocked on those longer term items. This is blocked on the shorter term discussion of "what are our options and what is the best of those" which needs to be discussed across teams.

This is an invasive change and a major policy change and should not be done lightly, as others have said.

@epage
Copy link
Contributor

epage commented Aug 26, 2024

As this is initially a cross-team discussion, I started a focused thread at https://internals.rust-lang.org/t/brainstorming-how-to-help-old-code-locked-to-old-time-build-on-new-toolchains/21438/2

@ehuss
Copy link
Contributor

ehuss commented Aug 28, 2024

We understand that the time issue has caused a lot of unexpected churn and like you are interested in finding ways of improving things now and in the future. To that end, we devoted most of @rust-lang/cargo's meeting to discuss this topic. We considered the possibility of patching the specific ranges of versions of the time crate, by various means. We're concerned about trying to ship a feature like this in a hurry, and potentially unexpected fallout of it, now and in the future:

  • Cargo crossing the deontological line of patching users' sources and not building exactly the original sources.
  • Having the user be surprised by this patching (e.g. because they were expecting the failure and were testing the failure).
  • Having to take this patching into account for future features like caching and verification.

Given that, we would prefer to not move forward with a short-term fix for this issue with the time crate. We are however considering solutions that could help in the future. We have opened #14458 to explore those ideas. In particular, we would like to continue with more incremental work that has already begun, such as:

  • Yank reasons which is currently a Project Goal, and has the potential to eventually ease towards a mutable database that could possibly support registry patches.
  • Experiments with patches. That work halted due to concerns over resolver interactions. Resolver interaction was a major component behind that change, but a situation like this with time shows that perhaps a solution that does not support Cargo.toml changes would be useful.
  • Providing an interface between rustc and cargo so that either side can better support reporting diagnostics related to situations like this. (Add a new --orchestrator-id flag to rustc compiler-team#635 is tangentially related to this.)

A mitigation is being added to rustc via rust-lang/rust#129343. We acknowledge that likely does not go far enough for many since it only provides a diagnostic and not a fix.

Other teams, including libs-api, lang, compiler, and release, are also working on improvements to try to better handle or prevent situations like this.

@ehuss ehuss closed this Aug 28, 2024
@kornelski kornelski deleted the go-back-in-time branch August 28, 2024 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-registries Area: registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.