-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
compiletest: Support opt-in Clang-based run-make tests and use them for testing xLTO. #57514
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
6491bc8
to
8fc4d91
Compare
|
||
if let Some(var) = env::var_os("RUSTBUILD_FORCE_CLANG_BASED_TESTS") { | ||
match &var.to_string_lossy()[..] { | ||
"1" | "yes" | "on" => { |
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.
Heh since this is an internal env var I think matching on only "yes" or "1" is probably fine, we tend to not really need that much error handling and support for internal features like this
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.
I wanted to try and make it less likely to have a typo causing things to be silently ignored. What would be really needed though would be fuzzy matching on env var names...
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.
True yeah, although we could also just check for existence here instead of checking for value perhaps?
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.
That's what wanted to do at first but then I remembered the CARGO_INCREMENTAL=0
confusion. Since it's not really complicated to support a few different options here, I just did that.
fc95038
to
4ad6205
Compare
I've simplified how Clang-based tests are set up. There's just a Which builders should we enable the tests on? |
Currently I believe we only enable LLDB on the dist builders for OSX, but we'll probably want to pick a Linux builder which isn't taking too long today and run it there. I don't think it particularly matters which one we choose! |
Oh no, one |
src/ci/docker/x86_64-gnu/Dockerfile
Outdated
@@ -16,9 +16,13 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ | |||
COPY scripts/sccache.sh /scripts/ | |||
RUN sh /scripts/sccache.sh | |||
|
|||
ENV RUSTBUILD_FORCE_CLANG_BASED_TESTS yes |
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.
Checking a recent test run this builder is already running too long at 2h11m, perhaps we can pick a faster builder like x86_64-gnu-debug to run these tests in?
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.
But that doesn't seem to run tests?
Was that when testing locally? Or when testing this on CI? (the Travis run for this PR doesn't include the builder activated here) |
Some cross-language run-make tests need a Clang compiler that matches the LLVM version of rustc. Since such a compiler usually isn't available these tests (marked with the "needs-matching-clang" directive) are ignored by default. For some CI jobs we do need these tests to run unconditionally though. In order to support this a --force-clang-based-tests flag is added to compiletest. If this flag is specified, compiletest will fail if it can't detect an appropriate version of Clang.
Oh right, the pre-check is using the LLVM-6.0 image. |
f4ef08a
to
6e0ffb3
Compare
I switched to the Is that what you meant, @alexcrichton? |
Ah true that wasn't running any tests... That looks fine, I really don't have an opinion about where this should go other than that this is quite a large ask for CI to build all of clang and run more tests. We need to be careful to not regress cycle time which is already really really bad. If possible can this run through |
aec74bf
to
821e715
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Huh, don't know what that is about. Cmake can't find python? |
Oh the containers all have varying amounts of software on them, you may need to update one aspect here or there to install more things like python. Also FWIW PR builds won't be great for timing because they don't have access to sccache credentials which means they can't actually write any contents to the cache, only pull them |
821e715
to
ca6053e
Compare
@bors try |
compiletest: Support opt-in Clang-based run-make tests and use them for testing xLTO. Some cross-language run-make tests need a Clang compiler that matches the LLVM version of `rustc`. Since such a compiler usually isn't available these tests (marked with the `needs-matching-clang` directive) are ignored by default. For some CI jobs we do need these tests to run unconditionally though. In order to support this a `--force-clang-based-tests` flag is added to compiletest. If this flag is specified, `compiletest` will fail if it can't detect an appropriate version of Clang. @rust-lang/infra The PR doesn't yet enable the tests yet. Do you have any recommendation for which jobs to enable them? cc #57438 r? @alexcrichton
So LLD doesn't seem to make a huge difference but it's still nice to have it available. |
☀️ Test successful - checks-travis |
Ok! Want to back out the changes to .travis.yml? Otherwise r=me |
ccbab3f
to
b17c10d
Compare
Ah, good catch! Done. @bors r=alexcrichton |
📌 Commit b17c10d has been approved by |
⌛ Testing commit b17c10d with merge dce0744fc43e1ce55ab841a40940b5afbe030909... |
💔 Test failed - status-appveyor |
@bors retry |
compiletest: Support opt-in Clang-based run-make tests and use them for testing xLTO. Some cross-language run-make tests need a Clang compiler that matches the LLVM version of `rustc`. Since such a compiler usually isn't available these tests (marked with the `needs-matching-clang` directive) are ignored by default. For some CI jobs we do need these tests to run unconditionally though. In order to support this a `--force-clang-based-tests` flag is added to compiletest. If this flag is specified, `compiletest` will fail if it can't detect an appropriate version of Clang. @rust-lang/infra The PR doesn't yet enable the tests yet. Do you have any recommendation for which jobs to enable them? cc #57438 r? @alexcrichton
☀️ Test successful - checks-travis, status-appveyor |
Some cross-language run-make tests need a Clang compiler that matches the LLVM version of
rustc
. Since such a compiler usually isn't available these tests (marked with theneeds-matching-clang
directive) are ignored by default.
For some CI jobs we do need these tests to run unconditionally though. In order to support this a
--force-clang-based-tests
flag is added to compiletest. If this flag is specified,compiletest
will fail if it can't detect an appropriate version of Clang.@rust-lang/infra The PR doesn't yet enable the tests yet. Do you have any recommendation for which jobs to enable them?
cc #57438
r? @alexcrichton