Rollup merge of rust-lang#73883 - ehuss:rustdoc-stage-previous, r=Mark-Simulacrum

Manishearth · web-flow · commit 7b2f44a57b68 · 2020-07-02T00:16:36.000-07:00
Compile rustdoc less often. Previously rustdoc was built 3 times with `x.py test`: 1. stage2 (using stage1 compiler) for compiletest tests (stage1-tools copied to stage2). 2. stage1 (using stage0 compiler) for std crate tests (stage0-tools copied to stage1). 3. stage2 test (using stage2 compiler) for rustdoc crate tests and error_index_generator (stage2-tools). This PR removes the majority of number 3, where it will instead use the stage1 compiler, which will share the artifacts from number 1. This matches the behavior of the libstd crate tests. I don't think it is entirely necessary to run the tests using stage2. At `-j2`, the last build step goes from about 300s to 70s on my machine. It's not a huge win, but shaving 4 minutes isn't bad. The other two builds would be pretty difficult (or undesired or impossible) to unify. It looks like std tests use stage1 very intentionally (see `force_use_stage1` and its history), and compiletests use the top stage very intentionally. Unfortunately the linkchecker builds all docs at stage2 (stage2-tools), which means a few build script artifacts are not shared. It's not really clear to me how to fix that (because it uses `default_doc`, there doesn't seem to be any control over the stages). --- For `x.py doc`, rustdoc was previously built three times (with compiler-docs): 1. stage2 (using stage1 compiler) for normal documentation output (stage1-tools copied to stage2). 2. stage1 (using stage0 compiler) for compiler-docs 3. stage2 (using stage2 compiler) for error_index_generator (stage2-tools) This PR combines these so that they consistently use the "top stage" rustdoc. I don't know why the compiler-docs was written to use stage minus one, but it seems better to be consistent across the doc steps. --- I've tried to test this with a variety of commands (`x.py doc`, `x.py test`, different `--stage` flags, `full-bootstrap`, setting `--target`, etc.) to try to make sure there aren't significant regressions here. It's tricky since there are so many variables, and this stuff is difficult for me to fully understand. Closes rust-lang#70799 (I think)
diff --git a/src/bootstrap/builder/tests.rs b/src/bootstrap/builder/tests.rs
@@ -54,6 +54,11 @@ fn dist_baseline() {
         &[dist::Std { compiler: Compiler { host: a, stage: 1 }, target: a },]
     );
     assert_eq!(first(builder.cache.all::<dist::Src>()), &[dist::Src]);
+    // Make sure rustdoc is only built once.
+    assert_eq!(
+        first(builder.cache.all::<tool::Rustdoc>()),
+        &[tool::Rustdoc { compiler: Compiler { host: a, stage: 2 } },]
+    );
 }
 
 #[test]
@@ -414,3 +419,77 @@ fn test_exclude() {
     // Ensure other tests are not affected.
     assert!(builder.cache.contains::<test::RustdocUi>());
 }
