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

Fix tests using only-i686 to use the correct only-x86 directive #90569

Merged
merged 1 commit into from
Nov 6, 2021

Conversation

wesleywiser
Copy link
Member

We translate i686 to x86 which means tests marked as only-i686
never ran. Update those tests to use only-x86.

We parse the only- architecture directive here

pub fn get_arch(triple: &str) -> &'static str {
let triple: Vec<_> = triple.split('-').collect();
for &(triple_arch, arch) in ARCH_TABLE {
if triple.contains(&triple_arch) {
return arch;
}
}
panic!("Cannot determine Architecture from triple");
}

and we translate i686 to x86 here

("i686", "x86"),

We translate `i686` to `x86` which means tests marked as `only-i686`
never ran. Update those tests to use `only-x86`.
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 4, 2021
@Mark-Simulacrum
Copy link
Member

Hm, how do we ensure new tests don't get added? I would have expected the panic! in that code to get triggered... maybe we can fix that as well?

@wesleywiser
Copy link
Member Author

wesleywiser commented Nov 4, 2021

The panic isn't triggered because we do find the arch but in the calling code, it doesn't match the only- directive:

name == util::get_arch(&self.target) || // architecture

ie:

  • we call util::get_arch passing in "i686-pc-windows-msvc"
  • we find i686 in ARCH_TABLE and so we return "x86"
  • but name is i686 so we don't match

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Nov 5, 2021

Right, I guess, what I'm trying to get at is that it seems like we should error on such a test that can never run. I'm not sure I'm fully understanding, but perhaps we should add a panic whenever name is present on the "left hand" of the mapping table?

Alternatively, we could also call the equivalent of get_arch on the name that maps it, but I'd prefer we don't have both only-i686 and only-x86 tests.

@wesleywiser
Copy link
Member Author

Right. I agree, ideally we would be able to detect an invalid only- flag which will cause the test to never run but compiletest isn't really set up to do that right now and it would be a much larger change than just fixing this. I'm also a little hesitant to introduce validation just for architectures as I think developers would naturally expect the other kinds of flags to be validated as well.

We could certainly file an issue to improve compiletest though.

I'm not sure I'm fully understanding, but perhaps we should add a panic whenever name is present on the "left hand" of the mapping table?

We also can't do that because some names are present as both keys and values in the mapping table (arm, aarch64, avx, etc).

@Mark-Simulacrum
Copy link
Member

Alright. r=me with an issue filed.

Did you spot check the other mappings in the table to check we don't have extra tests like this (for non-i686)?

@wesleywiser
Copy link
Member Author

Yes, I didn't see any other issues.

@bors r=Mark-Simulacrum rollup

@bors
Copy link
Contributor

bors commented Nov 5, 2021

📌 Commit 8c56ef0 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 5, 2021
…askrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#89942 (Reorder `widening_impl`s to make the doc clearer)
 - rust-lang#90569 (Fix tests using `only-i686` to use the correct `only-x86` directive)
 - rust-lang#90597 (Warn for variables that are no longer captured)
 - rust-lang#90623 (Remove more checks for LLVM < 12)
 - rust-lang#90626 (Properly register text_direction_codepoint_in_comment lint.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cd24ffb into rust-lang:master Nov 6, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants