Auto merge of #144244 - jieyouxu:pr-full-ci, r=Kobzol

Enforce that PR CI jobs are a subset of Auto CI jobs modulo carve-outs

### Background

Currently, it is possible for a PR with red PR-only CI to pass Auto CI, then all subsequent PR CI runs will be red until that is fixed, even in completely unrelated PRs. For instance, this happened with PR-CI-only Spellcheck (#144183).

See more discussions at [#t-infra > Spellcheck workflow now fails on all PRs (tree bad?)](https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra/topic/Spellcheck.20workflow.20now.20fails.20on.20all.20PRs.20.28tree.20bad.3F.29/with/529769404).

### CI invariant: PR CI jobs are a subset of Auto CI jobs modulo carve-outs

To prevent red PR CI in completely unrelated subsequent PRs and PR CI runs, we need to maintain an invariant that **PR CI jobs are a subset of Auto CI jobs modulo carve-outs**.

This is **not** a "strict" subset relationship: some jobs necessarily have to differ under PR CI and Auto CI environments, at least in the current setup. Still, we can try to enforce a weaker "subset modulo carve-outs" relationship between CI jobs and their corresponding Auto jobs. For instance:

- `x86_64-gnu-tools` will have `auto`-only env vars like `DEPLOY_TOOLSTATES_JSON: toolstates-linux.json`.
- `tidy` will want to `continue_on_error: true` in PR CI to allow for more "useful" compilation errors to also be reported, whereas it should be `continue_on_error: false` in Auto CI to prevent wasting Auto CI resources.

The **carve-outs** are:

1. `env` variables.
2. `continue_on_error`.

We enforce this invariant through `citool`, so only affects job definitions that are handled by `citool`. Notably, this is not sufficient *alone* to address the CI-only Spellcheck issue (#144183). To carry out this enforcement, we modify `citool` to auto-register PR jobs as Auto jobs with `continue_on_error` overridden to `false` **unless** there's an overriding Auto job for the PR job of the same name that only differs by the permitted **carve-outs**.

### Addressing the Spellcheck PR-only CI issue

Note that Spellcheck currently does not go through `citool` or `bootstrap`, and is its own GitHub Actions workflow. To actually address the PR-CI-only Spellcheck issue (#144183), and carry out the subset-modulo-carve-outs enforcement universally, this PR additionally **removes the current Spellcheck implementation** (a separate GitHub Actions Workflow). That is incompatible with Homu unless we do some hacks in the main CI workflow.

This effectively partially reverts #134006 (the separate workflow part, not the tidy extra checks component), but is not prejudice against relanding the `typos`-based spellcheck in another implementation that goes through the usual bootstrap CI workflow so that it does work with Homu. The `typos`-based spellcheck seems to have a good false-positive rate.

Closes #144183.

---

r? infra-ci

Author: bors

.github/workflows/spellcheck.yml

@@ -1,9 +1,9 @@
 #[cfg(test)]
 mod tests;
 
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, HashSet};
 
-use anyhow::Context as _;
+use anyhow::{Context as _, anyhow};
 use serde_yaml::Value;
 
 use crate::GitHubContext;
@@ -85,6 +85,10 @@ impl JobDatabase {
             .cloned()
             .collect()
     }
+
+    fn find_auto_job_by_name(&self, job_name: &str) -> Option<&Job> {
+        self.auto_jobs.iter().find(|job| job.name == job_name)
+    }
 }
 
 pub fn load_job_db(db: &str) -> anyhow::Result<JobDatabase> {
@@ -97,14 +101,118 @@ pub fn load_job_db(db: &str) -> anyhow::Result<JobDatabase> {
         db.apply_merge().context("failed to apply merge keys")
     };
 
-    // Apply merge twice to handle nested merges
+    // Apply merge twice to handle nested merges up to depth 2.
     apply_merge(&mut db)?;
     apply_merge(&mut db)?;
 
-    let db: JobDatabase = serde_yaml::from_value(db).context("failed to parse job database")?;
+    let mut db: JobDatabase = serde_yaml::from_value(db).context("failed to parse job database")?;
+
+    register_pr_jobs_as_auto_jobs(&mut db)?;
+
+    validate_job_database(&db)?;
+
     Ok(db)
 }
 
+/// Maintain invariant that PR CI jobs must be a subset of Auto CI jobs modulo carve-outs.
+///
+/// When PR jobs are auto-registered as Auto jobs, they will have `continue_on_error` overridden to
+/// be `false` to avoid wasting Auto CI resources.
+///
+/// When a job is already both a PR job and a auto job, we will post-validate their "equivalence
+/// modulo certain carve-outs" in [`validate_job_database`].
+///
+/// This invariant is important to make sure that it's not easily possible (without modifying
+/// `citool`) to have PRs with red PR-only CI jobs merged into `master`, causing all subsequent PR
+/// CI runs to be red until the cause is fixed.
+fn register_pr_jobs_as_auto_jobs(db: &mut JobDatabase) -> anyhow::Result<()> {
+    for pr_job in &db.pr_jobs {
+        // It's acceptable to "override" a PR job in Auto job, for instance, `x86_64-gnu-tools` will
+        // receive an additional `DEPLOY_TOOLSTATES_JSON: toolstates-linux.json` env when under Auto
+        // environment versus PR environment.
+        if db.find_auto_job_by_name(&pr_job.name).is_some() {
+            continue;
+        }
+
+        let auto_registered_job = Job { continue_on_error: Some(false), ..pr_job.clone() };
+        db.auto_jobs.push(auto_registered_job);
+    }
+
+    Ok(())
+}
+
+fn validate_job_database(db: &JobDatabase) -> anyhow::Result<()> {
+    fn ensure_no_duplicate_job_names(section: &str, jobs: &Vec<Job>) -> anyhow::Result<()> {
+        let mut job_names = HashSet::new();
+        for job in jobs {
+            let job_name = job.name.as_str();
+            if !job_names.insert(job_name) {
+                return Err(anyhow::anyhow!(
+                    "duplicate job name `{job_name}` in section `{section}`"
+                ));
+            }
+        }
+        Ok(())
+    }
+
+    ensure_no_duplicate_job_names("pr", &db.pr_jobs)?;
+    ensure_no_duplicate_job_names("auto", &db.auto_jobs)?;
+    ensure_no_duplicate_job_names("try", &db.try_jobs)?;
+    ensure_no_duplicate_job_names("optional", &db.optional_jobs)?;
+
+    fn equivalent_modulo_carve_out(pr_job: &Job, auto_job: &Job) -> anyhow::Result<()> {
+        let Job {
+            name,
+            os,
+            only_on_channel,
+            free_disk,
+            doc_url,
+            codebuild,
+
+            // Carve-out configs allowed to be different.
+            env: _,
+            continue_on_error: _,
+        } = pr_job;
+
+        if *name == auto_job.name
+            && *os == auto_job.os
+            && *only_on_channel == auto_job.only_on_channel
+            && *free_disk == auto_job.free_disk
+            && *doc_url == auto_job.doc_url
+            && *codebuild == auto_job.codebuild
+        {
+            Ok(())
+        } else {
+            Err(anyhow!(
+                "PR job `{}` differs from corresponding Auto job `{}` in configuration other than `continue_on_error` and `env`",
+                pr_job.name,
+                auto_job.name
+            ))
+        }
+    }
+
+    for pr_job in &db.pr_jobs {
+        // At this point, any PR job must also be an Auto job, auto-registered or overridden.
+        let auto_job = db
+            .find_auto_job_by_name(&pr_job.name)
+            .expect("PR job must either be auto-registered as Auto job or overridden");
+
+        equivalent_modulo_carve_out(pr_job, auto_job)?;
+    }
+
+    // Auto CI jobs must all "fail-fast" to avoid wasting Auto CI resources. For instance, `tidy`.
+    for auto_job in &db.auto_jobs {
+        if auto_job.continue_on_error == Some(true) {
+            return Err(anyhow!(
+                "Auto job `{}` cannot have `continue_on_error: true`",
+                auto_job.name
+            ));
+        }
+    }
+
+    Ok(())
+}
+
 /// Representation of a job outputted to a GitHub Actions workflow.
 #[derive(serde::Serialize, Debug)]
 struct GithubActionsJob {

src/ci/citool/src/jobs.rs

@@ -1,3 +1,4 @@
+use std::collections::BTreeMap;
 use std::path::Path;
 
 use super::Job;
@@ -146,3 +147,222 @@ fn validate_jobs() {
         panic!("Job validation failed:\n{error_messages}");
     }
 }
+
+#[test]
+fn pr_job_implies_auto_job() {
+    let db = load_job_db(
+        r#"
+envs:
+  pr:
+  try:
+  auto:
+  optional:
+
+pr:
+    - name: pr-ci-a
+      os: ubuntu
+      env: {}
+try:
+auto:
+optional:
+"#,
+    )
+    .unwrap();
+
+    assert_eq!(db.auto_jobs.iter().map(|j| j.name.as_str()).collect::<Vec<_>>(), vec!["pr-ci-a"])
+}
+
+#[test]
+fn implied_auto_job_keeps_env_and_fails_fast() {
+    let db = load_job_db(
+        r#"
+envs:
+  pr:
+  try:
+  auto:
+  optional:
+
+pr:
+    - name: tidy
+      env:
+        DEPLOY_TOOLSTATES_JSON: toolstates-linux.json
+      continue_on_error: true
+      os: ubuntu
+try:
+auto:
+optional:
+"#,
+    )
+    .unwrap();
+
+    assert_eq!(db.auto_jobs.iter().map(|j| j.name.as_str()).collect::<Vec<_>>(), vec!["tidy"]);
+    assert_eq!(db.auto_jobs[0].continue_on_error, Some(false));
+    assert_eq!(
+        db.auto_jobs[0].env,
+        BTreeMap::from([(
+            "DEPLOY_TOOLSTATES_JSON".to_string(),
+            serde_yaml::Value::String("toolstates-linux.json".to_string())
+        )])
+    );
+}
+
+#[test]
+#[should_panic = "duplicate"]
+fn duplicate_job_name() {
+    let _ = load_job_db(
+        r#"
+envs:
+  pr:
+  try:
+  auto:
+
+
+pr:
+    - name: pr-ci-a
+      os: ubuntu
+      env: {}
+    - name: pr-ci-a
+      os: ubuntu
+      env: {}
+try:
+auto:
+optional:
+"#,
+    )
+    .unwrap();
+}
+
+#[test]
+fn auto_job_can_override_pr_job_spec() {
+    let db = load_job_db(
+        r#"
+envs:
+  pr:
+  try:
+  auto:
+  optional:
+
+pr:
+    - name: tidy
+      os: ubuntu
+      env: {}
+try:
+auto:
+    - name: tidy
+      env:
+        DEPLOY_TOOLSTATES_JSON: toolstates-linux.json
+      continue_on_error: false
+      os: ubuntu
+optional:
+"#,
+    )
+    .unwrap();
+
+    assert_eq!(db.auto_jobs.iter().map(|j| j.name.as_str()).collect::<Vec<_>>(), vec!["tidy"]);
+    assert_eq!(db.auto_jobs[0].continue_on_error, Some(false));
+    assert_eq!(
+        db.auto_jobs[0].env,
+        BTreeMap::from([(
+            "DEPLOY_TOOLSTATES_JSON".to_string(),
+            serde_yaml::Value::String("toolstates-linux.json".to_string())
+        )])
+    );
+}
+
+#[test]
+fn compatible_divergence_pr_auto_job() {
+    let db = load_job_db(
+        r#"
+envs:
+  pr:
+  try:
+  auto:
+  optional:
+
+pr:
+    - name: tidy
+      continue_on_error: true
+      env:
+        ENV_ALLOWED_TO_DIFFER: "hello world"
+      os: ubuntu
+try:
+auto:
+    - name: tidy
+      continue_on_error: false
+      env:
+        ENV_ALLOWED_TO_DIFFER: "goodbye world"
+      os: ubuntu
+optional:
+"#,
+    )
+    .unwrap();
+
+    // `continue_on_error` and `env` are carve-outs *allowed* to diverge between PR and Auto job of
+    // the same name. Should load successfully.
+
+    assert_eq!(db.auto_jobs.iter().map(|j| j.name.as_str()).collect::<Vec<_>>(), vec!["tidy"]);
+    assert_eq!(db.auto_jobs[0].continue_on_error, Some(false));
+    assert_eq!(
+        db.auto_jobs[0].env,
+        BTreeMap::from([(
+            "ENV_ALLOWED_TO_DIFFER".to_string(),
+            serde_yaml::Value::String("goodbye world".to_string())
+        )])
+    );
+}
+
+#[test]
+#[should_panic = "differs"]
+fn incompatible_divergence_pr_auto_job() {
+    // `os` is not one of the carve-out options allowed to diverge. This should fail.
+    let _ = load_job_db(
+        r#"
+envs:
+  pr:
+  try:
+  auto:
+  optional:
+
+pr:
+    - name: tidy
+      continue_on_error: true
+      env:
+        ENV_ALLOWED_TO_DIFFER: "hello world"
+      os: ubuntu
+try:
+auto:
+    - name: tidy
+      continue_on_error: false
+      env:
+        ENV_ALLOWED_TO_DIFFER: "goodbye world"
+      os: windows
+optional:
+"#,
+    )
+    .unwrap();
+}
+
+#[test]
+#[should_panic = "cannot have `continue_on_error: true`"]
+fn auto_job_continue_on_error() {
+    // Auto CI jobs must fail-fast.
+    let _ = load_job_db(
+        r#"
+envs:
+  pr:
+  try:
+  auto:
+  optional:
+
+pr:
+try:
+auto:
+    - name: tidy
+      continue_on_error: true
+      os: windows
+      env: {}
+optional:
+"#,
+    )
+    .unwrap();
+}