Auto merge of #109133 - weihanglo:make-cargo-a-workspace, r=ehuss

Make cargo a workspace

8 commits in 7bf43f028ba5eb1f4d70d271c2546c38512c9875..39116ccc9b420a883a98a960f0597f9cf87414b8
2023-04-10 16:01:41 +0000 to 2023-04-15 20:24:15 +0000

- Make cargo a workspace (rust-lang/cargo#11851)
- Fix flaky not_found_permutations test. (rust-lang/cargo#11976)
- Use restricted Damerau-Levenshtein algorithm (rust-lang/cargo#11963)
- Correct the bug report for `cargo clippy --fix` (rust-lang/cargo#11882)
- Stabilize `cargo logout` (rust-lang/cargo#11950)
- Add more information to HTTP errors to help with debugging. (rust-lang/cargo#11878)
- Use registry.default for login/logout (rust-lang/cargo#11949)
- Change -C to be unstable (rust-lang/cargo#11960)

---

### What does this PR try to resolve?

Making cargo a workspace.

Why doing this?

* `rustc-workspace-hack` is primarily for sharing dependencies between rls and cargo, as rls previously depends on cargo. After rls retired, it is no longer the case sharing dependencies.
* It's q bit painful that cargo needs to deal with some dependency and licensing complexities. For example, #108665 failed because of the interaction bewteen `windows-sys` and `raw-dylib`. It currenctly blocks cargo's feature `-Zgitxodie` from moving forward.
* See rust-lang/cargo#11851

### Benchmark result

I've done a simple benchmark on both keeping or removing entire `rustc-workspace-hack`. It had no significant difference. Both took ~2m30s to finish `./x.py build -j8 src/tools/cargo src/tools/rls src/tools/clippy src/tools/miri src/tools/rustfmt`. Environment info:

```
host: aarch64-apple-darwin
os: Mac OS 13.2.1 [64-bit]
```

A sophisticated benchmark may be needed.

### Additional information

This depends on prior works from `@Muscraft` and `@ehuss.` Credits to them!

Cargo.toml

@@ -22,12 +22,6 @@ members = [
   "src/tools/remote-test-server",
   "src/tools/rust-installer",
   "src/tools/rust-demangler",
-  "src/tools/cargo",
-  "src/tools/cargo/crates/credential/cargo-credential-1password",
-  "src/tools/cargo/crates/credential/cargo-credential-macos-keychain",
-  "src/tools/cargo/crates/credential/cargo-credential-wincred",
-  "src/tools/cargo/crates/mdman",
-  # "src/tools/cargo/crates/resolver-tests",
   "src/tools/rustdoc",
   "src/tools/rls",
   "src/tools/rustfmt",
@@ -106,10 +100,6 @@ miniz_oxide.debug = 0
 object.debug = 0
 
 [patch.crates-io]
-# See comments in `src/tools/rustc-workspace-hack/README.md` for what's going on
-# here
-rustc-workspace-hack = { path = 'src/tools/rustc-workspace-hack' }
-
 # See comments in `library/rustc-std-workspace-core/README.md` for what's going on
 # here
 rustc-std-workspace-core = { path = 'library/rustc-std-workspace-core' }

src/bootstrap/bootstrap.py

@@ -822,7 +822,8 @@ def check_vendored_status(self):
         if self.use_vendored_sources:
             vendor_dir = os.path.join(self.rust_root, 'vendor')
             if not os.path.exists(vendor_dir):
-                sync_dirs = "--sync ./src/tools/rust-analyzer/Cargo.toml " \
+                sync_dirs = "--sync ./src/tools/cargo/Cargo.toml " \
+                            "--sync ./src/tools/rust-analyzer/Cargo.toml " \
                             "--sync ./compiler/rustc_codegen_cranelift/Cargo.toml " \
                             "--sync ./src/bootstrap/Cargo.toml "
                 print('error: vendoring required, but vendor directory does not exist.')

src/bootstrap/dist.rs

@@ -996,11 +996,14 @@ impl Step for PlainSourceTarball {
         // If we're building from git sources, we need to vendor a complete distribution.
         if builder.rust_info().is_managed_git_subrepository() {
             // Ensure we have the submodules checked out.
+            builder.update_submodule(Path::new("src/tools/cargo"));
             builder.update_submodule(Path::new("src/tools/rust-analyzer"));
 
             // Vendor all Cargo dependencies
             let mut cmd = Command::new(&builder.initial_cargo);
             cmd.arg("vendor")
+                .arg("--sync")
+                .arg(builder.src.join("./src/tools/cargo/Cargo.toml"))
                 .arg("--sync")
                 .arg(builder.src.join("./src/tools/rust-analyzer/Cargo.toml"))
                 .arg("--sync")

src/bootstrap/lib.rs

@@ -238,8 +238,6 @@ pub struct Build {
     ci_env: CiEnv,
     delayed_failures: RefCell<Vec<String>>,
     prerelease_version: Cell<Option<u32>>,
-    tool_artifacts:
-        RefCell<HashMap<TargetSelection, HashMap<String, (&'static str, PathBuf, Vec<String>)>>>,
 
     #[cfg(feature = "build-metrics")]
     metrics: metrics::BuildMetrics,
@@ -458,7 +456,6 @@ impl Build {
             ci_env: CiEnv::current(),
             delayed_failures: RefCell::new(Vec::new()),
             prerelease_version: Cell::new(None),
-            tool_artifacts: Default::default(),
 
             #[cfg(feature = "build-metrics")]
             metrics: metrics::BuildMetrics::init(),

src/bootstrap/metadata.rs

@@ -7,38 +7,35 @@ use crate::cache::INTERNER;
 use crate::util::output;
 use crate::{Build, Crate};
 
-#[derive(Deserialize)]
+/// For more information, see the output of
+/// <https://doc.rust-lang.org/nightly/cargo/commands/cargo-metadata.html>
+#[derive(Debug, Deserialize)]
 struct Output {
     packages: Vec<Package>,
 }
 
-#[derive(Deserialize)]
+/// For more information, see the output of
+/// <https://doc.rust-lang.org/nightly/cargo/commands/cargo-metadata.html>
+#[derive(Debug, Deserialize)]
 struct Package {
     name: String,
     source: Option<String>,
     manifest_path: String,
     dependencies: Vec<Dependency>,
 }
 
-#[derive(Deserialize)]
+/// For more information, see the output of
+/// <https://doc.rust-lang.org/nightly/cargo/commands/cargo-metadata.html>
+#[derive(Debug, Deserialize)]
 struct Dependency {
     name: String,
     source: Option<String>,
 }
 
+/// Collects and stores package metadata of each workspace members into `build`,
+/// by executing `cargo metadata` commands.
 pub fn build(build: &mut Build) {
-    // Run `cargo metadata` to figure out what crates we're testing.
-    let mut cargo = Command::new(&build.initial_cargo);
-    cargo
-        .arg("metadata")
-        .arg("--format-version")
-        .arg("1")
-        .arg("--no-deps")
-        .arg("--manifest-path")
-        .arg(build.src.join("Cargo.toml"));
-    let output = output(&mut cargo);
-    let output: Output = serde_json::from_str(&output).unwrap();
-    for package in output.packages {
+    for package in workspace_members(build) {
         if package.source.is_none() {
             let name = INTERNER.intern_string(package.name);
             let mut path = PathBuf::from(package.manifest_path);
@@ -57,3 +54,35 @@ pub fn build(build: &mut Build) {
         }
     }
 }
+
+/// Invokes `cargo metadata` to get package metadata of each workspace member.
+///
+/// Note that `src/tools/cargo` is no longer a workspace member but we still
+/// treat it as one here, by invoking an additional `cargo metadata` command.
+fn workspace_members(build: &Build) -> impl Iterator<Item = Package> {
+    let cmd_metadata = |manifest_path| {
+        let mut cargo = Command::new(&build.initial_cargo);
+        cargo
+            .arg("metadata")
+            .arg("--format-version")
+            .arg("1")
+            .arg("--no-deps")
+            .arg("--manifest-path")
+            .arg(manifest_path);
+        cargo
+    };
+
+    // Collects `metadata.packages` from the root workspace.
+    let root_manifest_path = build.src.join("Cargo.toml");
+    let root_output = output(&mut cmd_metadata(&root_manifest_path));
+    let Output { packages, .. } = serde_json::from_str(&root_output).unwrap();
+
+    // Collects `metadata.packages` from src/tools/cargo separately.
+    let cargo_manifest_path = build.src.join("src/tools/cargo/Cargo.toml");
+    let cargo_output = output(&mut cmd_metadata(&cargo_manifest_path));
+    let Output { packages: cargo_packages, .. } = serde_json::from_str(&cargo_output).unwrap();
+
+    // We only care about the root package from `src/tool/cargo` workspace.
+    let cargo_package = cargo_packages.into_iter().find(|pkg| pkg.name == "cargo").into_iter();
+    packages.into_iter().chain(cargo_package)
+}

src/bootstrap/tool.rs

@@ -1,4 +1,3 @@
-use std::collections::HashSet;
 use std::env;
 use std::fs;
 use std::path::PathBuf;
@@ -120,135 +119,9 @@ impl Step for ToolBuild {
             &self.target,
         );
         builder.info(&msg);
-        let mut duplicates = Vec::new();
-        let is_expected = compile::stream_cargo(builder, cargo, vec![], &mut |msg| {
-            // Only care about big things like the RLS/Cargo for now
-            match tool {
-                "rls" | "cargo" | "clippy-driver" | "miri" | "rustfmt" => {}
 
-                _ => return,
-            }
-            let (id, features, filenames) = match msg {
-                compile::CargoMessage::CompilerArtifact {
-                    package_id,
-                    features,
-                    filenames,
-                    target: _,
-                } => (package_id, features, filenames),
-                _ => return,
-            };
-            let features = features.iter().map(|s| s.to_string()).collect::<Vec<_>>();
-
-            for path in filenames {
-                let val = (tool, PathBuf::from(&*path), features.clone());
-                // we're only interested in deduplicating rlibs for now
-                if val.1.extension().and_then(|s| s.to_str()) != Some("rlib") {
-                    continue;
-                }
-
-                // Don't worry about compiles that turn out to be host
-                // dependencies or build scripts. To skip these we look for
-                // anything that goes in `.../release/deps` but *doesn't* go in
-                // `$target/release/deps`. This ensure that outputs in
-                // `$target/release` are still considered candidates for
-                // deduplication.
-                if let Some(parent) = val.1.parent() {
-                    if parent.ends_with("release/deps") {
-                        let maybe_target = parent
-                            .parent()
-                            .and_then(|p| p.parent())
-                            .and_then(|p| p.file_name())
-                            .and_then(|p| p.to_str())
-                            .unwrap();
-                        if maybe_target != &*target.triple {
-                            continue;
-                        }
-                    }
-                }
-
-                // Record that we've built an artifact for `id`, and if one was
-                // already listed then we need to see if we reused the same
-                // artifact or produced a duplicate.
-                let mut artifacts = builder.tool_artifacts.borrow_mut();
-                let prev_artifacts = artifacts.entry(target).or_default();
-                let prev = match prev_artifacts.get(&*id) {
-                    Some(prev) => prev,
-                    None => {
-                        prev_artifacts.insert(id.to_string(), val);
-                        continue;
-                    }
-                };
-                if prev.1 == val.1 {
-                    return; // same path, same artifact
-                }
-
-                // If the paths are different and one of them *isn't* inside of
-                // `release/deps`, then it means it's probably in
-                // `$target/release`, or it's some final artifact like
-                // `libcargo.rlib`. In these situations Cargo probably just
-                // copied it up from `$target/release/deps/libcargo-xxxx.rlib`,
-                // so if the features are equal we can just skip it.
-                let prev_no_hash = prev.1.parent().unwrap().ends_with("release/deps");
-                let val_no_hash = val.1.parent().unwrap().ends_with("release/deps");
-                if prev.2 == val.2 || !prev_no_hash || !val_no_hash {
-                    return;
-                }
-
-                // ... and otherwise this looks like we duplicated some sort of
-                // compilation, so record it to generate an error later.
-                duplicates.push((id.to_string(), val, prev.clone()));
-            }
-        });
-
-        if is_expected && !duplicates.is_empty() {
-            eprintln!(
-                "duplicate artifacts found when compiling a tool, this \
-                      typically means that something was recompiled because \
-                      a transitive dependency has different features activated \
-                      than in a previous build:\n"
-            );
-            let (same, different): (Vec<_>, Vec<_>) =
-                duplicates.into_iter().partition(|(_, cur, prev)| cur.2 == prev.2);
-            if !same.is_empty() {
-                eprintln!(
-                    "the following dependencies are duplicated although they \
-                      have the same features enabled:"
-                );
-                for (id, cur, prev) in same {
-                    eprintln!("  {}", id);
-                    // same features
-                    eprintln!("    `{}` ({:?})\n    `{}` ({:?})", cur.0, cur.1, prev.0, prev.1);
-                }
-            }
-            if !different.is_empty() {
-                eprintln!("the following dependencies have different features:");
-                for (id, cur, prev) in different {
-                    eprintln!("  {}", id);
-                    let cur_features: HashSet<_> = cur.2.into_iter().collect();
-                    let prev_features: HashSet<_> = prev.2.into_iter().collect();
-                    eprintln!(
-                        "    `{}` additionally enabled features {:?} at {:?}",
-                        cur.0,
-                        &cur_features - &prev_features,
-                        cur.1
-                    );
-                    eprintln!(
-                        "    `{}` additionally enabled features {:?} at {:?}",
-                        prev.0,
-                        &prev_features - &cur_features,
-                        prev.1
-                    );
-                }
-            }
-            eprintln!();
-            eprintln!(
-                "to fix this you will probably want to edit the local \
-                      src/tools/rustc-workspace-hack/Cargo.toml crate, as \
-                      that will update the dependency graph to ensure that \
-                      these crates all share the same feature set"
-            );
-            panic!("tools should not compile multiple copies of the same crate");
-        }
+        let mut cargo = Command::from(cargo);
+        let is_expected = builder.try_run_quiet(&mut cargo);
 
         builder.save_toolstate(
             tool,
@@ -299,7 +172,9 @@ pub fn prepare_tool_cargo(
             || path.ends_with("rustfmt")
         {
             cargo.env("LIBZ_SYS_STATIC", "1");
-            features.push("rustc-workspace-hack/all-static".to_string());
+        }
+        if path.ends_with("cargo") {
+            features.push("all-static".to_string());
         }
     }
 

src/tools/rls/Cargo.toml

@@ -7,7 +7,3 @@ license = "Apache-2.0/MIT"
 [dependencies]
 serde = { version = "1.0.143", features = ["derive"] }
 serde_json = "1.0.83"
-# A noop dependency that changes in the Rust repository, it's a bit of a hack.
-# See the `src/tools/rustc-workspace-hack/README.md` file in `rust-lang/rust`
-# for more information.
-rustc-workspace-hack = "1.0.0"