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

Cargo caused complete breakage of 4 pipelines #96349

Closed
Yuri6037 opened this issue Apr 23, 2022 · 8 comments
Closed

Cargo caused complete breakage of 4 pipelines #96349

Yuri6037 opened this issue Apr 23, 2022 · 8 comments
Labels
requires-nightly This issue requires a nightly compiler in some way. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue.

Comments

@Yuri6037
Copy link

A week ago Cargo dropped allowed_fail field in the test JSON output: 6562069.

This breaking change was never announced and caused 4 pipelines on GitLab to fail due the now missing field.

In the future I think rust should announce such breaking changes before committing the changes.

@Yuri6037 Yuri6037 changed the title Cargo caused complete breaking of 4 pipelines Cargo caused complete breakage of 4 pipelines Apr 23, 2022
@joshtriplett
Copy link
Member

allow_fail was a nightly-only feature. However, it looks like stable Rust emitted allow_fail: false on tests, rather than omitting the allow_fail attribute. In general, we shouldn't let nightly-only features affect stable output when not used, for exactly this reason.

@Yuri6037 To confirm, your pipelines were depending on the presence of allow_fail: false in the output?

@joshtriplett joshtriplett added T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. labels Apr 23, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 23, 2022
@Yuri6037
Copy link
Author

Yuri6037 commented Apr 23, 2022

allow_fail was a nightly-only feature. However, it looks like stable Rust emitted allow_fail: false on tests, rather than omitting the allow_fail attribute. In general, we shouldn't let nightly-only features affect stable output when not used, for exactly this reason.

@Yuri6037 To confirm, your pipelines were depending on the presence of allow_fail: false in the output?

Thanks, yes indeed the pipelines depends on a tool which did require the presence of allow_fail for serde decoding. When this field was dropped the tool started to emit errors in all pipelines using it.

EDIT: I'm right now preparing an update of the parsing tool to fix this. It would just be nice if there could be announcements when these changes occur to avoid this because fixing this is not as simple as just updating on git repo I have to actually re-run pipelines in all failing projects.

@joshtriplett
Copy link
Member

joshtriplett commented Apr 23, 2022

@Yuri6037 Ideally we shouldn't do breaking changes at all, with or without announcements. (We coordinate and make breaking changes on rare occasions to deal with safety issues, but that was not the case here.) #93416 was not known to be a breaking change when it was made; it was assumed to be the removal of a nightly feature, which wouldn't be subject to any stability guarantee. I would guess that nobody caught that allow_fail: false was being emitted as part of stable output.

@ehuss
Copy link
Contributor

ehuss commented Apr 23, 2022

I would guess that nobody caught that allow_fail: false was being emitted as part of stable output.

--format json is also unstable, so I don't think this is a stable change.

@nagisa nagisa added the requires-nightly This issue requires a nightly compiler in some way. label Apr 23, 2022
@nagisa
Copy link
Member

nagisa commented Apr 23, 2022

I tagged this requires-nightly given that everything so far points at this requiring a nightly. We can remove the tag if the issue author can confirm that they were seeing this with a stable compiler and without RUSTC_BOOTSTRAP=1-like workarounds and detail how.

@Yuri6037
Copy link
Author

The actual program being tested builds in stable rust. However the test command uses unstable features to access the JSON output formatting.

@joshtriplett joshtriplett removed regression-from-stable-to-stable Performance or correctness regression from one stable version to another. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Apr 24, 2022
@joshtriplett
Copy link
Member

@ehuss Thanks for the clarification!

@Enselic
Copy link
Member

Enselic commented Mar 16, 2024

Triage: There is no further action to take here then, right?

Closing, but feel free to reopen if I am mistaken.

@Enselic Enselic closed this as not planned Won't fix, can't repro, duplicate, stale Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires-nightly This issue requires a nightly compiler in some way. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants