From aa2c18e4df926611226b2d128610c2e8bb639c9e Mon Sep 17 00:00:00 2001
From: Harmen Stoppels <me@harmenstoppels.nl>
Date: Wed, 20 Nov 2024 16:15:28 +0100
Subject: [PATCH] spack style: import-check -> import, fix bugs, exclude
 spack.pkg (#47690)

---
 lib/spack/spack/cmd/modules/lmod.py           |  1 +
 lib/spack/spack/cmd/modules/tcl.py            |  1 +
 lib/spack/spack/cmd/style.py                  | 29 +++++++++++++------
 lib/spack/spack/directives.py                 |  1 -
 lib/spack/spack/solver/asp.py                 |  1 +
 lib/spack/spack/test/cmd/ci.py                |  1 +
 lib/spack/spack/test/cmd/pkg.py               |  1 +
 lib/spack/spack/test/cmd/style.py             | 23 +++++++++++++--
 lib/spack/spack/test/config.py                |  1 +
 lib/spack/spack/test/modules/common.py        |  1 +
 lib/spack/spack/test/package_class.py         |  1 +
 lib/spack/spack/util/path.py                  |  1 +
 lib/spack/spack/util/web.py                   |  1 +
 share/spack/spack-completion.fish             |  4 +--
 .../packages/find-externals1/package.py       |  3 +-
 .../packages/py-test-callback/package.py      |  3 +-
 .../repos/builtin/packages/chapel/package.py  |  3 ++
 .../builtin/packages/cray-fftw/package.py     |  2 ++
 .../repos/builtin/packages/pythia6/package.py |  4 +--
 .../repos/builtin/packages/upcxx/package.py   |  2 ++
 20 files changed, 63 insertions(+), 21 deletions(-)

diff --git a/lib/spack/spack/cmd/modules/lmod.py b/lib/spack/spack/cmd/modules/lmod.py
index 4fd6992a47a..3f2f7d2ec04 100644
--- a/lib/spack/spack/cmd/modules/lmod.py
+++ b/lib/spack/spack/cmd/modules/lmod.py
@@ -8,6 +8,7 @@
 import spack.cmd.common.arguments
 import spack.cmd.modules
 import spack.config
+import spack.modules
 import spack.modules.lmod
 
 
diff --git a/lib/spack/spack/cmd/modules/tcl.py b/lib/spack/spack/cmd/modules/tcl.py
index 4ca8ece7044..e31d5bcc46f 100644
--- a/lib/spack/spack/cmd/modules/tcl.py
+++ b/lib/spack/spack/cmd/modules/tcl.py
@@ -7,6 +7,7 @@
 import spack.cmd.common.arguments
 import spack.cmd.modules
 import spack.config
+import spack.modules
 import spack.modules.tcl
 
 
diff --git a/lib/spack/spack/cmd/style.py b/lib/spack/spack/cmd/style.py
index 5925d1120e5..9d164875ae2 100644
--- a/lib/spack/spack/cmd/style.py
+++ b/lib/spack/spack/cmd/style.py
@@ -15,6 +15,7 @@
 from llnl.util.filesystem import working_dir
 
 import spack.paths
+import spack.repo
 import spack.util.git
 from spack.util.executable import Executable, which
 
@@ -38,7 +39,7 @@ def grouper(iterable, n, fillvalue=None):
 #: double-check the results of other tools (if, e.g., --fix was provided)
 #: The list maps an executable name to a method to ensure the tool is
 #: bootstrapped or present in the environment.
-tool_names = ["import-check", "isort", "black", "flake8", "mypy"]
+tool_names = ["import", "isort", "black", "flake8", "mypy"]
 
 #: warnings to ignore in mypy
 mypy_ignores = [
@@ -370,10 +371,19 @@ def run_black(black_cmd, file_list, args):
 
 def _module_part(root: str, expr: str):
     parts = expr.split(".")
+    # spack.pkg is for repositories, don't try to resolve it here.
+    if ".".join(parts[:2]) == spack.repo.ROOT_PYTHON_NAMESPACE:
+        return None
     while parts:
         f1 = os.path.join(root, "lib", "spack", *parts) + ".py"
         f2 = os.path.join(root, "lib", "spack", *parts, "__init__.py")
-        if os.path.exists(f1) or os.path.exists(f2):
+
+        if (
+            os.path.exists(f1)
+            # ensure case sensitive match
+            and f"{parts[-1]}.py" in os.listdir(os.path.dirname(f1))
+            or os.path.exists(f2)
+        ):
             return ".".join(parts)
         parts.pop()
     return None
@@ -389,7 +399,7 @@ def _run_import_check(
     out=sys.stdout,
 ):
     if sys.version_info < (3, 9):
-        print("import-check requires Python 3.9 or later")
+        print("import check requires Python 3.9 or later")
         return 0
 
     is_use = re.compile(r"(?<!from )(?<!import )(?:llnl|spack)\.[a-zA-Z0-9_\.]+")
@@ -431,10 +441,11 @@ def _run_import_check(
             module = _module_part(root, m.group(0))
             if not module or module in to_add:
                 continue
-            if f"import {module}" not in filtered_contents:
-                to_add.add(module)
-                exit_code = 1
-                print(f"{pretty_path}: missing import: {module}", file=out)
+            if re.search(rf"import {re.escape(module)}\b(?!\.)", contents):
+                continue
+            to_add.add(module)
+            exit_code = 1
+            print(f"{pretty_path}: missing import: {module} ({m.group(0)})", file=out)
 
         if not fix or not to_add and not to_remove:
             continue
@@ -465,7 +476,7 @@ def _run_import_check(
     return exit_code
 
 
-@tool("import-check", external=False)
+@tool("import", external=False)
 def run_import_check(import_check_cmd, file_list, args):
     exit_code = _run_import_check(
         file_list,
@@ -474,7 +485,7 @@ def run_import_check(import_check_cmd, file_list, args):
         root=args.root,
         working_dir=args.initial_working_dir,
     )
-    print_tool_result("import-check", exit_code)
+    print_tool_result("import", exit_code)
     return exit_code
 
 
diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py
index e8a89dc5a5d..44d95f38373 100644
--- a/lib/spack/spack/directives.py
+++ b/lib/spack/spack/directives.py
@@ -36,7 +36,6 @@ class OpenMpi(Package):
 import re
 from typing import Any, Callable, List, Optional, Tuple, Union
 
-import llnl.util.lang
 import llnl.util.tty.color
 
 import spack.deptypes as dt
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 4b35a093b82..a57e1183b43 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -27,6 +27,7 @@
 
 import spack
 import spack.binary_distribution
+import spack.compiler
 import spack.compilers
 import spack.concretize
 import spack.config
diff --git a/lib/spack/spack/test/cmd/ci.py b/lib/spack/spack/test/cmd/ci.py
index 36aa992c639..bc50ca683f7 100644
--- a/lib/spack/spack/test/cmd/ci.py
+++ b/lib/spack/spack/test/cmd/ci.py
@@ -17,6 +17,7 @@
 import spack
 import spack.binary_distribution
 import spack.ci as ci
+import spack.cmd
 import spack.cmd.ci
 import spack.environment as ev
 import spack.hash_types as ht
diff --git a/lib/spack/spack/test/cmd/pkg.py b/lib/spack/spack/test/cmd/pkg.py
index 8dfdf4773de..8fdc89b1fd7 100644
--- a/lib/spack/spack/test/cmd/pkg.py
+++ b/lib/spack/spack/test/cmd/pkg.py
@@ -10,6 +10,7 @@
 
 from llnl.util.filesystem import mkdirp, working_dir
 
+import spack.cmd
 import spack.cmd.pkg
 import spack.main
 import spack.paths
diff --git a/lib/spack/spack/test/cmd/style.py b/lib/spack/spack/test/cmd/style.py
index eab4de18dda..ee33de9b4b0 100644
--- a/lib/spack/spack/test/cmd/style.py
+++ b/lib/spack/spack/test/cmd/style.py
@@ -295,7 +295,7 @@ def test_style_with_black(flake8_package_with_errors):
 
 
 def test_skip_tools():
-    output = style("--skip", "import-check,isort,mypy,black,flake8")
+    output = style("--skip", "import,isort,mypy,black,flake8")
     assert "Nothing to run" in output
 
 
@@ -314,6 +314,7 @@ class Example(spack.build_systems.autotools.AutotoolsPackage):
 def foo(config: "spack.error.SpackError"):
     # the type hint is quoted, so it should not be removed
     spack.util.executable.Executable("example")
+    print(spack.__version__)
 '''
     file.write_text(contents)
     root = str(tmp_path)
@@ -330,6 +331,7 @@ def foo(config: "spack.error.SpackError"):
 
     assert "issues.py: redundant import: spack.cmd" in output
     assert "issues.py: redundant import: spack.config" not in output  # comment prevents removal
+    assert "issues.py: missing import: spack" in output  # used by spack.__version__
     assert "issues.py: missing import: spack.build_systems.autotools" in output
     assert "issues.py: missing import: spack.util.executable" in output
     assert "issues.py: missing import: spack.error" not in output  # not directly used
@@ -349,6 +351,7 @@ def foo(config: "spack.error.SpackError"):
     output = output_buf.getvalue()
     assert exit_code == 1
     assert "issues.py: redundant import: spack.cmd" in output
+    assert "issues.py: missing import: spack" in output
     assert "issues.py: missing import: spack.build_systems.autotools" in output
     assert "issues.py: missing import: spack.util.executable" in output
 
@@ -369,8 +372,9 @@ def foo(config: "spack.error.SpackError"):
     # check that the file was fixed
     new_contents = file.read_text()
     assert "import spack.cmd" not in new_contents
-    assert "import spack.build_systems.autotools" in new_contents
-    assert "import spack.util.executable" in new_contents
+    assert "import spack\n" in new_contents
+    assert "import spack.build_systems.autotools\n" in new_contents
+    assert "import spack.util.executable\n" in new_contents
 
 
 @pytest.mark.skipif(sys.version_info < (3, 9), reason="requires Python 3.9+")
@@ -389,3 +393,16 @@ def test_run_import_check_syntax_error_and_missing(tmp_path: pathlib.Path):
     assert "syntax-error.py: could not parse" in output
     assert "missing.py: could not parse" in output
     assert exit_code == 1
+
+
+def test_case_sensitive_imports(tmp_path: pathlib.Path):
+    # example.Example is a name, while example.example is a module.
+    (tmp_path / "lib" / "spack" / "example").mkdir(parents=True)
+    (tmp_path / "lib" / "spack" / "example" / "__init__.py").write_text("class Example:\n    pass")
+    (tmp_path / "lib" / "spack" / "example" / "example.py").write_text("foo = 1")
+    assert spack.cmd.style._module_part(str(tmp_path), "example.Example") == "example"
+
+
+def test_pkg_imports():
+    assert spack.cmd.style._module_part(spack.paths.prefix, "spack.pkg.builtin.boost") is None
+    assert spack.cmd.style._module_part(spack.paths.prefix, "spack.pkg") is None
diff --git a/lib/spack/spack/test/config.py b/lib/spack/spack/test/config.py
index a5a4fc9787c..52e1cc9e04a 100644
--- a/lib/spack/spack/test/config.py
+++ b/lib/spack/spack/test/config.py
@@ -15,6 +15,7 @@
 import llnl.util.tty as tty
 from llnl.util.filesystem import join_path, touch, touchp
 
+import spack
 import spack.config
 import spack.directory_layout
 import spack.environment as ev
diff --git a/lib/spack/spack/test/modules/common.py b/lib/spack/spack/test/modules/common.py
index 7190689f824..c7f63f64b6d 100644
--- a/lib/spack/spack/test/modules/common.py
+++ b/lib/spack/spack/test/modules/common.py
@@ -12,6 +12,7 @@
 import spack.cmd.modules
 import spack.config
 import spack.error
+import spack.modules
 import spack.modules.common
 import spack.modules.tcl
 import spack.package_base
diff --git a/lib/spack/spack/test/package_class.py b/lib/spack/spack/test/package_class.py
index a8c541930bd..72eaa1f739b 100644
--- a/lib/spack/spack/test/package_class.py
+++ b/lib/spack/spack/test/package_class.py
@@ -21,6 +21,7 @@
 import spack.deptypes as dt
 import spack.error
 import spack.install_test
+import spack.package
 import spack.package_base
 import spack.repo
 import spack.spec
diff --git a/lib/spack/spack/util/path.py b/lib/spack/spack/util/path.py
index 5e7fa3a797b..5967c595b4a 100644
--- a/lib/spack/spack/util/path.py
+++ b/lib/spack/spack/util/path.py
@@ -55,6 +55,7 @@ def get_user():
 # Substitutions to perform
 def replacements():
     # break circular imports
+    import spack
     import spack.environment as ev
     import spack.paths
 
diff --git a/lib/spack/spack/util/web.py b/lib/spack/spack/util/web.py
index 892b64d333d..71756292542 100644
--- a/lib/spack/spack/util/web.py
+++ b/lib/spack/spack/util/web.py
@@ -26,6 +26,7 @@
 from llnl.util import lang, tty
 from llnl.util.filesystem import mkdirp, rename, working_dir
 
+import spack
 import spack.config
 import spack.error
 import spack.util.executable
diff --git a/share/spack/spack-completion.fish b/share/spack/spack-completion.fish
index 6c8f2521b42..7d1e996c3f0 100644
--- a/share/spack/spack-completion.fish
+++ b/share/spack/spack-completion.fish
@@ -2908,9 +2908,9 @@ complete -c spack -n '__fish_spack_using_command style' -s f -l fix -d 'format a
 complete -c spack -n '__fish_spack_using_command style' -l root -r -f -a root
 complete -c spack -n '__fish_spack_using_command style' -l root -r -d 'style check a different spack instance'
 complete -c spack -n '__fish_spack_using_command style' -s t -l tool -r -f -a tool
-complete -c spack -n '__fish_spack_using_command style' -s t -l tool -r -d 'specify which tools to run (default: import-check, isort, black, flake8, mypy)'
+complete -c spack -n '__fish_spack_using_command style' -s t -l tool -r -d 'specify which tools to run (default: import, isort, black, flake8, mypy)'
 complete -c spack -n '__fish_spack_using_command style' -s s -l skip -r -f -a skip
-complete -c spack -n '__fish_spack_using_command style' -s s -l skip -r -d 'specify tools to skip (choose from import-check, isort, black, flake8, mypy)'
+complete -c spack -n '__fish_spack_using_command style' -s s -l skip -r -d 'specify tools to skip (choose from import, isort, black, flake8, mypy)'
 
 # spack tags
 set -g __fish_spack_optspecs_spack_tags h/help i/installed a/all
diff --git a/var/spack/repos/builtin.mock/packages/find-externals1/package.py b/var/spack/repos/builtin.mock/packages/find-externals1/package.py
index b8adbac19cd..d6ddb320f91 100644
--- a/var/spack/repos/builtin.mock/packages/find-externals1/package.py
+++ b/var/spack/repos/builtin.mock/packages/find-externals1/package.py
@@ -5,7 +5,6 @@
 import os
 import re
 
-import spack.package_base
 from spack.package import *
 
 
@@ -26,7 +25,7 @@ def determine_spec_details(cls, prefix, exes_in_prefix):
         exes = [x for x in exe_to_path.keys() if "find-externals1-exe" in x]
         if not exes:
             return
-        exe = spack.util.executable.Executable(exe_to_path[exes[0]])
+        exe = Executable(exe_to_path[exes[0]])
         output = exe("--version", output=str)
         if output:
             match = re.search(r"find-externals1.*version\s+(\S+)", output)
diff --git a/var/spack/repos/builtin.mock/packages/py-test-callback/package.py b/var/spack/repos/builtin.mock/packages/py-test-callback/package.py
index 4abad2f679a..74744e0f16f 100644
--- a/var/spack/repos/builtin.mock/packages/py-test-callback/package.py
+++ b/var/spack/repos/builtin.mock/packages/py-test-callback/package.py
@@ -3,6 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
+import spack.builder
 import spack.pkg.builtin.mock.python as mp
 from spack.build_systems._checks import BuilderWithDefaults, execute_install_time_tests
 from spack.package import *
@@ -40,7 +41,7 @@ class MyBuilder(BuilderWithDefaults):
     def install(self, pkg, spec, prefix):
         pkg.install(spec, prefix)
 
-    spack.phase_callbacks.run_after("install")(execute_install_time_tests)
+    run_after("install")(execute_install_time_tests)
 
     def test_callback(self):
         self.pkg.test_callback()
diff --git a/var/spack/repos/builtin/packages/chapel/package.py b/var/spack/repos/builtin/packages/chapel/package.py
index 21f19a0c00e..3e8693c787b 100644
--- a/var/spack/repos/builtin/packages/chapel/package.py
+++ b/var/spack/repos/builtin/packages/chapel/package.py
@@ -6,6 +6,9 @@
 import os
 import subprocess
 
+import llnl.util.lang
+
+import spack.platforms
 import spack.platforms.cray
 from spack.package import *
 from spack.util.environment import is_system_path, set_env
diff --git a/var/spack/repos/builtin/packages/cray-fftw/package.py b/var/spack/repos/builtin/packages/cray-fftw/package.py
index 13526ecc618..4035ad39736 100644
--- a/var/spack/repos/builtin/packages/cray-fftw/package.py
+++ b/var/spack/repos/builtin/packages/cray-fftw/package.py
@@ -3,6 +3,8 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
+import llnl.util.lang
+
 from spack.package import *
 
 
diff --git a/var/spack/repos/builtin/packages/pythia6/package.py b/var/spack/repos/builtin/packages/pythia6/package.py
index f28bf5c970d..a027a19c570 100644
--- a/var/spack/repos/builtin/packages/pythia6/package.py
+++ b/var/spack/repos/builtin/packages/pythia6/package.py
@@ -138,9 +138,7 @@ def patch(self):
         # Use our provided CMakeLists.txt. The Makefile provided with
         # the source is GCC (gfortran) specific, and would have required
         # additional patching for the +root variant.
-        llnl.util.filesystem.copy(
-            os.path.join(os.path.dirname(__file__), "CMakeLists.txt"), self.stage.source_path
-        )
+        copy(os.path.join(os.path.dirname(__file__), "CMakeLists.txt"), self.stage.source_path)
         # Apply the variant value at the relevant place in the source.
         filter_file(
             r"^(\s+PARAMETER\s*\(\s*NMXHEP\s*=\s*)\d+",
diff --git a/var/spack/repos/builtin/packages/upcxx/package.py b/var/spack/repos/builtin/packages/upcxx/package.py
index 141060a9d58..6fdcaf6a46e 100644
--- a/var/spack/repos/builtin/packages/upcxx/package.py
+++ b/var/spack/repos/builtin/packages/upcxx/package.py
@@ -6,6 +6,8 @@
 import os
 import re
 
+import llnl.util.lang
+
 import spack.platforms
 from spack.package import *