If static.files directory is present, use that

[jsha]

This prepares for rust-lang/rust#101702 by adding special handling of the static.files directory.

I still need to figure out how to get the rustwide builder to build based on my local toolchain in order to test this. I've gotten it working once before but I remember it was kinda complicated.

[syphar]

thank you for working on this!

some thoughts:

• won't rustdoc also put invocation specific files into that sub-directory? So the normal build also has to handle this somehow?
• when the files are also in the same subdirectory, do the links to these also include the subdirectory? or are they requested from the crate root as right now?

Also it would be awesome if we could start adding more tests for the build specifics too. Build-tests are relatively recent (see tests in this module) but I would love to cover more of the build process in these.

[jsha]

won't rustdoc also put invocation specific files into that sub-directory? So the normal build also has to handle this somehow?

Nope! That's part of the goal of using a different directory in rust-lang/rust#101702. Static files and invocation files will never be in the same directory. Invocation specific files will still be generated in the doc root (search-index, crates, source-files) or alongside HTML files in subdirectories (trait implementors, sidebar-items).

when the files are also in the same subdirectory, do the links to these also include the subdirectory? or are they requested from the crate root as right now?

After rust-lang/rust#101702 lands, files like main-xxx.js will still be loaded (<link> / <script>) from whatever URL is pointed to by --static-root-path. Those links will not tack on static.files at the end. One way to think about it is: the default value for --static-root-path is <doc root>/static.files. It's not precisely that, because the default value is more like ../../../static.files depending on how deep in the directory structure the current HTML file is. The ../ part is not applied when --static-root-path is given.

Also it would be awesome if we could start adding more tests for the build specifics too.

Sounds good, I'll give it a shot.

[jsha]

I was able to manually test this against a try build, using this patch to allow our rustwide builder to use CI builds:

diff --git a/Cargo.toml b/Cargo.toml
index e9e446eb..7716ab52 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -51,7 +51,7 @@ schemamama = "0.3"
 schemamama_postgres = "0.3"
 systemstat = "0.2.0"
 prometheus = { version = "0.13.0", default-features = false }
-rustwide = "0.15.0"
+rustwide = { version = "0.15.0", features = ["unstable-toolchain-ci"] }
 mime_guess = "2"
 zstd = "0.11.0"
 hostname = "0.3.1"
diff --git a/src/docbuilder/rustwide_builder.rs b/src/docbuilder/rustwide_builder.rs
index ff63baa5..2c4c62de 100644
--- a/src/docbuilder/rustwide_builder.rs
+++ b/src/docbuilder/rustwide_builder.rs
@@ -18,6 +18,7 @@ use anyhow::{anyhow, bail, Error};
 use docsrs_metadata::{Metadata, DEFAULT_TARGETS, HOST_TARGET};
 use failure::Error as FailureError;
 use postgres::Client;
+use regex::Regex;
 use rustwide::cmd::{Command, CommandError, SandboxBuilder, SandboxImage};
 use rustwide::logging::{self, LogStorage};
 use rustwide::toolchain::ToolchainError;
