archcheck: block new catch-all crate names (common, utils, core, ...)

.github/workflows/rust-static-ci.yml

@@ -168,11 +168,14 @@ jobs:
       # Pull requests only: archcheck writes its findings as rdjson, offline
       # from the clippy step's caches, even when clippy failed. Exit 1 means
       # required findings, which reviewdog decides on; anything else fails.
-      # no_new_entries and no_raised_caps compare with the baseline of the
-      # base: the first parent of the synthetic merge (fetch-depth 0 above),
-      # once resolve-migration-base.sh has checked the second is this pull
-      # request. If the base has no baseline, an empty one; if it can't be
-      # resolved, a warning and no comparison.
+      # The base is the first parent of the synthetic merge (fetch-depth 0
+      # above), once resolve-migration-base.sh has checked the second is this
+      # pull request. no_new_entries, no_raised_caps and crate_names.no_new
+      # compare with its architecture-baseline.toml, and
+      # crate_names.policy_changed with its ARCHITECTURE.toml; an empty file
+      # stands in for one the base lacks. If the base can't be resolved, a
+      # warning: the first two and policy_changed are skipped, and no_new
+      # allows only the crates listed when it was added.
       - name: Check repository structure
         id: archcheck
         if: >-
@@ -192,19 +195,33 @@ jobs:
             --env CARGO_TARGET_DIR=target/buildkite-lint --env PULL_REQUEST_HEAD "space-ci:${GITHUB_SHA}" bash -c '
               sccache --start-server >/dev/null 2>&1 && export RUSTC_WRAPPER=sccache
               mkdir -p target && git config --global --add safe.directory /workspace
-              base_file=target/archcheck-base-baseline.toml base_args=()
-              if base="$(bash ci/scripts/resolve-migration-base.sh "" "${PULL_REQUEST_HEAD}")"; then
-                if ! git cat-file -e "${base}:architecture-baseline.toml" 2>/dev/null; then
-                  echo "archcheck: ${base} has no architecture-baseline.toml; comparing with an empty one"
-                  : >"${base_file}" && base_args=(--base-baseline "${base_file}")
-                elif git show "${base}:architecture-baseline.toml" >"${base_file}"; then
-                  echo "archcheck: comparing with architecture-baseline.toml from ${base}"
-                  base_args=(--base-baseline "${base_file}")
+              base_args=() missing=()
+              # Passes the base copy of a file, or an empty file if the base has none.
+              base_copy() {
+                local file="target/archcheck-base-$2"
+                if ! git cat-file -e "${base}:$2" 2>/dev/null; then
+                  echo "archcheck: ${base} has no $2; comparing with an empty one"
+                  : >"${file}" && base_args+=("--$1" "${file}")
+                elif git show "${base}:$2" >"${file}"; then
+                  echo "archcheck: comparing with $2 from ${base}"
+                  base_args+=("--$1" "${file}")
+                else
+                  missing+=("$2")
                 fi
+              }
+              if base="$(bash ci/scripts/resolve-migration-base.sh "" "${PULL_REQUEST_HEAD}")"; then
+                base_copy base-baseline architecture-baseline.toml
+                base_copy base-architecture ARCHITECTURE.toml
+              else
+                missing=(architecture-baseline.toml ARCHITECTURE.toml)
               fi
