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

Added ability to crosscompile doctests #6892

Merged
merged 3 commits into from Sep 19, 2019

Conversation

@Goirad
Copy link

commented Apr 29, 2019

This commit adds the ability to cross-compile and run doctests.
Like before cargo checks if target == host, the difference is that if there is a runtool defined in config.toml, it passes the information forward to rustdoc so that it can run the doctests with that tool. If no tool is defined and the target != host, cargo instead displays a message that doctests will not be compiled because of the missing runtool.

See here for the companion PR in the rust project that modifies rustdoc to accept the relevant options as well as allow ignoring doctests on a per target level.
Partially resolves #6460

See here for the tracking issue.

@rust-highfive

This comment has been minimized.

Copy link

commented Apr 29, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

src/cargo/ops/cargo_test.rs Outdated Show resolved Hide resolved
src/cargo/ops/cargo_test.rs Outdated Show resolved Hide resolved
@alexcrichton

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Thanks for the PR, but shouldn't the rustdoc PR also have support for passing arguments to the runtool?

@Goirad

This comment has been minimized.

Copy link
Author

commented Apr 30, 2019

Thanks for the PR, but shouldn't the rustdoc PR also have support for passing arguments to the runtool?

Yes it does already, rustdoc can take a --runtool argument which it will use for running tests. It is in the other PR. For example

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Er sorry my point is that there's a comment here that says //currently does not support passing args along with the runtool, but that should be fixed before an implementation is landed in rustdoc

@jethrogb

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

How? Compiletest runtool support just doesn't allow passing arguments with spaces in them, I guess that could also be done here for consistency? But I don't think compiletest should really be setting any precedents for a public API.

@Goirad Goirad force-pushed the Goirad:doctest-xcompile branch from 602c035 to 7b092de May 2, 2019
@ehuss ehuss added the S-blocked label May 8, 2019
@Goirad

This comment has been minimized.

Copy link
Author

commented May 14, 2019

I have fixed some tests that were causing the checks to fail. At this point I am waiting on advisement for the runtool argument passing.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@Goirad I'm not sure there's much advice to give? The arguments specified need to be handled and passed to rustdoc, but they currently aren't. There's not really a preference of how to handle them, but they need to be handled.

@jethrogb

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

@alexcrichton I suppose the question is how to pass arguments with spaces?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented May 15, 2019

I don't really understand the question in the sense that there's not any code to talk about to see where a bug might be or comment on that. If arguments are passed as process arguments it doesn't matter, it's only if one end does string parsing or something like that.

@jethrogb

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

The main question is how to get consistent behavior across all tools?

  1. bootstrap runs rustdoc and compiletest
  2. cargo runs rustdoc and compilation artifacts

compiletest takes a single --runtool argument that is then space-split to get the cmd and args.
cargo obtains from its configuration a vector of cmd followed by args, these may contain spaces.

How is cargo supposed to pass its vector to rustdoc in a way that's compatible with what bootstrap will be doing?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented May 16, 2019

I don't really know nor do I have a preference. I don't personally have time to help design this feature, but I can provide review feedback that this isn't handled and needs to be handled.

@Goirad Goirad force-pushed the Goirad:doctest-xcompile branch from 7b092de to 1be4c34 Jun 5, 2019
@Goirad

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

@ehuss As per Alex's suggestion, Cargo now passes arguments too, and the companion PR has been updated so that rustdoc accepts and uses them

@Goirad

This comment has been minimized.

Copy link
Author

commented Jun 7, 2019

I have modified the companion PR for rustdoc to feature-gate ignore-foo attributes in doctests.
I would like some input on how to handle this in Cargo. As the PR is currently written, there is no new feature in Cargo, and if someone runs cargo test --target foo without setting a runner in .cargo/config for foo, the only difference is that a message is displayed warning them that doctests were skipped because no runner was set, instead of the current behavior, which is to completely skip the doctest section.
If on the other hand a runner is defined, then Cargo automatically adds -Z unstable-options to the rustdoc invokations, since --runtool is unstable in rustdoc, and passes the runtool and its arguments. It also enables the feature for ignoring doctests on a per target basis, since that seems critical for cross-compiling doctests.
My question is if any of these three things should be behind feature-gates in Cargo, namely cross-compiling doctests at all, passing runtool, and enabling per-target ignores; and if so whether there should be one feature for all of them, or some more granular approach.

src/cargo/ops/cargo_test.rs Outdated Show resolved Hide resolved
@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

I think it's fine to skip feature gates here since we'll just inherit the feature gates enabled in rustdoc.

@Goirad Goirad force-pushed the Goirad:doctest-xcompile branch from 1be4c34 to f3734aa Jun 10, 2019
@jethrogb

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

