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

add target features when extracting and running doctests #49864

Merged
merged 2 commits into from
Apr 14, 2018

Conversation

QuietMisdreavus
Copy link
Member

When rendering documentation, rustdoc will happily load target features into the cfg environment from the current target, but fails to do this when doing anything with doctests. This would lead to situations where, thanks to #48759, functions tagged with #[target_feature] couldn't run doctests, thanks to the automatic #[doc(cfg(target_feature = "..."))].

Currently, there's no way to pass codegen options to rustdoc that will affect its rustc sessions, but for now this will let you use target features that come default on the platform you're targeting.

Fixes #49723

@rust-highfive
Copy link
Collaborator

r? @steveklabnik

(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 Apr 11, 2018
@QuietMisdreavus
Copy link
Member Author

r? @GuillaumeGomez

@TimNN
Copy link
Contributor

TimNN commented Apr 11, 2018

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:00:47] configure: rust.quiet-tests     := True
---
[00:44:47] ..............................................................................i.....................
[00:44:53] .....................i..............................................................................
---
[00:45:36] i..........................................................................i........................
---
[00:46:33] .............................................i......................................................
---
[00:50:33] .............................i......................................................................
[00:50:48] ..............................................................i.....................................
[00:51:03] ...............................................i....................................................
[00:51:24] ....................................................................................................
[00:51:46] ....................................................................................................
[00:52:08] ....................................................................................................
[00:52:34] ...i...............................................................................................i
[00:53:01] .............................................................................test [run-pass] run-pass/mir_heavy_promoted.rs has been running for over 60 seconds
[00:53:12] .......................
[00:53:43] ....................................................................................................
[00:54:19] .................................................................ii.................................
[00:55:05] ............................i....................................................i.ii.test [run-pass] run-pass/saturating-float-casts.rs has been running for over 60 seconds
[00:55:12] ..............
[00:55:55] .........................................................................................iiiiiii....
---
[00:58:10] ...................i............................................................ii.iii..............
[00:58:17] ....................................................................................................
[00:58:25] .........i..............................i...........................................................
[00:58:33] ....................................................................................................
[00:58:40] .....................i..............................................................................
[00:58:48] ....................................................................................................
[00:58:58] ....................................................................................................
[00:59:08] ....................................................................................................
[00:59:19] ....................................................................................................
[00:59:33] ....................................................................................................
[00:59:42] ..............i.....................................................................................
[00:59:52] ..................i..ii.............................................................................
[01:00:02] ....................................................................................................
[01:00:12] ....................................................................................................
[01:00:22] .....................................................................................i..............
[01:00:32] ...............................i....................................................................
---
[01:01:10] ...........................i........................................................................
[01:01:12] ....................................................................i...............................
[01:01:13] ................i.......................................................
---
[01:01:28] .............i........................
---
[01:01:59] i...i..ii....i.............ii.........iii......i..i...i...ii..i..i..ii.....
---
[01:02:02] i.......i......................i.......
---
[01:02:41] iiii.......i..i........i..i.i.............i..........iiii...........i...i..........ii.i.i.......ii..
[01:02:42] ....ii...
---
[01:10:31] ....................F...............................................................................
[01:12:07] .......i............................................................................................
---
[01:12:30] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--rustdoc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustdoc" "--src-base" "/checkout/src/test/rustdoc" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "rustdoc" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-3.9/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zmiri -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zmiri -Zunstable-options  -Lnative=/checnknown-linux-gnu/stage0-sysroot/lib/rustlib/x86_64-unknown-linux-gnu
---
56592 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/incremental/syntax-33oa6nnkk1g08/s-f006tf4qli-10t4q2j-v8bl3ay9sjlv

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 @TimNN. (Feature Requests)

@GuillaumeGomez
Copy link
Member

It'd be actually nice to pass compile options to tests... We should definitely talk about it!

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 11, 2018

Thanks, I think that for the time being this is a good solution!

