-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Update to the 2021 edition via cargo fix
breaks code
#88903
Comments
Thanks for your bug report. This is expected with Cargo's new
But I'm guessing you're aware of that, considering you reported that issue that's linked there, rust-lang/cargo#9450, and you're the one who suggested adding this very notice and the |
Yes I'm well aware of this. I've raised this bug report because I do not feel that these messages are on the level that is expected for the edition migration mechanism. As far as I'm aware most of them where written with (That all written it is unfortunately not possible to fix that on diesels side as fixing this does require a major version bump. At least in my opinion this is very unfortunate that such a change is "forced" by the language itself without offering the necessary tools to circumvent those issues at library level.) |
I'm assuming that was not done that automatically because it's hard to make sure it's correct. @ehuss, I'm assuming you know more about this?
Our goal with the automatic migrations was never to be fully perfect. The edition blog post states: "The automated migrations are not necessarily perfect: there might be some corner cases where manual changes are still required." Cargo already warns about all resolver related changes during the migration, and for We can try to improve
Feel free to suggest a better message to display from
Agreed. I'd be very happy to see a concrete proposal for that. :) |
Yea, unfortunately we don't have the capability to modify TOML files in cargo at this time. |
Thanks for clarifying that. I've looked again into the corresponding RFC and you are right, both state that automated migrations should only work in most of the cases. Doing a crater run is there definitively a way to measure the breakage, but I'm not sure if that's the correct interpretation of the result numbers. The combination That written another thing I noticed while reading RFC 2052 and RFC 3085 is that RFC 2052 states the following:
(Source) As of now that's not the case for the example provided. It compiles without any warning using the 2018 edition, but fails to compile with the 2021 edition.
Let me clarify that I only suggested that this is an acceptable workaround for the situation where people manually switch to the new
It's totally understandable to have such an approach but please keep in mind that you only shift the work from one volunteer project (cargo and the edition upgrade) to another one (diesel). Additionally you are doing this without given us any tool to handle this situation in any way, at least not without releasing a breaking change release at least while using this proposed solution.
It would already be an improvement if |
Note that next to all the crates from crates.io, crater also tests all crates from public GitHub repositories that have a |
I've found a way to fix this in diesel without issuing a breaking release. It would be great if the message in |
I've issued a new diesel (1.4.8) to address this. Would be great to get the messages updated on cargo/rusts side till the next release. If someone points me into the right direction (reading where to change that + where to open a PR) I might find some time to work on this this week. |
That's good news! Are you suggesting that the messages can be removed? Here are the two places:
|
I think it's better to change the message in such a way that it just suggests updating diesel instead of completely removing the message. I've opened a PR(rust-lang/cargo#9927) for this. |
Change diesel compatibility messages Diesel 1.4.8 fixes the critical behaviour. This commit changes the corresponding messages for `cargo fix` and normal builds to prompt the user to just update the diesel version to fix the corresponding compilation errors. As discussed in rust-lang/rust#88903 (comment) Fixes rust-lang/rust#88903 Fixes rust-lang#9450
When trying to run cargo fix on TrueNAS Core 13.0 via shell in the .cargo directory, I only get the response: error: could not find "Cargo.toml" in "root/.cargo" or any parent directory. Trying to go deeper in the tree: cd bin Hope someone can help me... |
I tried to update a project with the following dependencies:
(Just having an empty project with those dependencies sufficient to reproduce the issue)
I executed the following commands as outlined here:
I expected to see this happen: Everything succeeds without errors
Instead, this happened: The final check commando fails with error messages
The final error message already gives an indication why this error happens. (As already outlined here and here I feel that this change is nothing that is covered by any edition RFC, as any RFC states that changing the edition should be a crate local change and should not affect any upstream or down stream crates. That's at least in my opinion clearly not the case here. That written I accept that removing this feature from the 2021 edition will likely not happen so let's stop that discussion here 😞 )
To give a more productive indication on how that workflow can be improved:
cargo fix --edition
would just fix the problem and either add the corresponding section toCargo.toml
cargo fix --edition
just setsresolver = "1"
in theCargo.toml" for edition upgrades and only for new projects the
resolver = "2"feature is used (and that it is set explicitly in the
Cargo.toml` rather than just changing the default)cargo fix --edition
should emit the same warning ascargo check
already does. As the messaging currently reads it's unclear if the build will fail afterwards or not. Onlycargo check
gives the user a clear indication how to actually fix that problemMeta
rustc --version --verbose
:Backtrace
@rustbot label +A-edition-2021
The text was updated successfully, but these errors were encountered: