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

Move coverage tests from run-make-fulldeps to run-make #85007

Closed

Conversation

richkadel
Copy link
Contributor

Fixes: #83830

The first commit was migrated from another PR that failed because CI had errors likely resulting from trying to run the coverage tests in run-make. (See: #84797 (comment))

So moving the tests should be done as it's own separate PR.

To attempt to resolve those CI errors, this PR also updates bootstrap to add LLVM library link path to run-make.

When moving coverage tests from run-make-fulldeps to run-make,
some targets failed in CI with an obscure message:

failed to execute command: "musl-g++" "-ffunction-sections"
"-fdata-sections" "-fPIC" "-m32" "-march=i686" "-Wl,-melf_i386"
"-static" "-Wa,-mrelax-relocations=no" "-print-file-name=libstdc++.a"

error: No such file or directory (os error 2)

The coverage tests include # needs-profiler-support and these are the
first run-make tests to require it (as far as I can tell).

There is a special case in bootstrap for adding the LLVM library link
path, and it applies to run-make-fulldeps. This commit adds it for
run-make as well.

r? @tmandry
cc: @wesleywiser @cuviper (regarding the change in bootstrap/tests.rs

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 6, 2021
@richkadel richkadel force-pushed the coverage-tests-to-run-make branch from cc894d1 to 8939a19 Compare May 6, 2021 20:31
@Mark-Simulacrum
Copy link
Member

I forget - maybe we've already had this conversation - but while this doesn't seem like a bad step, it may also make sense to move the coverage tests into compiletest itself (similar to how we have mir-opt tests, in theory). I'm not quite sure what is too unique about them that they need to be driven by Make, but my guess is writing the driver in Rust is likely going to help avoid some of the problems that've cropped up over time, and will help avoid tying coverage tests to any 'related' dependencies (e.g., run-make tests depend on rustdoc, but coverage tests likely don't need rustdoc).

@richkadel
Copy link
Contributor Author

Making a coverage-specific compiletest has been discussed a couple of times. It's a good idea, but there just hasn't been time (especially for me to get ramped up on how to write a compiletest, let alone migrate all of the logic from the Makefiles, which is non-trivial).

I don't think I've ever filed an Issue, though, so I'll do that at least.

@cuviper
Copy link
Member

cuviper commented May 6, 2021

The first commit was migrated from another PR that failed because CI had errors likely resulting from trying to run the coverage tests in run-make. (See: #84797 (comment))

I don't understand why that would need the LLVM library path.

@richkadel
Copy link
Contributor Author

Filed #85009

@richkadel
Copy link
Contributor Author

richkadel commented May 6, 2021

I don't understand why that would need the LLVM library path.

To be honest, I don't know if it will. No one so far has been able to explain why the MUSL build target fails in CI (very early in the build) with:

failed to execute command: "musl-g++" "-ffunction-sections"
"-fdata-sections" "-fPIC" "-m32" "-march=i686" "-Wl,-melf_i386"
"-static" "-Wa,-mrelax-relocations=no" "-print-file-name=libstdc++.a"

error: No such file or directory (os error 2)

I don't know anything about the MUSL target, but I think libstdc++.a may be in the LLVM library path, and the coverage tests did not fail under run-make-fulldeps but they do fail under run-make; and I can't find any other substantial differences.

@richkadel
Copy link
Contributor Author

Also, it's not 100% certain that the error: error: No such file or directory (os error 2) refers to libstdc++.a ... just a guess.

@bors
Copy link
Contributor

bors commented May 7, 2021

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

@richkadel richkadel force-pushed the coverage-tests-to-run-make branch from 8939a19 to 9e41596 Compare May 7, 2021 19:53
@rust-log-analyzer

This comment has been minimized.

@richkadel richkadel force-pushed the coverage-tests-to-run-make branch from 9e41596 to baa07f7 Compare May 7, 2021 20:04
@rust-log-analyzer

This comment has been minimized.

@richkadel richkadel force-pushed the coverage-tests-to-run-make branch from baa07f7 to 25692e8 Compare May 7, 2021 20:13
@rust-log-analyzer

This comment has been minimized.

@richkadel richkadel force-pushed the coverage-tests-to-run-make branch from 25692e8 to 013aa9a Compare May 7, 2021 20:18
@rust-log-analyzer

This comment has been minimized.

Fixes: rust-lang#83830

The first commit was migrated from another PR that failed because CI had
errors likely resulting from trying to run the coverage tests in
run-make. (See: rust-lang#84797 (comment))

So moving the tests should be done as it's own separate PR.

To attempt to resolve those CI errors, this PR also updates bootstrap to
add LLVM library link path to run-make.

When moving coverage tests from run-make-fulldeps to run-make,
some targets failed in CI with an obscure message:

failed to execute command: "musl-g++" "-ffunction-sections"
"-fdata-sections" "-fPIC" "-m32" "-march=i686" "-Wl,-melf_i386"
"-static" "-Wa,-mrelax-relocations=no" "-print-file-name=libstdc++.a"

error: No such file or directory (os error 2)

The coverage tests include # needs-profiler-support and these are the
first run-make tests to require it (as far as I can tell).

There is a special case in bootstrap for adding the LLVM library link
path, and it applies to run-make-fulldeps. This commit adds it for
run-make as well.
When moving `coverage` tests from `run-make-fulldeps` to `run-make`,
some targets failed in CI with an obscure message:

failed to execute command: "musl-g++" "-ffunction-sections"
"-fdata-sections" "-fPIC" "-m32" "-march=i686" "-Wl,-melf_i386"
"-static" "-Wa,-mrelax-relocations=no" "-print-file-name=libstdc++.a"

error: No such file or directory (os error 2)

The coverage tests include `# needs-profiler-support` and these are the
first `run-make` tests to require it (as far as I can tell).

There is a special case in `bootstrap` for adding the LLVM library link
path, and it applies to `run-make-fulldeps`. This commit adds it for
`run-make` as well.
@richkadel richkadel force-pushed the coverage-tests-to-run-make branch from 013aa9a to 4881fc7 Compare May 7, 2021 20:32
@tmandry
Copy link
Member

tmandry commented May 7, 2021

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented May 7, 2021

📌 Commit 4881fc7 has been approved by tmandry

@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 May 7, 2021
@bors
Copy link
Contributor

bors commented May 8, 2021

⌛ Testing commit 4881fc7 with merge 8f0f5468b2b5cb2a26d03fbd3e99dd47d4568e6a...

@rust-log-analyzer
Copy link
Collaborator

The job dist-i586-gnu-i586-i686-musl failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
DirectMap1G:    53477376 kB
    Finished dev [unoptimized + debuginfo] target(s) in 0.18s


failed to execute command: "musl-g++" "-ffunction-sections" "-fdata-sections" "-fPIC" "-m32" "-march=i686" "-Wl,-melf_i386" "-static" "-Wa,-mrelax-relocations=no" "-print-file-name=libstdc++.a"
error: No such file or directory (os error 2)

failed to run: /checkout/obj/build/bootstrap/debug/bootstrap --stage 2 test --host= --target i586-unknown-linux-gnu,i686-unknown-linux-musl
Build completed unsuccessfully in 0:00:00

@bors
Copy link
Contributor

bors commented May 8, 2021

💔 Test failed - checks-actions

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 8, 2021
@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 8, 2021
@richkadel
Copy link
Contributor Author

Well, I did try to test this using the target Docker image and run.sh, and some networking limitations prevented me from seeing the entire suite succeed, but it seemed to build way past the error shown here.

I don't know what else to try.

@ghost
Copy link

ghost commented May 8, 2021

Well, I did try to test this using the target Docker image and run.sh, and some networking limitations prevented me from seeing the entire suite succeed, but it seemed to build way past the error shown here.

I'm able to reproduce this with DEPLOY=1, but I'm also not able to reproduce this without that environment variable.

@ghost
Copy link

ghost commented May 8, 2021

Also, it's not 100% certain that the error: error: No such file or directory (os error 2) refers to libstdc++.a ... just a guess.

I guess it refers to musl-g++, because that builder builds LLVM only for x86_64-unknown-linux-gnu (there's no other "Building LLVM" message in the log I linked), not for musl, so musl-g++ could be absent.

@richkadel
Copy link
Contributor Author

Thanks for the insights!

I did see a different error message (different from CI) in my Docker repro that seems to confirm musl-g++ is missing.

So I don't know if run-make is building for the wrong target (the platform has musl in its name), or if that platform never ran run-make-fulldeps (because of my network constraints, the test didn't get that far for me), or if run-make-fulldeps is configured differently, and building musl-g++ (if so, I need to apply that configuration different to run-make as well).

@hyd-dev Since you are able to reproduce, did all tests for dist-i586-gnu-i586-i686-musl finish successfully (without this PR)?

And if so, does it run run-make-fulldeps (and in particular run-make-fulldeps/coverage-* tests)?

@ghost
Copy link

ghost commented May 8, 2021

without this PR

I guess you could just check the log of another (merged) PR: https://github.com/rust-lang-ci/rust/runs/2534368555 (looks like run-make-fulldeps was not run on CI).
I could also try locally, but I guess that would take some time (I didn't try to run it without this PR).

@richkadel
Copy link
Contributor Author

You're right. Thank you. No need to check locally. That explains the difference at least.

@richkadel
Copy link
Contributor Author

@jyn514 @petrochenkov - IIRC both of you recommended moving coverage tests from run-make-fulldeps to run-make.

I'm considering abandoning this PR and leaving the coverage tests in run-make-fulldeps since run-make may introduce multiple incompatibilities.

Please let me know if you have a better recommendation.

As recently revealed (see prior comment), apparently run-make runs on more targets than run-make-fulldeps, so the recent CI failures with this PR are because the coverage tests are attempted on platforms they've never targeted before.

I could try # ignore-musl but there's no guarantee that MUSL is the only platform that would fail.

I don't know if it's worth trying to identify ALL of the run-make targets that don't already run run-make-fulldeps, but I don't like the idea of trying and failing CI many times, and then generating a [set of directives that looks like this (7 ignores)](https://github.com/richkadel/rust/blob/1fa48cf181f1fe7e1aba133be199804c652fe55c/src/test/run-make/issue-36710/Makefile.

I don't see a clear, well understood path to porting tests from run-make-fulldeps to run-make.

Thanks in advance for your thoughts and recommendations!

@@ -1399,7 +1399,7 @@ note: if you're sure you want to do this, please open an issue as to why. In the
// requirement, but the `-L` library path is not propagated across
// separate compilations. We can add LLVM's library path to the
// platform-specific environment variable as a workaround.
if !builder.config.dry_run && suite.ends_with("fulldeps") {
if !builder.config.dry_run && (suite == "ui-fulldeps" || mode == "run-make") {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should be removed. We now know this did not fix the CI issues in the MUSL target.

@jyn514
Copy link
Member

jyn514 commented May 9, 2021

I don't understand run-make very well at all (I've also had trouble in #83775) and also I don't have time to review this.

@bors
Copy link
Contributor

bors commented May 12, 2021

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

@richkadel
Copy link
Contributor Author

No known way to resolve this, so I'm closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move coverage tests from run-make-fulldeps to run-make
8 participants