-              if ((${#base_args[@]} == 0)); then
-                echo "::warning title=archcheck::Could not resolve the base baseline; no_new_entries and no_raised_caps are skipped."
-              fi
+              for file in "${missing[@]}"; do
+                case "${file}" in
+                  architecture-baseline.toml) skipped="no_new_entries and no_raised_caps are skipped, and crate_names.no_new allows only the crates listed when it was added" ;;
+                  ARCHITECTURE.toml) skipped="crate_names.policy_changed is skipped" ;;
+                esac
+                echo "::warning title=archcheck::Could not read the base ${file}; ${skipped}."
+              done
               exec cargo arch --format rdjson "${base_args[@]}" >target/archcheck.rdjson' || status=$?
           if ((status > 1)); then echo "::error title=archcheck::cargo arch exited ${status}; see above."; exit 1; fi
 

ARCHITECTURE.toml

@@ -77,3 +77,18 @@
 
 [rules.pattern_ratchet.rootfs_copy.stale_entries]
 mode = "required"
+
+[rules.crate_names]
+# A crate matches if a part of its package or library name, lowercased and
+# split on `-` and `_`, is one of these.
+banned_tokens = ["common", "shared", "util", "utils", "helper", "helpers", "misc", "core"]
+allow = []                            # exact package names exempt from the rule
+
+[rules.crate_names.no_new]            # no new catch-all crate, and no new baseline entries
+mode = "required"
+
+[rules.crate_names.stale_entries]     # entries and allow names that no longer apply
+mode = "advisory"
+
+[rules.crate_names.policy_changed]    # changes to this table, compared with the base branch
+mode = "advisory"

architecture-baseline.toml

@@ -139,3 +139,23 @@
 "crates/libs/template/mgr/src/service.rs" = 3
 "crates/libs/vol/mgr/server/src/staging.rs" = 2
 
+# [crate_names] maps each workspace package with a catch-all name (a part such
+# as `common`, `utils` or `core`) to its member path. Only remove entries: a pull
+# request that adds one is reported, and fails the check.
+[crate_names]
+pplx-space-common-cli = "crates/libs/common/cli"
+pplx-space-common-connection-admission = "crates/libs/common/connection/admission"
+pplx-space-common-core = "crates/libs/common/core"
+pplx-space-common-grpc = "crates/libs/common/grpc"
+pplx-space-common-grpc-lb = "crates/libs/common/grpc-lb"
+pplx-space-common-io = "crates/libs/common/io"
+pplx-space-common-jobs = "crates/libs/common/jobs"
+pplx-space-common-lru-reaper = "crates/libs/common/lru-reaper"
+pplx-space-common-proto-build = "crates/libs/common/proto-build"
+pplx-space-common-svc = "crates/libs/common/svc"
+pplx-space-common-work-coalescer = "crates/libs/common/work-coalescer"
+pplx-space-credential-injection-core = "crates/libs/credential/injection/core"
+pplx-space-execution-common = "crates/libs/execution/common"
+pplx-space-net-common = "crates/libs/net/common"
+pplx-space-node-runtime-core = "crates/libs/node/runtime/core"
+pplx-space-spaced-common = "crates/libs/spaced/common"

ci/scripts/order-archcheck-findings.sh

@@ -16,16 +16,23 @@
 # this a pull request's own findings can be pushed out by the tree's.
 #
 # The pull request's own findings, in this order, each in report order:
-# - no_new_entries and no_raised_caps findings. archcheck reports these only
-#   for entries that differ from the base branch's copy of
-#   architecture-baseline.toml, by table and key, so they include the
-#   entries of a renamed or split table.
+# - Findings that compare with the base branch: no_new_entries and
+#   no_raised_caps, which archcheck reports only for entries that differ from
+#   the base's copy of architecture-baseline.toml, by table and key, so they
+#   include the entries of a renamed or split table; and
+#   crate_names.policy_changed, which it reports only for differences from the
+#   base's ARCHITECTURE.toml.
 # - Findings in a file the pull request changes, under its old or new name.
-#   archcheck's only other findings in architecture-baseline.toml are stale
-#   entries for files that are gone, so one there is the pull request's when
-#   its entry names a file it changes. An entry's key may be indented and a
-#   basic or literal string; a basic string with an escape JSON lacks (\e,
-#   \U) names no file.
+#   archcheck's other findings in architecture-baseline.toml are stale
+#   entries, and crate_names.no_new findings on [crate_names] entries. One
+#   there is the pull request's when its entry names a file it changes: the
+#   key of a file entry, or, in [crate_names], the Cargo.toml of the member
+#   path the entry maps to. A key or path may be indented and a basic or
+#   literal string; a basic string with an escape JSON lacks (\e, \U) names
+#   no file.
+#
+# Required (ERROR) findings are ordered too, but the workflow annotates them
+# from archcheck's own report, in a run of their own that fails the job.
 #
 # reviewdog's -filter-mode=file keeps findings in files whose diff has a
 # +++ line: those with changes, under their new name. A pure rename, a mode
@@ -53,14 +60,26 @@ jq --arg baseline "${baseline}" --arg own "${own}" --arg q "'" \
     [$pages[][]] as $listed
     | ([$listed[] | .filename, .previous_filename // empty] | INDEX(.)) as $files
     | ([$listed[] | select(.changes > 0) | .filename] | INDEX(.)) as $diffed
-    | def entry_path:
-        capture("^\\s*(?:\"(?<basic>(?:[^\"\\\\]|\\\\.)*)\"|\($q)(?<literal>[^\($q)]*)\($q))\\s*=")
-        | if .basic != null then (try ("\"" + .basic + "\"" | fromjson) catch null) else .literal end;
-      ([$head_baseline | split("\n") | to_entries[]
-          | select([.value | entry_path | select(. != null and $files[.] != null)] | length > 0)
-          | .key + 1 | tostring]
+    | def string_value:
+        if .basic != null then (try ("\"" + .basic + "\"" | fromjson) catch null) else .literal end;
+      def quoted: "(?:\"(?<basic>(?:[^\"\\\\]|\\\\.)*)\"|\($q)(?<literal>[^\($q)]*)\($q))";
+      def entry_path: capture("^\\s*\(quoted)\\s*=") | string_value;
+      # The Cargo.toml of the member path a [crate_names] entry maps to.
+      def member_manifest:
+        capture("^[^=]+=\\s*\(quoted)\\s*(?:#.*)?$") | string_value
+        | select(. != null) | if . == "" then "Cargo.toml" else . + "/Cargo.toml" end;
+      def table_header: capture("^\\s*\\[\\s*(?<name>[^\\]\\s]+)\\s*\\]\\s*(?:#.*)?$").name;
+      ([foreach ($head_baseline | split("\n") | to_entries[]) as $line (null;
+          first($line.value | table_header) // .;
+          {table: ., number: ($line.key + 1), text: $line.value})
+          | select(.table as $table
+              | [.text | if $table == "crate_names" then member_manifest else entry_path end
+                  | select(. != null and $files[.] != null)]
+              | length > 0)
+          | .number | tostring]
         | INDEX(.)) as $named_lines
-    | def compares_with_base: (.code.value // "") | test("\\.(no_new_entries|no_raised_caps)$");
+    | def compares_with_base:
+        (.code.value // "") | test("\\.(no_new_entries|no_raised_caps)$|^crate_names\\.policy_changed$");
       def own:
         compares_with_base
         or if .location.path == $baseline then $named_lines[.location.range.start.line // 0 | tostring] != null

ci/scripts/test-order-archcheck-findings.sh

@@ -1,8 +1,9 @@
 #!/usr/bin/env bash
 # Regression test for ci/scripts/order-archcheck-findings.sh: a pull request's
-# own findings come first, those comparing with the base's baseline leading,
-# each group in report order; a stale baseline entry is its own when it names
-# a changed file; and --own counts only what reviewdog shows.
+# own findings come first, those comparing with the base's baseline or
+# ARCHITECTURE.toml leading, each group in report order; a stale baseline
+# entry is its own when it names a changed file, or a [crate_names] entry the
+# changed Cargo.toml of its member; and --own counts only what reviewdog shows.
 set -euo pipefail
 
 SCRIPT="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/order-archcheck-findings.sh"
@@ -177,6 +178,45 @@ expect "file list" "new name.rs:1 ${B}:2 second.rs:1 third.rs:1 $(numbered tree
 expect "pure rename" "moved.rs:1 edited.rs:1 $(numbered tree 11)" moved.rdjson moved.json
 expect "pure rename, file mode" "edited.rs:1" --own=file moved.rdjson moved.json
 
+# crate_names.policy_changed compares ARCHITECTURE.toml with the base's, so
+# it leads with the other base comparisons; a required no_new finding on a new
+# crate's Cargo.toml is the pull request's, and kept.
+printf '[file_size]\n' >"${B}"
+{
+    tree_findings
+    diagnostic own.rs 1 file_size.listed_no_growth
+    diagnostic crates/new-utils/Cargo.toml 2 crate_names.no_new ERROR
+    diagnostic ARCHITECTURE.toml 80 crate_names.policy_changed
+    diagnostic ARCHITECTURE.toml 81 crate_names.stale_entries
+} | report >policy.rdjson
+pages ARCHITECTURE.toml own.rs crates/new-utils/Cargo.toml >policy.json
+expect "crate_names policy" \
+    "ARCHITECTURE.toml:80 own.rs:1 crates/new-utils/Cargo.toml:2 ARCHITECTURE.toml:81" \
+    --own=nofilter policy.rdjson policy.json
+
+# A stale [crate_names] entry is the pull request's when it changes the
+# Cargo.toml of the member path the entry maps to (old or new name), however
+# the key and path are written; an empty path is the root package.
+cat >"${B}" <<'EOF'
+[file_size]
+"crates/z/src/lib.rs" = 1300
+
+[crate_names]
+pplx-space-x-common = "crates/x"
+"pplx-space-y-utils" = 'crates/y'  # moved
+pplx-space-z-core = "crates/z"
+pplx-space-root-misc = ""
+EOF
+{
+    tree_findings
+    for line in 5 6 7 8; do diagnostic "${B}" "${line}" crate_names.stale_entries; done
+} | report >crates.rdjson
+{
+    printf '[{"filename": "crates/y2/Cargo.toml", "previous_filename": "crates/y/Cargo.toml", "status": "renamed", "changes": 0}]'
+    pages crates/x/Cargo.toml Cargo.toml crates/z/src/lib.rs
+} >crates.json
+expect "crate_names entries" "${B}:5 ${B}:6 ${B}:8 $(numbered tree 11) ${B}:7" crates.rdjson crates.json
+
 if bash "${SCRIPT}" source.rdjson 2>/dev/null; then
     fail "the script accepted a missing <pull-request-files> argument"
 fi