import-check: improve how problematic imports are displayed (#48825)
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.
This commit is contained in:
parent
12280f864c
commit
763b35a2e0
23
.github/workflows/valid-style.yml
vendored
23
.github/workflows/valid-style.yml
vendored
@ -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:
|
||||
|
Loading…
Reference in New Issue
Block a user