Make compiletest do exact matching on triples #49158

varkor · 2018-03-19T01:38:17Z

This avoids the issues of the previous substring matching, ensuring ARCH_TABLE and OS_TABLE will no longer contain redundant entries. Fixes #48893.

r? @rkruppe

…ang#47737" This reverts commit 16ac85c.

sanxiyn · 2018-03-19T15:16:08Z

I don't think this PR is correct, because we already rely on substring matching. For example, with this change, arm-linux-androideabi will not match ignore-android.

varkor · 2018-03-19T15:20:33Z

@sanxiyn: Ah, that's awkward — do you know how prevalent this is (that is: using substring matching)? Would it be plausible to special case strings like android instead?

sanxiyn · 2018-03-19T15:27:54Z

I think you should review the output of rustc --print target-list and add missing entries to ARCH_TABLE and OS_TABLE. It should be doable although tedious.

hanna-kruppe · 2018-03-19T15:37:38Z

Putting all the minor variations of the triple into compiletest probably also makes adding new targets (incl. minor variations of existing targets) more annoying, but hopefully less so than learning months later your ignore-* lines don't do what they appear to be doing.

varkor · 2018-03-19T18:04:12Z

To clarify: you think the tests should be all updated too, so that:
// ignore-android becomes:

// ignore-android
// ignore-androideabi

I.e. no more substring matching whatsoever.

hanna-kruppe · 2018-03-19T18:14:13Z

No, that's not what I'm saying. Triples contain more information (and in some cases, random noise) than we need for the purpose of "ignore this test on this architecture". For example, consider how i386, i586 and i686 triples are all considered for ignore-x86 currently, or how arm64 in triples is treated as an alias for aarch64. I'm in favor of extending that canonicalization to other variations in arch and OS components. This still requires someone to go through all the triples, but only in compiletest, not in every test ever (where it would add no useful information).

This is a judgement call to some degree, there may be distinctions in triples that we may want to preserve. But we'll learn about those when someone wants to make that distinction in a test.

varkor · 2018-03-19T19:09:31Z

Okay, I've added the OSes and architectures that weren't previously considered, and added what I think are reasonable canonicalisations for each of them — they should match the previous substring matching unless something very specific was being done before (which is probably most easily observed by letting bors test it).

hanna-kruppe · 2018-03-19T19:32:19Z

src/tools/compiletest/src/util.rs

    ("bitrig", "bitrig"),
    ("cloudabi", "cloudabi"),
    ("darwin", "macos"),
    ("dragonfly", "dragonfly"),
+    ("eabi", "eabi"),
+    ("eabihf", "eabi"),
+    ("elf", "elf"),


Uh these seem rather odd. I know triples can be weird sometimes, are these actual "operating systems" or do they just happen to be in that position in the triple?

Um, I guess a better question is: which triples do these entries come from?

They did seem odd, but I didn't feel certain enough to exclude them. They come fromthumbv6m-none-eabi (and the other thumb* triples) and msp430-none-elf. Do you think I should just exclude these?

Ah so these triples have no "vendor" component, so the second component is the "OS" and the last component is the ABI.

Ah — is it always safe to make this assumption? (I.e. elided component is vendor?)

Edit: It seems 3-component triples are not consistent about which component they leave out, so I'm ignoring these cases specifically now. It shouldn't be a problem, because they were ignored previously too.

bors · 2018-03-19T19:48:33Z

@varkor: 🔑 Insufficient privileges: Not in reviewers

bors · 2018-03-19T19:48:33Z

@varkor: 🔑 Insufficient privileges: not in try users

varkor · 2018-03-19T19:49:43Z

Not sure why bors thought I was trying anything funny... I know my place.

hanna-kruppe · 2018-03-19T20:01:07Z

src/tools/compiletest/src/header.rs

-                name == util::get_pointer_width(&self.target) ||    // pointer width
-                name == self.stage_id.split('-').next().unwrap() || // stage
-                Some(name) == util::get_env(&self.target) ||        // env
+                util::matches_os(&self.target, triple.get(2), name) || // target


I was always worried about "hard-coding" the meaning of triple components like this since they are somewhat flexible. Since it turns out we even have triples in-tree for which this breaks down, let's not do that...

hanna-kruppe · 2018-03-19T20:02:36Z

src/tools/compiletest/src/util.rs