@@ -73,7 +74,16 @@ impl RustwideBuilder {
             .purge_all_build_dirs()
             .map_err(FailureError::compat)?;
 
-        let toolchain = Toolchain::dist(&config.toolchain);
+        // If the toolchain is all hex, assume it references an artifact from
+        // CI, for instance an `@bors try` build. 
+        let re = Regex::new(r"^[a-fA-F0-9]+$").unwrap();
+        let toolchain = if re.is_match(&config.toolchain) {
+            debug!("using CI build {}", &config.toolchain);
+            Toolchain::ci(&config.toolchain, false)
+        } else {
+            debug!("using toolchain {}", &config.toolchain);
+            Toolchain::dist(&config.toolchain)
+        };
 
         Ok(RustwideBuilder {
             workspace,
@@ -109,69 +119,70 @@ impl RustwideBuilder {
 
     pub fn update_toolchain(&mut self) -> Result<bool> {
         // Ignore errors if detection fails.
-        let old_version = self.detect_rustc_version().ok();
+        let old_version = Some("FOO");//self.detect_rustc_version().ok();
 
         let mut targets_to_install = DEFAULT_TARGETS
             .iter()
             .map(|&t| t.to_string()) // &str has a specialized ToString impl, while &&str goes through Display
             .collect::<HashSet<_>>();
 
-        let installed_targets = match self.toolchain.installed_targets(&self.workspace) {
-            Ok(targets) => targets,
-            Err(err) => {
-                if let Some(&ToolchainError::NotInstalled) = err.downcast_ref::<ToolchainError>() {
-                    Vec::new()
-                } else {
-                    return Err(err.compat().into());
-                }
-            }
-        };
-
-        // The extra targets are intentionally removed *before* trying to update.
-        //
-        // If a target is installed locally and it goes missing the next update, rustup will block
-        // the update to avoid leaving the system in a broken state. This is not a behavior we want
-        // though when we also remove the target from the list managed by docs.rs: we want that
-        // target gone, and we don't care if it's missing in the next update.
-        //
-        // Removing it beforehand works fine, and prevents rustup from blocking the update later in
-        // the method.
-        //
-        // Note that this means that non tier-one targets will be uninstalled on every update,
-        // and will not be reinstalled until explicitly requested by a crate.
-        for target in installed_targets {
-            if !targets_to_install.remove(&target) {
-                self.toolchain
-                    .remove_target(&self.workspace, &target)
-                    .map_err(FailureError::compat)?;
-            }
-        }
+//        let installed_targets = match self.toolchain.installed_targets(&self.workspace) {
+//            Ok(targets) => targets,
+//            Err(err) => {
+//                if let Some(&ToolchainError::NotInstalled) = err.downcast_ref::<ToolchainError>() {
+//                    Vec::new()
+//                } else {
+//                    eprintln!("{}", err);
+//                    return Err(err.compat().into());
+//                }
+//            }
+//        };
+//
+//        // The extra targets are intentionally removed *before* trying to update.
+//        //
+//        // If a target is installed locally and it goes missing the next update, rustup will block
+//        // the update to avoid leaving the system in a broken state. This is not a behavior we want
+//        // though when we also remove the target from the list managed by docs.rs: we want that
+//        // target gone, and we don't care if it's missing in the next update.
+//        //
+//        // Removing it beforehand works fine, and prevents rustup from blocking the update later in
+//        // the method.
+//        //
+//        // Note that this means that non tier-one targets will be uninstalled on every update,
+//        // and will not be reinstalled until explicitly requested by a crate.
+//        for target in installed_targets {
+//            if !targets_to_install.remove(&target) {
+//                self.toolchain
+//                    .remove_target(&self.workspace, &target)
+//                    .map_err(FailureError::compat)?;
+//            }
+//        }
 
         self.toolchain
             .install(&self.workspace)
             .map_err(FailureError::compat)?;
-
-        for target in &targets_to_install {
-            self.toolchain
-                .add_target(&self.workspace, target)
-                .map_err(FailureError::compat)?;
-        }
+//
+//        for target in &targets_to_install {
+//            self.toolchain
+//                .add_target(&self.workspace, target)
+//                .map_err(FailureError::compat)?;
+//        }
         // NOTE: rustup will automatically refuse to update the toolchain
         // if `rustfmt` is not available in the newer version
         // NOTE: this ignores the error so that you can still run a build without rustfmt.
         // This should only happen if you run a build for the first time when rustfmt isn't available.
-        if let Err(err) = self.toolchain.add_component(&self.workspace, "rustfmt") {
-            warn!("failed to install rustfmt: {}", err);
-            info!("continuing anyway, since this must be the first build");
-        }
+//        if let Err(err) = self.toolchain.add_component(&self.workspace, "rustfmt") {
+//            warn!("failed to install rustfmt: {}", err);
+//            info!("continuing anyway, since this must be the first build");
+//        }
 
         self.rustc_version = self.detect_rustc_version()?;
 
-        let has_changed = old_version.as_deref() != Some(&self.rustc_version);
-        if has_changed {
+//        let has_changed = old_version.as_deref() != Some(&self.rustc_version);
+//        if has_changed {
             self.add_essential_files()?;
-        }
-        Ok(has_changed)
+//        }
+        Ok(true)
     }
 
     fn detect_rustc_version(&self) -> Result<String> {

This should be used with export DOCSRS_TOOLCHAIN=62fd07b1ee7441dad3ff012468c1251d889a5f5b, which is the try build generated here. Note there has been another revision since, to improve performance, so we'll want to test it one last time before everything is nailed down.

[jsha]

Alright, rust-lang/rust#101702 is fully reviewed and ready to go. I've manually tested this docs.rs branch using the instructions above and the latest try build,

export DOCSRS_TOOLCHAIN=6703d43029387eef125596dd7071d0d4fab902c3

Which was built here: rust-lang/rust#101702 (comment)

And it all works.

[jyn514]

I was able to manually test this against a try build, using this patch to allow our rustwide builder to use CI builds:

this is fantastic, could you make a PR with that please? I've wanted to support this for local development for ages.

[jsha]

I will try. Unfortunately most of the patch is just commenting out lines of code I don't fully understand, so I need to figure out what those do so I can decide to either make them not happen for CI builds or fix them so they work with CI builds. But I definitely agree this would be very helpful for development and would like to have it.