spack style: import-check -> import, fix bugs, exclude spack.pkg (#47690)

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
 
 

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
 
 

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:
+            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}", file=out)
+            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
 
 

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

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

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

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

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

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

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

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

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
 

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

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

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)

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()

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

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 *
 
 

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+",

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 *