+            return name == "emscripten" || name == "wasm32-bare"
+        }
+        for &(triple_os, os) in OS_TABLE {
+            if target_os == &triple_os {


... instead let's simply look at all triple components (like it previously did, just without the substring search now). Namespace collisions between the different triple components are practically impossible and would cause far worse problems in the ecosystem of "things that consume triples".

Sure, I wasn't sure if the components could ever coïncide — but I shall do that instead.

pietroalbini · 2018-03-19T23:27:59Z

@varkor it was actually a bors bug (servo/homu#150), don't worry :)

sanxiyn · 2018-03-20T05:17:59Z

@bors r+

bors · 2018-03-20T05:17:59Z

📌 Commit dfaecbb has been approved by sanxiyn

hanna-kruppe

I don't want to r- over this now, but I'd really appreciate if you squashed the commits after the revert because right now there's some back and forth (later commits undoing quite a bit of the earlier ones) which only makes sense in the context of this review, not when viewing history.

I also left two nits.

hanna-kruppe · 2018-03-20T09:43:29Z

src/tools/compiletest/src/header.rs

-                name == util::get_pointer_width(&self.target) ||    // pointer width
-                name == self.stage_id.split('-').next().unwrap() || // stage
-                Some(name) == util::get_env(&self.target) ||        // env
+                util::matches_os(&self.target, &triple, name) || // target


Nit: I think now that these functions look at the whole triple again, there's not much to be gained from splitting here and passing the component explicitly (especially for matches_os which needs the whole triple anyway). Perhaps just revert these changes and split in the util::* functions to keep the interface simpler?

hanna-kruppe · 2018-03-20T09:44:46Z

src/tools/compiletest/src/header.rs

+                name == util::get_arch(&triple) ||               // architecture
+                name == util::get_pointer_width(&self.target) ||       // pointer width
+                name == self.stage_id.split('-').next().unwrap() ||    // stage
+                Some(&name) == triple.get(3) ||                        // env


Nit: This is a pure refactoring, right? If so, I'd prefer to leave it out of this PR, for consistency. (Splitting multiple times is sure to have completely neglegible performance impact.)

This avoids the issues of the previous substring matching, ensuring `ARCH_TABLE` and `OS_TABLE` will no longer contain redundant entries.

varkor · 2018-03-20T11:51:04Z

@rkruppe: bors can wait! I've squashed most of the commits down and simplified the changes.

hanna-kruppe · 2018-03-20T11:56:45Z

Thanks!

@bors r+

bors · 2018-03-20T11:56:45Z

📌 Commit 61e1fbc has been approved by rkruppe

Make compiletest do exact matching on triples This avoids the issues of the previous substring matching, ensuring `ARCH_TABLE` and `OS_TABLE` will no longer contain redundant entries. Fixes rust-lang#48893. r? @rkruppe

Rollup of 23 pull requests - Successful merges: #48374, #48596, #48759, #48939, #49029, #49069, #49093, #49109, #49117, #49140, #49158, #49188, #49189, #49209, #49211, #49216, #49225, #49231, #49234, #49242, #49244, #49105, #49038 - Failed merges:

bors · 2018-03-22T20:01:14Z

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

Revert "Remove useless powerpc64 entry from ARCH_TABLE, closes rust-l…

ca476dd

…ang#47737" This reverts commit 16ac85c.

varkor force-pushed the compiletest-triples branch from b7df111 to 2d5bb5c Compare March 19, 2018 11:42

hanna-kruppe reviewed Mar 19, 2018

View reviewed changes

hanna-kruppe suggested changes Mar 19, 2018

View reviewed changes

pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 19, 2018

pietroalbini mentioned this pull request Mar 19, 2018

Permissions checks broken on master servo/homu#150

Closed

rust-lang deleted a comment from bors Mar 20, 2018

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 Mar 20, 2018

hanna-kruppe suggested changes Mar 20, 2018

View reviewed changes

varkor added 2 commits March 20, 2018 11:23

Add more canonicalisations for OS_TABLE and ARCH_TABLE

eae6d51

Make compiletest do exact matching on triples

61e1fbc

This avoids the issues of the previous substring matching, ensuring `ARCH_TABLE` and `OS_TABLE` will no longer contain redundant entries.

varkor force-pushed the compiletest-triples branch from dfaecbb to 61e1fbc Compare March 20, 2018 11:49

kennytm mentioned this pull request Mar 21, 2018

Rollup of 20 pull requests #49236

Closed

kennytm mentioned this pull request Mar 22, 2018

Rollup of 23 pull requests #49264

Merged

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 22, 2018

alexcrichton merged commit 61e1fbc into rust-lang:master Mar 22, 2018

varkor deleted the compiletest-triples branch March 22, 2018 21:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make compiletest do exact matching on triples #49158

Make compiletest do exact matching on triples #49158

varkor commented Mar 19, 2018

sanxiyn commented Mar 19, 2018

varkor commented Mar 19, 2018

sanxiyn commented Mar 19, 2018

hanna-kruppe commented Mar 19, 2018

varkor commented Mar 19, 2018

hanna-kruppe commented Mar 19, 2018

varkor commented Mar 19, 2018

hanna-kruppe Mar 19, 2018

hanna-kruppe Mar 19, 2018 •

edited

Loading

varkor Mar 19, 2018

hanna-kruppe Mar 19, 2018

varkor Mar 19, 2018 •

edited

Loading

bors commented Mar 19, 2018

bors commented Mar 19, 2018

varkor commented Mar 19, 2018

hanna-kruppe Mar 19, 2018

hanna-kruppe Mar 19, 2018

varkor Mar 19, 2018

pietroalbini commented Mar 19, 2018 •

edited

Loading

sanxiyn commented Mar 20, 2018

bors commented Mar 20, 2018

hanna-kruppe left a comment

hanna-kruppe Mar 20, 2018

hanna-kruppe Mar 20, 2018

varkor commented Mar 20, 2018

hanna-kruppe commented Mar 20, 2018

bors commented Mar 20, 2018

bors commented Mar 22, 2018

Make compiletest do exact matching on triples #49158

Make compiletest do exact matching on triples #49158

Conversation

varkor commented Mar 19, 2018

sanxiyn commented Mar 19, 2018

varkor commented Mar 19, 2018

sanxiyn commented Mar 19, 2018

hanna-kruppe commented Mar 19, 2018

varkor commented Mar 19, 2018

hanna-kruppe commented Mar 19, 2018

varkor commented Mar 19, 2018

Choose a reason for hiding this comment

hanna-kruppe Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varkor Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

bors commented Mar 19, 2018

bors commented Mar 19, 2018

varkor commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietroalbini commented Mar 19, 2018 • edited Loading

sanxiyn commented Mar 20, 2018

bors commented Mar 20, 2018

hanna-kruppe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varkor commented Mar 20, 2018

hanna-kruppe commented Mar 20, 2018

bors commented Mar 20, 2018

bors commented Mar 22, 2018

hanna-kruppe Mar 19, 2018 •

edited

Loading

varkor Mar 19, 2018 •

edited

Loading

pietroalbini commented Mar 19, 2018 •

edited

Loading