It allows users to at least provide their own hidden environment (main, #[target_feature], use static/run-time feature detection, etc.) and that is something that we will always want to provide for maximum control.

It'd be actually nice to pass compile options to tests... We should definitely talk about it!

If cargo test would set the RUSTFLAGS environment variable for doc tests that would already be great.

@QuietMisdreavus
Copy link
Member Author

Just passing RUSTFLAGS to doctests won't work - rustdoc handles the compilation of doctests itself, which bypasses any other command-line flags that you may want to hand to rustc. Rustdoc would need to handle the -C flag itself and pass those codegen options on when setting up the tests.

@QuietMisdreavus
Copy link
Member Author

I was able to get this test to work locally, so i'm blaming the CI environment. Is there something i'm not seeing here? @gnzlbg I thought the sse feature was supposed to be always enabled on x86_64?

/// ```
/// assert!(false);
/// ```
#[cfg(target_feature = "sse")]
Copy link
Contributor

Choose a reason for hiding this comment

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

If this doctest fails, is because foo was not part of the binary, so... "sse" is not set for some reason, which is very weird. It is set for x86_64 always. cc @alexcrichton

Copy link
Member

Choose a reason for hiding this comment

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

Hm I'm not sure why this isn't working, rustc +nightly --print cfg clearly shows it's there.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you run the test locally (i.e. x.py test --stage 1 src/test/rustdoc --test-args "target-feature") then it will work properly, implying there's something off with the Travis setup.

Copy link
Member

Choose a reason for hiding this comment

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

Aha right yes, you can tag this as // no-system-llvm

@QuietMisdreavus
Copy link
Member Author

Aha right yes, you can tag this as // no-system-llvm

Well, this makes travis skip the test at least. It still runs and passes locally on my system, too, so hopefully it works this time.

I've reverted the test to the original version (that has feature detection both on the item and in the test) and squashed it down. If travis still agrees (i.e. skips the test) then this PR is ready. I have a local branch waiting for this to add a -C flag to rustdoc to allow for setting codegen options, including manually setting features.

@QuietMisdreavus
Copy link
Member Author

Alright, passed travis! @GuillaumeGomez ready for review!

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 13, 2018

@alexcrichton what was the problem? IIRC there is some stuff with target features that we are only doing with Rust's LLVM, was that it?

@alexcrichton
Copy link
Member

We can only read a list of target features from our own LLVM, not system LLVMs

@GuillaumeGomez
Copy link
Member

Let's go then!

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 14, 2018

📌 Commit 3366032 has been approved by GuillaumeGomez

@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 Apr 14, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Apr 14, 2018
…ures, r=GuillaumeGomez

add target features when extracting and running doctests

When rendering documentation, rustdoc will happily load target features into the cfg environment from the current target, but fails to do this when doing anything with doctests. This would lead to situations where, thanks to rust-lang#48759, functions tagged with `#[target_feature]` couldn't run doctests, thanks to the automatic `#[doc(cfg(target_feature = "..."))]`.

Currently, there's no way to pass codegen options to rustdoc that will affect its rustc sessions, but for now this will let you use target features that come default on the platform you're targeting.

Fixes rust-lang#49723
bors added a commit that referenced this pull request Apr 14, 2018
Rollup of 14 pull requests

Successful merges: #49908, #49876, #49916, #49951, #49465, #49922, #49866, #49915, #49886, #49913, #49852, #49958, #49871, #49864

Failed merges:
@bors bors merged commit 3366032 into rust-lang:master Apr 14, 2018
bors added a commit that referenced this pull request Apr 16, 2018
rustdoc: port the -C option from rustc

Blocked on #49864. The included test won't work without those changes, so this PR includes those commits as well.

When documenting items that require certain target features, it helps to be able to force those target features into existence. Rather than include a flag just to parse those features, i instead decided to port the `-C` flag from rustc in its entirety. It takes the same parameters, because it runs through the same parsing function. This has the added benefit of being able to control the codegen of doctests as well.

One concern i have with the flag is that i set it to stable here. My rationale is that it is a direct port of functionality on rustc that is currently stable, used only in mechanisms that it is originally used for. If needed, i can set it back to be unstable.
@QuietMisdreavus QuietMisdreavus deleted the doctest-target-features branch May 9, 2018 20:57
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.

8 participants