bootstrap: Use cargo's build.warnings=deny rather than -Dwarnings

This has two major advantages. First, it makes us less dependent on the
rustc shim, which is nice but not super important. More importantly, it
gives us much nicer caching properties.

Previously, `x build --warnings warn && x build --warnings deny` would
rebuild all of bootstrap, and if there were any warnings emitted on the
last build of the compiler, they would *not* fail the build, because
cargo would cache the output rather than rerunning the shim.

After this change, bootstrap rebuilds instantly, and cargo knows that it
should fail the build but *without* invalidating the cache.

Author: jyn514

src/bootstrap/bootstrap.py

@@ -1113,6 +1113,19 @@ def build_bootstrap_cmd(self, env):
         else:
             env["RUSTFLAGS"] = "-Zallow-features="
 
+        if not os.path.isfile(self.cargo()):
+            raise Exception("no cargo executable found at `{}`".format(self.cargo()))
+        args = [
+            self.cargo(),
+            "build",
+            "--jobs=" + self.jobs,
+            "--manifest-path",
+            os.path.join(self.rust_root, "src/bootstrap/Cargo.toml"),
+            "-Zroot-dir=" + self.rust_root,
+        ]
+        # verbose cargo output is very noisy, so only enable it with -vv
+        args.extend("--verbose" for _ in range(self.verbose - 1))
+
         target_features = []
         if self.get_toml("crt-static", build_section) == "true":
             target_features += ["+crt-static"]
@@ -1131,28 +1144,15 @@ def build_bootstrap_cmd(self, env):
         else:
             deny_warnings = self.warnings == "deny"
         if deny_warnings:
-            env["RUSTFLAGS"] += " -Dwarnings"
+            args += ["-Zwarnings"]
+            env["CARGO_BUILD_WARNINGS"] = "deny"
 
         # Add RUSTFLAGS_BOOTSTRAP to RUSTFLAGS for bootstrap compilation.
         # Note that RUSTFLAGS_BOOTSTRAP should always be added to the end of
-        # RUSTFLAGS to be actually effective (e.g., if we have `-Dwarnings` in
-        # RUSTFLAGS, passing `-Awarnings` from RUSTFLAGS_BOOTSTRAP should override it).
+        # RUSTFLAGS, since that causes RUSTFLAGS_BOOTSTRAP to override RUSTFLAGS.
         if "RUSTFLAGS_BOOTSTRAP" in env:
             env["RUSTFLAGS"] += " " + env["RUSTFLAGS_BOOTSTRAP"]
 
-        if not os.path.isfile(self.cargo()):
-            raise Exception("no cargo executable found at `{}`".format(self.cargo()))
-        args = [
-            self.cargo(),
-            "build",
-            "--jobs=" + self.jobs,
-            "--manifest-path",
-            os.path.join(self.rust_root, "src/bootstrap/Cargo.toml"),
-            "-Zroot-dir=" + self.rust_root,
-        ]
-        # verbose cargo output is very noisy, so only enable it with -vv
-        args.extend("--verbose" for _ in range(self.verbose - 1))
-
         if "BOOTSTRAP_TRACING" in env:
             args.append("--features=tracing")
 

src/bootstrap/bootstrap_test.py

@@ -244,8 +244,10 @@ def test_warnings(self):
             if toml_warnings is not None:
                 configure_args = ["--set", "rust.deny-warnings=" + toml_warnings]
 
-            _, env = self.build_args(configure_args, args=["--warnings=warn"])
-            self.assertFalse("-Dwarnings" in env["RUSTFLAGS"])
+            args, env = self.build_args(configure_args, args=["--warnings=warn"])
+            self.assertFalse("CARGO_BUILD_WARNINGS" in env)
+            self.assertFalse("-Zwarnings" in args)
 
-            _, env = self.build_args(configure_args, args=["--warnings=deny"])
-            self.assertTrue("-Dwarnings" in env["RUSTFLAGS"])
+            args, env = self.build_args(configure_args, args=["--warnings=deny"])
+            self.assertEqual("deny", env["CARGO_BUILD_WARNINGS"])
+            self.assertTrue("-Zwarnings" in args)

src/bootstrap/src/core/builder/cargo.rs

@@ -459,6 +459,11 @@ impl Builder<'_> {
         let out_dir = self.stage_out(compiler, mode);
         cargo.env("CARGO_TARGET_DIR", &out_dir);
 
+        // Set this unconditionally. Cargo silently ignores `CARGO_BUILD_WARNINGS` when `-Z
+        // warnings` isn't present, which is hard to debug, and it's not worth the effort to keep
+        // them in sync.
+        cargo.arg("-Zwarnings");
+
         // Bootstrap makes a lot of assumptions about the artifacts produced in the target
         // directory. If users override the "build directory" using `build-dir`
         // (https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#build-dir), then
@@ -1159,8 +1164,10 @@ impl Builder<'_> {
             lint_flags.push("-Wunused_lifetimes");
 
             if self.config.deny_warnings {
-                lint_flags.push("-Dwarnings");
-                rustdocflags.arg("-Dwarnings");
+                // We use this instead of `lint_flags` so that we don't have to rebuild all
+                // workspace dependencies when `deny-warnings` changes, but we still get an error
+                // immediately instead of having to wait until the next rebuild.
+                cargo.env("CARGO_BUILD_WARNINGS", "deny");
             }
 
             rustdocflags.arg("-Wrustdoc::invalid_codeblock_attributes");