@alexcrichton can you elaborate? I'm not sure how that would work. In your proposal, would cargo test --target ... just run rustdoc --runtool ... and the user would see an error message because cargo passed unstable arguments?

@Goirad

This comment has been minimized.

Copy link
Author

commented Jun 10, 2019

One option would be to just opt in to unstable arguments as well as per-target-ignores when a runtool is found. Although that leads to the situation where to be able to opt in to per-target ignores you need to have a runtool defined for the current target.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Actually thinking about this it's already stable behavior today that we're not running doctests on cross-compiled scenarios with a runner configured. That can't regress in Cargo (and would if this patch lands as-is) so it probably means that we'll need a feature gate of some form in Cargo to enable this behavior.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

This seems reasonable enough to me, and it's just blocked on the rustc PR landing. Before landing this can you open a tracking issue in this repository explaining the feature, what's unstable, etc?

@Goirad

This comment has been minimized.

Copy link
Author

commented Jun 18, 2019

@alexcrichton Yes of course, tracking issue here.

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

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

@Goirad Goirad force-pushed the Goirad:doctest-xcompile branch 2 times, most recently from 3309c8e to 240cf7f Sep 12, 2019
@Goirad

This comment has been minimized.

Copy link
Author

commented Sep 12, 2019

@alexcrichton I've added tests for both of the new features introduced in rustdoc

Copy link
Member

left a comment

Thanks! Could a test be added which exercises the runner functionality as well?

src/cargo/ops/cargo_test.rs Outdated Show resolved Hide resolved
@alexcrichton

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

Could the tests also be updated to assert the right flags (like --target) are being passed to rustdoc?

@Goirad Goirad force-pushed the Goirad:doctest-xcompile branch from 240cf7f to f6ef349 Sep 16, 2019
@Goirad

This comment has been minimized.

Copy link
Author

commented Sep 16, 2019

@alexcrichton Tests have been updated

Copy link
Member

left a comment

Can this also update the list of unstable features at src/doc/src/reference/unstable.md?

src/cargo/ops/cargo_test.rs Outdated Show resolved Hide resolved
tests/testsuite/test.rs Outdated Show resolved Hide resolved
tests/testsuite/test.rs Outdated Show resolved Hide resolved
tests/testsuite/test.rs Outdated Show resolved Hide resolved
@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

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

@Goirad Goirad force-pushed the Goirad:doctest-xcompile branch from f6ef349 to 26d3e7e Sep 18, 2019
@Goirad Goirad force-pushed the Goirad:doctest-xcompile branch 2 times, most recently from d54bdd6 to 8731579 Sep 18, 2019
Dario Gonzalez
@Goirad Goirad force-pushed the Goirad:doctest-xcompile branch from 8731579 to a2209fc Sep 18, 2019
@Goirad

This comment has been minimized.

Copy link
Author

commented Sep 18, 2019

@alexcrichton I have removed all OS restrictions from the tests

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

@bors: r+

awesome, thanks!

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

📌 Commit a2209fc has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

⌛️ Testing commit a2209fc with merge eadbaec...

bors added a commit that referenced this pull request Sep 18, 2019
Added ability to crosscompile doctests

This commit adds the ability to cross-compile and run doctests.
Like before cargo checks if target == host, the difference is that if there is a runtool defined in config.toml, it passes the information forward to rustdoc so that it can run the doctests with that tool. If no tool is defined and the target != host, cargo instead displays a message that doctests will not be compiled because of the missing runtool.

See [here](rust-lang/rust#60387) for the companion PR in the rust project that modifies rustdoc to accept the relevant options as well as allow ignoring doctests on a per target level.
Partially resolves [#6460](#6460)

See [here](#7040) for the tracking issue.
@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing eadbaec to master...

@bors bors merged commit a2209fc into rust-lang:master Sep 19, 2019
11 checks passed
11 checks passed
homu Test successful
Details
rust-lang.cargo Build #20190918.8 succeeded
Details
rust-lang.cargo (Linux beta) Linux beta succeeded
Details
rust-lang.cargo (Linux nightly) Linux nightly succeeded
Details
rust-lang.cargo (Linux stable) Linux stable succeeded
Details
rust-lang.cargo (Windows x86_64-msvc) Windows x86_64-msvc succeeded
Details
rust-lang.cargo (build_std) build_std succeeded
Details
rust-lang.cargo (docs) docs succeeded
Details
rust-lang.cargo (macOS) macOS succeeded
Details
rust-lang.cargo (resolver) resolver succeeded
Details
rust-lang.cargo (rustfmt) rustfmt succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.