Add a mode for reading docs.rs metadata when running rustdoc #543
Conversation
| What's the backtrace of that error? |
Backtrace |
| Huh, so it looks like it's not actually related to |
| Hmm, try manually removing |
| This looks like it was the issue: I removed them both with sudo and now things seem to be working. |
| Sure, but this needs to be handled on the rustwide side. |
| It looks like this is also a crater bug: whenever you interrupt the process (with Ctrl+C) it will leave around files owned by root, causing it to fail the next time you run tests. |
| I don't think there are any existing tests for |
| Is there a way to run a minicrater test against only some crates, instead of all of them? Right now doc_small { ex: "doc", crate_select: "local", mode: "rustdoc", ..Default::default() },and local-crates = ["build-pass", "docs-rs-features"]which makes it seem like only 2 crates should be run, but it's running all of them. |
| I tried this diff: diff --git a/src/config.rs b/src/config.rs index a7e1c08..bdbe514 100644 --- a/src/config.rs +++ b/src/config.rs @@ -119,7 +119,9 @@ impl Config { } pub fn should_skip(&self, c: &Crate) -> bool { - self.crate_config(c).map(|c| c.skip).unwrap_or(false) + // Default to `true` for local crates so it doesn't always run every test + let default_skip = matches!(c, Crate::Local(_)); + self.crate_config(c).map(|c| c.skip).unwrap_or(default_skip) } pub fn should_skip_tests(&self, c: &Crate) -> bool {but it ended up skipping every crate, even crates with |
| @jyn514 |
| "res": "spurious-fixed", | ||
| "res": "test-pass", | ||
| "runs": [ | ||
| { | ||
| "log": "stable/local/memory-hungry", | ||
| "res": "build-fail:oom" | ||
| "res": "test-pass" | ||
| }, | ||
| { | ||
| "log": "beta/local/memory-hungry", | ||
| "res": "test-fail:oom" | ||
| "res": "test-pass" |
There was a problem hiding this comment.
This should not have been blessed.
There was a problem hiding this comment.
How can I make sure my changes are right if they're different on CI and locally?
There was a problem hiding this comment.
I ended up only running minicrater::doc which seems to work well enough. But I'd have the same question if I needed to modify resource_exhaustion.
This comment has been minimized.
This comment has been minimized.
| @bors try |
| @bors try again |
[WIP] Read docs.rs metadata when running rustdoc Closes #532 <details><summary>Outdated errors</summary> I'm having trouble running the test suite, so I haven't yet added a test. My idea was to have a crate that only builds successfully when you read the metadata, but I'm not sure if that would work with the way the test suite is set up. Current errors: ``` $ cargo test minicrater -- single_thread_small --ignored --test-threads 1 [2020-09-05T02:02:04Z INFO rustwide::cmd] running `Command { std: "/home/joshua/src/rust/crater/work/cargo-home/bin/cargo" "+stable" "install" "lazy_static", kill_on_drop: false }` [2020-09-05T02:02:04Z INFO rustwide::cmd] [stderr] Updating crates.io index [2020-09-05T02:02:04Z INFO rustwide::cmd] [stderr] error: specified package `lazy_static v1.4.0` has no binaries [2020-09-05T02:02:04Z ERROR crater::utils] Permission denied (os error 13) [2020-09-05T02:02:04Z ERROR crater::utils] note: run with `RUST_BACKTRACE=1` to display a backtrace. [2020-09-05T02:02:04Z INFO crater] command failed ', /home/joshua/.local/lib/cargo/registry/src/github.com-1ecc6299db9ec823/assert_cmd-0.10.1/src/assert.rs:154:13 ``` </details> Before merging, I need to publish the `docsrs-metadata` package to crates.io so crater doesn't depend on a git version. r? `@pietroalbini` help
| 💔 Test failed - checks-actions |
| @bors try |
[WIP] Read docs.rs metadata when running rustdoc Closes #532 <details><summary>Outdated errors</summary> I'm having trouble running the test suite, so I haven't yet added a test. My idea was to have a crate that only builds successfully when you read the metadata, but I'm not sure if that would work with the way the test suite is set up. Current errors: ``` $ cargo test minicrater -- single_thread_small --ignored --test-threads 1 [2020-09-05T02:02:04Z INFO rustwide::cmd] running `Command { std: "/home/joshua/src/rust/crater/work/cargo-home/bin/cargo" "+stable" "install" "lazy_static", kill_on_drop: false }` [2020-09-05T02:02:04Z INFO rustwide::cmd] [stderr] Updating crates.io index [2020-09-05T02:02:04Z INFO rustwide::cmd] [stderr] error: specified package `lazy_static v1.4.0` has no binaries [2020-09-05T02:02:04Z ERROR crater::utils] Permission denied (os error 13) [2020-09-05T02:02:04Z ERROR crater::utils] note: run with `RUST_BACKTRACE=1` to display a backtrace. [2020-09-05T02:02:04Z INFO crater] command failed ', /home/joshua/.local/lib/cargo/registry/src/github.com-1ecc6299db9ec823/assert_cmd-0.10.1/src/assert.rs:154:13 ``` </details> Before merging, I need to publish the `docsrs-metadata` package to crates.io so crater doesn't depend on a git version. r? `@pietroalbini` help
| 💔 Test failed - checks-actions |
| This might be a bad idea. Some of my crates have a line similar to this: That |
| I think the plan was to set RUSTC_BOOTSTRAP so there wouldn't be spurious failures. |
| I have not been able to get the tests to pass locally. |
| Can you run it with |
| Ugh, this was #545. |
Here is an example where crater runs are useful, and likely misleading because the metadata is ignored: rust-lang/cargo#10343 (comment) |
This is bad, though. It means that the first time we notice regressions in rustdoc is after they're already impacting people publishing crates to the official registry. You could argue that docs.rs should be using a stable toolchain, and I don't disagree, but I don't see a way to make that possible without breaking basically every major crate ... rust-lang/docs.rs#506 |
I think you are underestimating how many crates use nightly features. Basically all major crates I know are using |
| I think there's two use cases for crater: preventing regressions in general, and preventing regressions for stable users. For users on stable, they are not typically impacted by anything enabled by this PR, and indeed are rather the reverse: when I'm using a stable toolchain and running cargo doc, I'm building tokio etc without the docs.rs metadata enabled feature flags etc. That scenario wouldn't be tested if we landed this PR. I agree that there is a major whole in our regression prevention story around docs.rs and crates not having a good way to work around nightly bugs impacting new releases (I guess manual rebuild requests). If we wanted to prevent ICEs and the like there, then we'd need a fairly frequent (i.e., more than new betas) crater run for nightly - this is feasible machine capacity wise I suspect, but poses a real challenge from a human triage hands perspective. (Certainly I don't have bandwidth to do that triage). For crater runs that already happen on nightly (e.g. from PRs) it makes sense to me we'd potentially want to also run in this mode (in addition to the "bare" mode we typically do); in fact, I could see us just always doing that. (Duplicating each crate similar to compare-mode=nll in compiletest, though in this case with maybe lightweight detection of it not being needed due to lack of metadata?) I think the tradeoff between testing builds that are only exercised by CI and docs.rs (i.e., in both cases surfacing issues to crate authors more so than users) and regular cargo doc builds (which would be covered by the current mode) is not clear. Doing both seems like a relatively easy out, and might not be hard to add - we already run multiple cargo invocations I think sometimes, so we'd just do that. |
👍 this is a good point. I like your idea of adding a separate mode for docs.rs metadata and only using that for nightly :) I can try to implement that. |
| Just to clarify, I think we should consider just always running both with and without docs.rs metadata - that is likely to be simpler and would likely cover both desires better. I'm not sure it'll be a 2x hit in terms of runtime either. |
I am confused. Is the idea to run both within the same run, so it reuses the build cache? I am not really sure how to implement that :( at least not in a way that doesn't break the reports. |
jyn514 changed the title | I think the way the code works it basically runs cargo doc today, I'm saying we would run it with the changes made in this PR and without the changes made in this PR. |
jyn514 force-pushed the doc-runs branch 2 times, most recently from 5c215f8 to fbebafb Compare
Ok, I implemented that. I didn't realize that crater could run cargo more than once, but I found in |
| @bors r+ |
| 📌 Commit fbebafb has been approved by |
Add a mode for reading docs.rs metadata when running rustdoc Closes #532 Before merging, I need to publish the `docsrs-metadata` package to crates.io so crater doesn't depend on a git version.
| 💔 Test failed - checks-actions |
- Add tests for doc runs - Only give an error when docs.rs feature is set in the test crate This allows distinguishing between 'any doc run' and 'doc run that uses docs.rs features' - Set RUSTC_BOOTSTRAP for doc runs This is required for both docs.rs features and the flags passed by docsrs_metadata to cargo. - Only use docs.rs features for libraries `docsrs_metadata` unconditionally passes `--lib` to cargo, which gives a hard error when no library is present. This also required changing the setup in `runner/tests` to pass through the full package metadata to all test runners.
| Tests should hopefully be fixed - I had to update the reports for the |
| @bors r+ |
| 📌 Commit 339c2ed has been approved by |
| ☀️ Test successful - checks-actions |
6 participants
Closes #532
Before merging, I need to publish the
docsrs-metadatapackage to crates.io so crater doesn't depend on a git version.