+
+#[test]
+fn doc_default() {
+    let mut config = configure(&[], &[]);
+    config.compiler_docs = true;
+    config.cmd = Subcommand::Doc { paths: Vec::new(), open: false };
+    let build = Build::new(config);
+    let mut builder = Builder::new(&build);
+    builder.run_step_descriptions(&Builder::get_step_descriptions(Kind::Doc), &[]);
+    let a = INTERNER.intern_str("A");
+
+    // error_index_generator uses stage 1 to share rustdoc artifacts with the
+    // rustdoc tool.
+    assert_eq!(
+        first(builder.cache.all::<doc::ErrorIndex>()),
+        &[doc::ErrorIndex { compiler: Compiler { host: a, stage: 1 }, target: a },]
+    );
+    assert_eq!(
+        first(builder.cache.all::<tool::ErrorIndex>()),
+        &[tool::ErrorIndex { compiler: Compiler { host: a, stage: 1 } }]
+    );
+    // This is actually stage 1, but Rustdoc::run swaps out the compiler with
+    // stage minus 1 if --stage is not 0. Very confusing!
+    assert_eq!(
+        first(builder.cache.all::<tool::Rustdoc>()),
+        &[tool::Rustdoc { compiler: Compiler { host: a, stage: 2 } },]
+    );
+}
+
+#[test]
+fn test_docs() {
+    // Behavior of `x.py test` doing various documentation tests.
+    let mut config = configure(&[], &[]);
+    config.cmd = Subcommand::Test {
+        paths: vec![],
+        test_args: vec![],
+        rustc_args: vec![],
+        fail_fast: true,
+        doc_tests: DocTests::Yes,
+        bless: false,
+        compare_mode: None,
+        rustfix_coverage: false,
+        pass: None,
+    };
+    let build = Build::new(config);
+    let mut builder = Builder::new(&build);
+    builder.run_step_descriptions(&Builder::get_step_descriptions(Kind::Test), &[]);
+    let a = INTERNER.intern_str("A");
+
+    // error_index_generator uses stage 1 to share rustdoc artifacts with the
+    // rustdoc tool.
+    assert_eq!(
+        first(builder.cache.all::<doc::ErrorIndex>()),
+        &[doc::ErrorIndex { compiler: Compiler { host: a, stage: 1 }, target: a },]
+    );
+    assert_eq!(
+        first(builder.cache.all::<tool::ErrorIndex>()),
+        &[tool::ErrorIndex { compiler: Compiler { host: a, stage: 1 } }]
+    );
+    // Unfortunately rustdoc is built twice. Once from stage1 for compiletest
+    // (and other things), and once from stage0 for std crates. Ideally it
+    // would only be built once. If someone wants to fix this, it might be
+    // worth investigating if it would be possible to test std from stage1.
+    // Note that the stages here are +1 than what they actually are because
+    // Rustdoc::run swaps out the compiler with stage minus 1 if --stage is
+    // not 0.
+    assert_eq!(
+        first(builder.cache.all::<tool::Rustdoc>()),
+        &[
+            tool::Rustdoc { compiler: Compiler { host: a, stage: 1 } },
+            tool::Rustdoc { compiler: Compiler { host: a, stage: 2 } },
+        ]
+    );
+}
diff --git a/src/bootstrap/doc.rs b/src/bootstrap/doc.rs
@@ -518,8 +518,7 @@ impl Step for Rustc {
         let out = builder.compiler_doc_out(target);
         t!(fs::create_dir_all(&out));
 
-        // Get the correct compiler for this stage.
-        let compiler = builder.compiler_for(stage, builder.config.build, target);
+        let compiler = builder.compiler(stage, builder.config.build);
 
         if !builder.config.compiler_docs {
             builder.info("\tskipping - compiler/librustdoc docs disabled");
@@ -599,8 +598,7 @@ impl Step for Rustdoc {
         let out = builder.compiler_doc_out(target);
         t!(fs::create_dir_all(&out));
 
-        // Get the correct compiler for this stage.
-        let compiler = builder.compiler_for(stage, builder.config.build, target);
+        let compiler = builder.compiler(stage, builder.config.build);
 
         if !builder.config.compiler_docs {
             builder.info("\tskipping - compiler/librustdoc docs disabled");
@@ -639,9 +637,10 @@ impl Step for Rustdoc {
     }
 }
 
-#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
+#[derive(Ord, PartialOrd, Debug, Copy, Clone, Hash, PartialEq, Eq)]
 pub struct ErrorIndex {
-    target: Interned<String>,
+    pub compiler: Compiler,
+    pub target: Interned<String>,
 }
 
 impl Step for ErrorIndex {
@@ -655,26 +654,26 @@ impl Step for ErrorIndex {
     }
 
     fn make_run(run: RunConfig<'_>) {
-        run.builder.ensure(ErrorIndex { target: run.target });
+        let target = run.target;
+        // error_index_generator depends on librustdoc. Use the compiler that
+        // is normally used to build rustdoc for other documentation so that
+        // it shares the same artifacts.
+        let compiler =
+            run.builder.compiler_for(run.builder.top_stage, run.builder.config.build, target);
+        run.builder.ensure(ErrorIndex { compiler, target });
     }
 
     /// Generates the HTML rendered error-index by running the
     /// `error_index_generator` tool.
     fn run(self, builder: &Builder<'_>) {
-        let target = self.target;
-
-        builder.info(&format!("Documenting error index ({})", target));
-        let out = builder.doc_out(target);
+        builder.info(&format!("Documenting error index ({})", self.target));
+        let out = builder.doc_out(self.target);
         t!(fs::create_dir_all(&out));
-        let compiler = builder.compiler(2, builder.config.build);
-        let mut index = tool::ErrorIndex::command(builder, compiler);
+        let mut index = tool::ErrorIndex::command(builder, self.compiler);
         index.arg("html");
         index.arg(out.join("error-index.html"));
         index.arg(crate::channel::CFG_RELEASE_NUM);
 
-        // FIXME: shouldn't have to pass this env var
-        index.env("CFG_BUILD", &builder.config.build);
-
         builder.run(&mut index);
     }
 }
diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs
@@ -1454,8 +1454,11 @@ impl Step for ErrorIndex {
     }
 
     fn make_run(run: RunConfig<'_>) {
-        run.builder
-            .ensure(ErrorIndex { compiler: run.builder.compiler(run.builder.top_stage, run.host) });
+        // error_index_generator depends on librustdoc. Use the compiler that
+        // is normally used to build rustdoc for other tests (like compiletest
+        // tests in src/test/rustdoc) so that it shares the same artifacts.
+        let compiler = run.builder.compiler_for(run.builder.top_stage, run.host, run.host);
+        run.builder.ensure(ErrorIndex { compiler });
     }
 
     /// Runs the error index generator tool to execute the tests located in the error
@@ -1467,22 +1470,23 @@ impl Step for ErrorIndex {
     fn run(self, builder: &Builder<'_>) {
         let compiler = self.compiler;
 
-        builder.ensure(compile::Std { compiler, target: compiler.host });
-
         let dir = testdir(builder, compiler.host);
         t!(fs::create_dir_all(&dir));
         let output = dir.join("error-index.md");
 
-        let mut tool = tool::ErrorIndex::command(
-            builder,
-            builder.compiler(compiler.stage, builder.config.build),
-        );
-        tool.arg("markdown").arg(&output).env("CFG_BUILD", &builder.config.build);
+        let mut tool = tool::ErrorIndex::command(builder, compiler);
+        tool.arg("markdown").arg(&output);
 
-        builder.info(&format!("Testing error-index stage{}", compiler.stage));
+        // Use the rustdoc that was built by self.compiler. This copy of
+        // rustdoc is shared with other tests (like compiletest tests in
+        // src/test/rustdoc). This helps avoid building rustdoc multiple
+        // times.
+        let rustdoc_compiler = builder.compiler(builder.top_stage, builder.config.build);
+        builder.info(&format!("Testing error-index stage{}", rustdoc_compiler.stage));
         let _time = util::timeit(&builder);
         builder.run_quiet(&mut tool);
-        markdown_test(builder, compiler, &output);
+        builder.ensure(compile::Std { compiler: rustdoc_compiler, target: rustdoc_compiler.host });
+        markdown_test(builder, rustdoc_compiler, &output);
     }
 }
 
@@ -1797,9 +1801,13 @@ impl Step for CrateRustdoc {
 
     fn run(self, builder: &Builder<'_>) {
         let test_kind = self.test_kind;
+        let target = self.host;
 
-        let compiler = builder.compiler(builder.top_stage, self.host);
-        let target = compiler.host;
+        // Use the previous stage compiler to reuse the artifacts that are
+        // created when running compiletest for src/test/rustdoc. If this used
+        // `compiler`, then it would cause rustdoc to be built *again*, which
+        // isn't really necessary.
+        let compiler = builder.compiler_for(builder.top_stage, target, target);
         builder.ensure(compile::Rustc { compiler, target });
 
         let mut cargo = tool::prepare_tool_cargo(
@@ -1825,6 +1833,32 @@ impl Step for CrateRustdoc {
             cargo.arg("'-Ctarget-feature=-crt-static'");
         }
 
+        // This is needed for running doctests on librustdoc. This is a bit of
+        // an unfortunate interaction with how bootstrap works and how cargo
+        // sets up the dylib path, and the fact that the doctest (in
+        // html/markdown.rs) links to rustc-private libs. For stage1, the
+        // compiler host dylibs (in stage1/lib) are not the same as the target
+        // dylibs (in stage1/lib/rustlib/...). This is different from a normal
+        // rust distribution where they are the same.
+        //
+        // On the cargo side, normal tests use `target_process` which handles
+        // setting up the dylib for a *target* (stage1/lib/rustlib/... in this
+        // case). However, for doctests it uses `rustdoc_process` which only
+        // sets up the dylib path for the *host* (stage1/lib), which is the
+        // wrong directory.
+        //
+        // It should be considered to just stop running doctests on
+        // librustdoc. There is only one test, and it doesn't look too
+        // important. There might be other ways to avoid this, but it seems
+        // pretty convoluted.
+        //
+        // See also https://github.com/rust-lang/rust/issues/13983 where the
+        // host vs target dylibs for rustdoc are consistently tricky to deal
+        // with.
+        let mut dylib_path = dylib_path();
+        dylib_path.insert(0, PathBuf::from(&*builder.sysroot_libdir(compiler, target)));
+        cargo.env(dylib_path_var(), env::join_paths(&dylib_path).unwrap());
+
         if !builder.config.verbose_tests {
             cargo.arg("--quiet");
         }
diff --git a/src/bootstrap/tool.rs b/src/bootstrap/tool.rs
@@ -366,7 +366,7 @@ bootstrap_tool!(
     ExpandYamlAnchors, "src/tools/expand-yaml-anchors", "expand-yaml-anchors";
 );
 
-#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
+#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq, Ord, PartialOrd)]
 pub struct ErrorIndex {
     pub compiler: Compiler,
 }
@@ -392,9 +392,9 @@ impl Step for ErrorIndex {
     fn make_run(run: RunConfig<'_>) {
         // Compile the error-index in the same stage as rustdoc to avoid
         // recompiling rustdoc twice if we can.
-        let stage = if run.builder.top_stage >= 2 { run.builder.top_stage } else { 0 };
-        run.builder
-            .ensure(ErrorIndex { compiler: run.builder.compiler(stage, run.builder.config.build) });
+        let host = run.builder.config.build;
+        let compiler = run.builder.compiler_for(run.builder.top_stage, host, host);
+        run.builder.ensure(ErrorIndex { compiler });
     }
 
     fn run(self, builder: &Builder<'_>) -> PathBuf {
@@ -449,7 +449,7 @@ impl Step for RemoteTestServer {
     }
 }
 
-#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
+#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq, Ord, PartialOrd)]
 pub struct Rustdoc {
     /// This should only ever be 0 or 2.
     /// We sometimes want to reference the "bootstrap" rustdoc, which is why this option is here.
