Use `# min-llvm-version: 11.0` to force a minimum LLVM version #81688

pnkfelix · 2021-02-03T02:55:03Z

Use # min-llvm-version: 11.0 to force a minimum LLVM version, rather than ad-hoc internal solution.

In particular: the specific code to define LLVM_VERSION_11_PLUS here was, for some reason, using $(shell ...) with bash-specific variable replacement code. On non-bash platforms like dash, that shell invocation would fail, and the
LLVM_VERSION_11_PLUS check would always fail, the test would always be ignored, and thus be treated as a "success" (in the sense that --bless would never do anything).

Note in particular that GNU Make treats the SHELL variable as a very special case: it does not inherit the value of SHELL from the user's environment. Except on Windows. See more explanation in the GNU Make docs.
The effect of this is that these tests end up using /bin/sh (except on Windows) for their $(shell ...) invocations, and thus we see differing behaviors depending on whether your /bin/sh links to /bin/dash or to /bin/bash.

This was causing me a lot of pain.

…r than ad-hoc internal solution. In particular: the specific code to define LLVM_VERSION_11_PLUS here was, for some reason, using `$(shell ...)` with bash-specific variable replacement code. On non-bash platforms like dash, that `shell` invocation would fail, and the LLVM_VERSION_11_PLUS check would always fail, the test would always be ignored, and thus be treated as a "success" (in the sense that `--bless` would never do anything). This was causing me a lot of pain.

rust-highfive · 2021-02-03T02:55:06Z

r? @Mark-Simulacrum

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

Mark-Simulacrum

r=me

Mark-Simulacrum · 2021-02-03T03:01:06Z

src/test/run-make-fulldeps/coverage/coverage_tools.mk

@@ -14,10 +14,3 @@
 # Therefore, `-C link-dead-code` is no longer automatically enabled.

 UNAME = $(shell uname)
-
-# Rust option `-Z instrument-coverage` uses LLVM Coverage Mapping Format version 4,


Presumably this file is not adding much value anymore and we should move the uname to tools.mk?

Yeah but lets maybe leave that for a later PR. Or at least, this may need to be backported when I add the other beta backports that depend on it (maybe)?

Hmm on second thought I guess the backport of this PR isn't actually necessary since all it will mean is that the tests on beta will continue to be incorrectly ignored

pnkfelix · 2021-02-03T05:51:37Z

@bors r+

(I was tempted to up the priority on this, since it is blocking other work I am doing, but I can just cherry-pick it on those PR's and run --bless with the cherry-pick, so I'll let it go into the queue normally)

bors · 2021-02-03T05:51:38Z

📌 Commit 03c000e has been approved by pnkfelix

pnkfelix · 2021-02-03T05:51:56Z

@bors r=simulacrum

bors · 2021-02-03T05:51:57Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Update test-various to Ubuntu 20.04 #81535

bors · 2021-02-03T05:51:58Z

📌 Commit 03c000e has been approved by simulacrum

pnkfelix · 2021-02-03T05:53:40Z

