Package sh breaks --import-mode=importlib

[sbacchio]

Hi, I have track down an issue involving the package sh and the option --import-mode=importlib.
When package sh is loaded, an importer is added to sys.meta_path:

>>> import sys
>>> sys.meta_path
[<class '_frozen_importlib.BuiltinImporter'>, <class '_frozen_importlib.FrozenImporter'>, <class '_frozen_importlib_external.PathFinder'>]
>>> import sh
>>> sys.meta_path
[<sh.ModuleImporterFromVariables object at 0x7f567a9e4a10>, <class '_frozen_importlib.BuiltinImporter'>, <class '_frozen_importlib.FrozenImporter'>, <class '_frozen_importlib_external.PathFinder'>]
>>> 

This breaks the importing system of pytest at this line, since the new importer does not have the method find_spec.
I get, indeed, the following error:

../../../anaconda3/lib/python3.7/site-packages/_pytest/pathlib.py:483: in import_path
    spec = meta_importer.find_spec(module_name, [str(path.parent)])
E   AttributeError: 'ModuleImporterFromVariables' object has no attribute 'find_spec'

Should this issue be resolved from pytest side, or from sh side?

[sbacchio]

The necessary changes in sh (amoffat/sh#536) have been made and this issue should be resolved with the next release of the module.

I understand that find_module (e.g. this was used in sh) has been deprecated in favor of find_spec, but maybe pytest should support both in order to avoid similar issues?

I could open a PR with the following changes:

-             spec = meta_importer.find_spec(module_name, [str(path.parent)])
+             try:
+                 spec = meta_importer.find_spec(module_name, [str(path.parent)])
+             except AttributeError:
+                 found = meta_importer.find_module(module_name, [str(path.parent)])
+                 spec = ModuleSpec(module_name, found) if found is not None else None

with from importlib.machinery import ModuleSpec in the imports

[nicoddemus]

Thanks @sbacchio for the report!

I understand that find_module (e.g. this was used in sh) has been deprecated in favor of find_spec, but maybe pytest should support both in order to avoid similar issues?

While the proposed suggestion is simple, it incurs some maintenance overhead because in the future we will need to also remove this code (with breaking release possibly), and in general fixing this "upstream" in the affected packages is more beneficial overall.

For this reason I would rather wait and see if this affects more modules: so far it doesn't seem to be the case, don't know if because people don't use --importmode=importlib much or there's no problems.

Again thanks for the report and patching up sh. 👍

Closing this for now.