From 763b35a2e0f526469cbd9f5fb17f869031964c12 Mon Sep 17 00:00:00 2001
From: Harmen Stoppels <me@harmenstoppels.nl>
Date: Mon, 3 Feb 2025 05:56:38 +0100
Subject: [PATCH] import-check: improve how problematic imports are displayed
 (#48825)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The import-check action now presents problematic import statements
introduced by the PR better.

The idea is roughly:

* Let (V₁, E₁) be the graph of modules as vertices and import statements
  as edges before the change
* Let (V₂, E₂) be the graph after the code change, which is typically a small
  perturbation of (V₁, E₁).
* X₁ = FAS(V₁, E₁) is the feedback arc set before (a minimal set of edges to
  delete to make it acyclic)
* X₂ = FAS(V₂, E₂ ∖ X₁) is the feedback arc set after deletion of the minimal
  set of edges that made the old graph acyclic.
* X₃ = FAS(V₂, E₂) is the feedback arc set after

Previously I displayed X₁ and X₃ and users had to diff themselves.

Now, I'm showing X₂, which is a small set, typically directly related to
code changes.

However, it can be that a small code change adding say 2 problematic imports
creates a completely different solution X₃ that only requires deletion of just 1
different import. In that case the user is informed that they can potentially do
less work.

So for PR #48784 the output is now:

> The overall number of problematic import statements increased by 1 from 31 to 32.
> This is likely a direct consequence of the following import statements:
>
> ```
> spack/config imports: spack.spec, spack.util.path, spack.util.remote_file_cache
> ```
>
> However, instead of removing 3 import statements, it is sufficient to remove only 1
> import statement from the following list:
>
> ```
> spack/concretize imports: spack.bootstrap, spack.solver.asp
> spack/environment imports: spack.bootstrap, spack.environment
> spack/fetch_strategy imports: spack.version.git_ref_lookup
> spack/install_test imports: spack.build_environment, spack.package_base
> spack/modules imports: spack.modules
> spack/platforms imports: spack.config
> spack/relocate imports: spack.bootstrap
> spack/repo imports: spack.package_base, spack.patch, spack.tag
> spack/spec imports: spack.binary_distribution, spack.compiler, spack.compilers, spack.concretize, spack.environment, spack.hash_types, spack.provider_index, spack.repo, spack.spec_parser, spack.store, spack.traverse, spack.variant, spack.version.git_ref_lookup
> spack/subprocess_context imports: spack.environment
> spack/util/gpg imports: spack.bootstrap
> spack/util/package_hash imports: spack.package_base
> spack/util/path imports: spack.config, spack.environment
> spack/util/remote_file_cache imports: spack.util.web
> ```

from which the user can figure out that
`spack/util/remote_file_cache imports: spack.util.web` is the "bottleneck" now.
---
 .github/workflows/valid-style.yml | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/.github/workflows/valid-style.yml b/.github/workflows/valid-style.yml
index 6b85f5d0c67..152b8823da1 100644
--- a/.github/workflows/valid-style.yml
+++ b/.github/workflows/valid-style.yml
@@ -121,31 +121,14 @@ jobs:
       uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683
       with:
         repository: haampie/circular-import-fighter
-        ref: b5d6ce9be35f602cca7d5a6aa0259fca10639cca
+        ref: 9a2c728c97f594dec0210e7f85cb7167eaf29176
         path: circular-import-fighter
     - name: Install dependencies
       working-directory: circular-import-fighter
       run: make -j dependencies
-    - name: Problematic imports before
+    - name: Circular import check
       working-directory: circular-import-fighter
-      run: make SPACK_ROOT=../old SUFFIX=.old
-    - name: Problematic imports after
-      working-directory: circular-import-fighter
-      run: make SPACK_ROOT=../new SUFFIX=.new
-    - name: Compare import cycles
-      working-directory: circular-import-fighter
-      run: |
-        edges_before="$(head -n1 solution.old)"
-        edges_after="$(head -n1 solution.new)"
-        if [ "$edges_after" -gt "$edges_before" ]; then
-          printf '\033[1;31mImport check failed: %s imports need to be deleted, ' "$edges_after"
-          printf 'previously this was %s\033[0m\n'  "$edges_before"
-          printf 'Compare \033[1;97m"Problematic imports before"\033[0m and '
-          printf '\033[1;97m"Problematic imports after"\033[0m.\n'
-          exit 1
-        else
-          printf '\033[1;32mImport check passed: %s <= %s\033[0m\n' "$edges_after" "$edges_before"
-        fi
+      run: make -j compare "SPACK_ROOT=../old ../new"
 
   # Further style checks from pylint
   pylint: