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.

Sign up for GitHub

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

Jump to bottom

Add CI step to prevent new untranslatable diagnostics being added #111218

Closed
wants to merge 1 commit into from
Closed

Add CI step to prevent new untranslatable diagnostics being added #111218

wants to merge 1 commit into from

Conversation

clubby789
Copy link
Contributor

This is in order to prevent new untranslatable diagnostics from being added, as this requires additional work to migrate them later on.

@rustbot
Copy link
Collaborator

rustbot commented May 4, 2023

r? @Mark-Simulacrum

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

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels May 4, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 4, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@clubby789 clubby789 force-pushed the no-new-untranslatable branch from fcc1ba4 to c8a3051 Compare May 4, 2023 20:58
@clubby789
Copy link
Contributor Author

Ideally this would also work with x run/x test but we can't recursively invoke x because of the file locking

@clubby789
Copy link
Contributor Author

Hmm, this doesn't seem to work right now. I'm guessing the first set of warnings hit deny-warnings and stop the build?

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 4, 2023
@clubby789
Copy link
Contributor Author

@rustbot ready

@clubby789 clubby789 force-pushed the no-new-untranslatable branch from c8a3051 to 494c353 Compare May 5, 2023 01:10
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 5, 2023
@clubby789 clubby789 changed the title Add CI step to prevent new untranslatable diagnostics being added to CI Add CI step to prevent new untranslatable diagnostics being added May 5, 2023
@Mark-Simulacrum
Copy link
Member

I'm not a fan of this approach. I think it would be better to start with adding allow's to all crates/modules that have this problem and then gradually remove those when we're ready with that crate.

r? @davidtwco

@rustbot rustbot assigned davidtwco and unassigned Mark-Simulacrum May 6, 2023
@bors
Copy link
Contributor

bors commented May 9, 2023

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

@clubby789
Copy link
Contributor Author

I'm a bit worried that moving to allows will make it harder to differentiate between "Hasn't been looked at yet" and "Too complex to migrate right now"/"Cannot be migrate". A comment on each allow might help, but right now there's around 900 remaining diagnostics across around 100 files

@davidtwco
Copy link
Member

I'm a bit worried that moving to allows will make it harder to differentiate between "Hasn't been looked at yet" and "Too complex to migrate right now"/"Cannot be migrate". A comment on each allow might help, but right now there's around 900 remaining diagnostics across around 100 files

Apologies for the delay in replying here, but I think I agree with @Mark-Simulacrum that adding allows would be a slightly nicer approach, requiring less new machinery. It's a little bit more up-front work if we wanted to add comments to everything, but we could start with a coarser approach of just adding #[allow] on modules.

@davidtwco davidtwco added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 27, 2023
@JohnCSimon
Copy link
Member

@clubby789
ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks!

@clubby789
Copy link
Contributor Author

Sorry, missed the ping. Closing now in favour of #120693

@clubby789 clubby789 closed this Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@davidtwco davidtwco

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@clubby789 @rustbot @rust-log-analyzer @Mark-Simulacrum @bors @davidtwco @JohnCSimon