(Namely, I needed this change to be able to bless output for PR #81257 and PR #81294 on my machine that is using /bin/dash instead of /bin/bash as its /bin/sh implementation.)

…run-make-tests, r=simulacrum Use `# min-llvm-version: 11.0` to force a minimum LLVM version Use `# min-llvm-version: 11.0` to force a minimum LLVM version, rather than ad-hoc internal solution. In particular: the specific code to define LLVM_VERSION_11_PLUS here was, for some reason, using `$(shell ...)` with bash-specific variable replacement code. On non-bash platforms like dash, that `shell` invocation would fail, and the LLVM_VERSION_11_PLUS check would always fail, the test would always be ignored, and thus be treated as a "success" (in the sense that `--bless` would never do anything). This was causing me a lot of pain.

m-ou-se · 2021-02-03T16:12:36Z

@bors r-

This failed in #81707: https://github.com/rust-lang-ci/rust/runs/1823202705

pnkfelix · 2021-02-03T18:29:32Z

Right: The problem here is that this has always been broken on CI, and my change here is fixing things so that we now actually observe the brokenness.

One quick fix could be to try to explicitly set the SHELL to /bin/bash in the coverage-reports Makefile.

But personally I would prefer to just outright disable the coverage-reports test in this PR, and leave fixing it's Makefile and re-enabling the test to a follow-up PR.

Mark-Simulacrum · 2021-02-03T19:29:57Z

I think that's the right step; it's an unstable feature and we should not go out of our way to get its tests working.

cc @tmandry @richkadel

(Test was silently ignored on Linux CI prior to parent commit that switched to using `# min-llvm-version`. But the switch made the ignoring stop, exposing other brokenness in the form of bash-dependent syntax in the `$(shell ...)` invocations.)

pnkfelix · 2021-02-03T21:03:36Z

@bors r=simulacrum rollup=never

bors · 2021-02-03T21:03:37Z

📌 Commit 5ce6710 has been approved by simulacrum

bors · 2021-02-03T23:36:56Z

⌛ Testing commit 5ce6710 with merge 89164fe516d4c71fb849a3559e61f1bdbb249a23...

bors · 2021-02-04T00:38:06Z

💔 Test failed - checks-actions

rust-log-analyzer · 2021-02-04T00:38:06Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

wesleywiser · 2021-02-04T16:11:55Z

It looks to me like this might just need a retry? aarch64-gnu failed in the "run the build" step but I can't get any of the output, even though I can for previous steps.

@bors retry

m-ou-se · 2021-02-04T17:03:21Z

Yeah, the aarch64-gnu runner was having some problems a bit earlier. It seems to be working properly again now.

bors · 2021-02-05T06:12:31Z

⌛ Testing commit 5ce6710 with merge 9e5d58f...

bors · 2021-02-05T08:53:25Z

☀️ Test successful - checks-actions
Approved by: simulacrum
Pushing 9e5d58f to master...

Ensures `make` tests run under /bin/dash (if available), like CI, and fixes a Makefile Note: This cherrypicks rust-lang#81688 (`@pnkfelix)` Updates `tools.mk` to explicitly require `SHELL := /bin/dash`, since CI uses `dash` but other environments (including developer local machines) may default to `bash`. Replaces bash-specific shell command in one Makefile with a dash-compatible alternative, and re-enables the affected Makefile test. Removes apparently redundant definition of `UNAME`. Also see: [zulip discussion thread](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/how.20to.20run.2Fbless.20src.2Ftest.2Frun-make-fulldeps.2Fcoverage.20.3F) r? `@pnkfelix` FYI: `@wesleywiser` `@tmandry`

Ensures `make` tests run under /bin/dash (if available), like CI, and fixes a Makefile Note: This cherrypicks rust-lang#81688 (``@pnkfelix)`` Updates `tools.mk` to explicitly require `SHELL := /bin/dash`, since CI uses `dash` but other environments (including developer local machines) may default to `bash`. Replaces bash-specific shell command in one Makefile with a dash-compatible alternative, and re-enables the affected Makefile test. Removes apparently redundant definition of `UNAME`. Also see: [zulip discussion thread](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/how.20to.20run.2Fbless.20src.2Ftest.2Frun-make-fulldeps.2Fcoverage.20.3F) r? ``@pnkfelix`` FYI: ``@wesleywiser`` ``@tmandry``

Ensures `make` tests run under /bin/dash (if available), like CI, and fixes a Makefile Note: This cherrypicks rust-lang#81688 (`@pnkfelix)` Updates `tools.mk` to explicitly require `SHELL := /bin/dash`, since CI uses `dash` but other environments (including developer local machines) may default to `bash`. Replaces bash-specific shell command in one Makefile with a dash-compatible alternative, and re-enables the affected Makefile test. Removes apparently redundant definition of `UNAME`. Also see: [zulip discussion thread](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/how.20to.20run.2Fbless.20src.2Ftest.2Frun-make-fulldeps.2Fcoverage.20.3F) r? `@pnkfelix` FYI: `@wesleywiser` `@tmandry`

rust-highfive assigned Mark-Simulacrum Feb 3, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 3, 2021

pnkfelix added A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 3, 2021

Mark-Simulacrum reviewed Feb 3, 2021

View reviewed changes

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Feb 3, 2021

bless the coverage-spanview output. Cc rust-lang#81688.

1783c47

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 Feb 3, 2021

m-ou-se mentioned this pull request Feb 3, 2021

Rollup of 5 pull requests #81707

Closed

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 Feb 3, 2021

Ignore broken test.

5ce6710

(Test was silently ignored on Linux CI prior to parent commit that switched to using `# min-llvm-version`. But the switch made the ignoring stop, exposing other brokenness in the form of bash-dependent syntax in the `$(shell ...)` invocations.)

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 3, 2021

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 4, 2021

richkadel mentioned this pull request Feb 4, 2021

Ensures make tests run under /bin/dash (if available), like CI, and fixes a Makefile #81734

Merged

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 Feb 4, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 5, 2021

bors merged commit 9e5d58f into rust-lang:master Feb 5, 2021

rustbot added this to the 1.51.0 milestone Feb 5, 2021

richkadel mentioned this pull request Feb 11, 2021

Improve coverage results on certain derived traits that show two different counters #79626

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `# min-llvm-version: 11.0` to force a minimum LLVM version #81688

Use `# min-llvm-version: 11.0` to force a minimum LLVM version #81688

pnkfelix commented Feb 3, 2021 •

edited

Loading

rust-highfive commented Feb 3, 2021

Mark-Simulacrum left a comment

Mark-Simulacrum Feb 3, 2021

pnkfelix Feb 3, 2021

pnkfelix Feb 3, 2021 •

edited

Loading

pnkfelix commented Feb 3, 2021

bors commented Feb 3, 2021

pnkfelix commented Feb 3, 2021

bors commented Feb 3, 2021

bors commented Feb 3, 2021

pnkfelix commented Feb 3, 2021

m-ou-se commented Feb 3, 2021

pnkfelix commented Feb 3, 2021 •

edited

Loading

Mark-Simulacrum commented Feb 3, 2021

pnkfelix commented Feb 3, 2021

bors commented Feb 3, 2021

bors commented Feb 3, 2021

bors commented Feb 4, 2021

rust-log-analyzer commented Feb 4, 2021

wesleywiser commented Feb 4, 2021 •

edited

Loading

m-ou-se commented Feb 4, 2021

bors commented Feb 5, 2021

bors commented Feb 5, 2021

Use # min-llvm-version: 11.0 to force a minimum LLVM version #81688

Use # min-llvm-version: 11.0 to force a minimum LLVM version #81688

Conversation

pnkfelix commented Feb 3, 2021 • edited Loading

rust-highfive commented Feb 3, 2021

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Mark-Simulacrum Feb 3, 2021

Choose a reason for hiding this comment

pnkfelix Feb 3, 2021

Choose a reason for hiding this comment

pnkfelix Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

pnkfelix commented Feb 3, 2021

bors commented Feb 3, 2021

pnkfelix commented Feb 3, 2021

bors commented Feb 3, 2021

bors commented Feb 3, 2021

pnkfelix commented Feb 3, 2021

m-ou-se commented Feb 3, 2021

pnkfelix commented Feb 3, 2021 • edited Loading

Mark-Simulacrum commented Feb 3, 2021

pnkfelix commented Feb 3, 2021

bors commented Feb 3, 2021

bors commented Feb 3, 2021

bors commented Feb 4, 2021

rust-log-analyzer commented Feb 4, 2021

wesleywiser commented Feb 4, 2021 • edited Loading

m-ou-se commented Feb 4, 2021

bors commented Feb 5, 2021

bors commented Feb 5, 2021

Use `# min-llvm-version: 11.0` to force a minimum LLVM version #81688

Use `# min-llvm-version: 11.0` to force a minimum LLVM version #81688

pnkfelix commented Feb 3, 2021 •

edited

Loading

pnkfelix Feb 3, 2021 •

edited

Loading

pnkfelix commented Feb 3, 2021 •

edited

Loading

wesleywiser commented Feb 4, 2021 •

edited

Loading