-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Add new --bypass-ignore-backends
option
#147633
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
base: master
Are you sure you want to change the base?
Add new --bypass-ignore-backends
option
#147633
Conversation
Some changes occurred in src/tools/compiletest cc @jieyouxu This PR modifies If appropriate, please update |
To provide more context here, I suggested this to allow gradual removal of That being said, now that I think about it, I wonder how would the workflow look like. If you use the option that this PR adds, it would just run all tests; some of them would fail, some of them would succeed. But it wouldn't be trivial to figure out which one of those that succeeded actually contain I guess what I really wanted was something like @jieyouxu Let me know if you have any opinions on this. |
Here are the 3 use cases we currently have in the CI of cg_gcc I can think of:
We already have the logic to do this in the CI of cg_gcc, but we need a flag like |
Maybe we could do a general override mechanism, something like |
I guess that makes sense if there are other |
I'm hesitant about generalizing this to be a general ignore-override mechanism, for a few reasons:
"This test fails only on $ignored_conditions and I don't know why" is the one such an "ignore-override" would generally be designed for, but it feels a bit too generic. Another use case might be "this conditional directive is unneeded", but that's hard to assess because it might not fail in CI, but it might fail locally for non-CI-tested configurations without said directive. |
if builder.config.cmd.bypass_ignore_backends() { | ||
cmd.arg("--bypass-ignore-backends"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: if we do go with this, can you please document this alongside //@ ignore-backends
in rustc-dev-guide?
"CODEGEN BACKEND [NAME | PATH]", | ||
); | ||
) | ||
.optflag("", "bypass-ignore-backends", "ignore `//@ ignore-backends` statements"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: s/statements/directives/.
/// Whether to ignore `//@ ignore-backends`. | ||
pub bypass_ignore_backends: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: probably "bypass", ignoring ignore sounds a bit confusing at first glance.
Given that
This seems okay, though please do document this flag and its intended purpose in rustc-dev-guide, or else no one else will have clues on what it's supposed to do. |
Fixes #147063.
It adds a new option to
bootstrap
to allow to ignore//@ ignore-backends
statements in tests.cc @jieyouxu @antoyo
r? @Kobzol