-
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
Add serde to the cargotest
test suite
#62587
Conversation
This branch is based before rust-lang#62574, so that rust-lang#62562 can be reproduced. This should succeed when merged into `master`.
(rust_highfive has picked a reviewer for you, use r? to override) |
I did observe a failure reproducing #62562 by running |
FWIW I wouldn't recommend doing this since it's pretty unlikely to prevent the failure that happened. We already build serde as part of normal builds (and serde_derive) since we build Cargo with a bootstrapped rustc. The cargotest test suite suffers from the problem that it needs to be frozen, so it's basically frozen at years-old commits for all the other projects. While we can make sure that version of the crate continues to build we'll never be able to provide the guarantee that the latest version of the complier always compiles the latest version of serde. We don't really lose anything by adding this of course, but we won't be picking up a benefit of guaranteeing this never happens again. |
It’s a good point that testing a frozen commit is less useful than testing the current latest crates.io version. However, how come #62393 managed to land if serde_derive is already built on CI with each PR’s new compiler? |
I can locally update serde_derive from v1.0.81 to v1.0.94, so I'd naively guess that it has to do with recent code added to serde broke and we're not compiling the recent code in serde, but I haven't verified if this is the case. |
Ping from triage @alexcrichton, any update? |
@SimonSapin do you still feel like this should land? (TBH I think we should remove the entire cargotest test suite myself) |
Ok, if removing this suite entirely is in the cards there’s probably no point to this PR. |
This branch is based before #62574, so that #62562 can be reproduced. This should succeed when merged into
master
.