Invalid soname for known_wheel_files in editable mode

[beenje]

We noticed an issue on one of our CI runner when building our project in editable mode. Importing the module fails. I tracked down the issue to the known_wheel_files dict passed to install in the _pytango_editable.py file imported in _pytango_editable.pth: {'tango._tango': 'tango/_tango.so.10.1.0.0'} (see log)

$ python -c 'import sys;print(sys.path)'
['', '/opt/conda/lib/python313.zip', '/opt/conda/lib/python3.13', '/opt/conda/lib/python3.13/lib-dynload', '/opt/conda/lib/python3.13/site-packages', '/builds/tango-controls/pytango']
$ python -c 'import tango;print(tango.utils.info())'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
    import tango;print(tango.utils.info())
    ^^^^^^^^^^^^
  File "/builds/tango-controls/pytango/tango/__init__.py", line 247, in <module>
    from ._tango import (
    ...<179 lines>...
    )
ModuleNotFoundError: No module named 'tango._tango'

When working, we have: {'tango._tango': 'tango/_tango.so'}(log)

In both cases, we have the same files under site-packages/tango:

$ ls -la /opt/conda/lib/python3.13/site-packages/tango
total 77804
drwxr-xr-x   2 root root     4096 Sep 11 13:09 .
drwxr-xr-x 127 root root     4096 Sep 11 13:09 ..
-rw-r--r--   1 root root 26550824 Sep 11 13:09 _tango.so
-rw-r--r--   1 root root 26550824 Sep 11 13:09 _tango.so.10
-rw-r--r--   1 root root 26550824 Sep 11 13:09 _tango.so.10.1.0.0

My understanding is that _tango.so.10.1.0.0 isn't a valid PEP 3149-style filename as it doesn't end with .so, meaning that _tango isn't recognised as a loadable extension.

scikit-build-core should always use the .so file.

[henryiii]

It's probably iterating in a random order and the "last" one wins. I'm guessing it's an issue with our filtering of filenames.

[LecrisUT]

The core of the issue is that however you defined _tango target, it is not a format that is well recognized, e.g. python_add_library , pybind11 helpers etc. will tag the libraries with the appropriate python tags indicating what python interpreter it was built for and would handle removing the SOVERSION etc.

Could give some more context on how _tango library is to be used? Does it have only python bindings or is it supposed to be linked to by other applications as well? If the second part is not applicable, the SOVERSION et. al. can be removed

[henryiii]

We need to refactor this a bit, we are stripping all extensions to make the module list, but we should handle a.py and a.txt in the same directory. We might need two dicts instead, or we can strip the importlib.machinery.*_SUFFIXES later.

For your package, A python extension module library must be shared is incorrect. It must be a MODULE, not SHARED. You are incorrectly computing a filename, too, as the SOABI is missing. Since you are using pybind11, I'd recommend using pybind11's pybind11_add_library, it does the right thing for you. If not, you can use python_add_module from FindPython, which also does the right thing. https://gitlab.com/tango-controls/pytango/-/blob/e31422e3733a546f62fbdca8d7203d0f9e795ab7/CMakeLists.txt#L144 is incorrect, as is the lines below setting the extension without adding SOABI. All of this is handled correctly by the pybind11 or FindPython helpers.

[henryiii]

Dropping the patch I started here:

diff --git a/src/scikit_build_core/build/_pathutil.py b/src/scikit_build_core/build/_pathutil.py
index 3bf020e..733d338 100644
--- a/src/scikit_build_core/build/_pathutil.py
+++ b/src/scikit_build_core/build/_pathutil.py
@@ -1,6 +1,7 @@
 from __future__ import annotations

 import os
+import importlib.machinery
 from pathlib import Path
 from typing import TYPE_CHECKING

@@ -14,6 +15,12 @@ if TYPE_CHECKING:
 __all__ = ["is_valid_module", "packages_to_file_mapping", "path_to_module", "scantree"]


+ALL_SUFFIXES = {
+   *importlib.machinery.SOURCE_SUFFIXES,
+   *importlib.machinery.BYTECODE_SUFFIXES,
+   *importlib.machinery.EXTENSION_SUFFIXES,
+}
+
 def __dir__() -> list[str]:
     return __all__

@@ -70,7 +77,9 @@ def packages_to_file_mapping(

 def is_valid_module(path: Path) -> bool:
     parts = path.parts
+    name, _, ext = path.name.partition(".")
     return (
         all(p.isidentifier() for p in parts[:-1])
-        and parts[-1].split(".", 1)[0].isidentifier()
+        and name.isidentifier()
+        and f".{ext}" in ALL_SUFFIXES
     )
diff --git a/tests/test_editable_unit.py b/tests/test_editable_unit.py
index ba15f9d..04e0981 100644
--- a/tests/test_editable_unit.py
+++ b/tests/test_editable_unit.py
@@ -12,6 +12,7 @@ from scikit_build_core.build._editable import (
     editable_redirect,
     libdir_to_installed,
     mapping_to_modules,
+    is_valid_module,
 )
 from scikit_build_core.build._pathutil import packages_to_file_mapping

@@ -203,3 +204,16 @@ def test_navigate_editable_pkg(editable_package: EditablePackage, virtualenv: VE
     if sys.version_info >= (3, 9):
         virtualenv.execute("import pkg.src_files")
         virtualenv.execute("import pkg.installed_files")
+
+
+def test_is_valid_module():
+    assert is_valid_module(Path("one.py"))
+    assert is_valid_module(Path("one/two.py"))
+    assert is_valid_module(Path("one/two.pyc"))
+    assert not is_valid_module(Path("one/two.py.other"))
+    assert not is_valid_module(Path("1one/two.py"))
+    assert not is_valid_module(Path("one/2two.py"))
+    assert not is_valid_module(Path("one/two"))
+    assert not is_valid_module(Path("one/.py"))
+    assert not is_valid_module(Path(".py"))

It will need a larger refactor though to handle data files correctly, so it will probably have to wait till after my trip to India tomorrow.

[LecrisUT]

I thought we don't copy .pyc files, instead we want to compile and put the artifact instead. Otherwise I remember there are 2 distinct handlings of extenstion_suffix and source_suffix, which on second thought could be merged.

[beenje]

Thanks a lot for the recommendation @henryiii!

For the context, we moved from boost to pybind11 recently.

SHARE was indeed incorrect. I changed that to MODULE and also use pybind11_add_module. We now only get one file, like _tango.cpython-313-darwin.so which is what we want.