Codecov Report

Merging #9026 (c0ac815) into master (9dfabcd) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #9026   +/-   ##
=======================================
  Coverage   98.37%   98.37%           
=======================================
  Files         156      156           
  Lines       20281    20281           
=======================================
  Hits        19951    19951           
  Misses        330      330           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

This is useful for MicroPython applications without enough resources to have MICROPY_ENABLE_EXTERNAL_IMPORT enabled, but which still want to allow users to do some application-specific imports beyond builtins.

If a port wants to provide "custom" imports the idea is for it to enable MICROPY_ENABLE_EXTERNAL_IMPORT and then provide the desired functionality via a custom mp_import_stat(). It doesn't need to have a filesystem.

But it looks like you actually want more sophisticated import behaviour, to support built-in packages. #5701 intended to do this but was closed. Maybe it needs to be revisited? Or even the patch you link above could be merged here?

I'm not against being able to override mp_builtin___import__(), if it makes sense. But I think it should be done a bit differently to match mp_builtin_open() in py/builtin.h.

If a port wants to provide "custom" imports the idea is for it to enable MICROPY_ENABLE_EXTERNAL_IMPORT and then provide the desired functionality via a custom mp_import_stat(). It doesn't need to have a filesystem.

Our main resource constraint is build size. On the minimal port, MICROPY_ENABLE_EXTERNAL_IMPORT requires about 1372 bytes. We don't use most of it, so it is relatively expensive to add customizations that way (≈2%).

But it looks like you actually want more sophisticated import behaviour, to support built-in packages.

The current PR is motivated by adding another (future) customization inspired by #8381, so that's why I started looking for a more generic way to customize imports. If adding that gets us closer in terms of build size, maybe we'll just enable external imports though.

#5701 intended to [support built-in packages] but was closed. Maybe it needs to be revisited? Or even the patch you link above could be merged here?

The patch we used served us well over the past years but I wasn't sure if it was generic enough to be useful in MicroPython. If you think it is, I'd be happy to make a pull request. This particular patch might not actually be needed once we drop external imports from all of our ports (it's still enabled on some) and just register a module with a dot in the name.

I'm not against being able to override mp_builtin___import__(), if it makes sense. But I think it should be done a bit differently to match mp_builtin_open() in py/builtin.h.

Thanks, I'll have a look at how that works.

Another option would be to add a hook that gets called just prior to raising the ImportError. Ports can then fill in the gap as needed. It's probably only useful if MICROPY_ENABLE_EXTERNAL_IMPORT is disabled but it could be added in both places in principle.

Like this, for example:

diff --git a/py/builtin.h b/py/builtin.h
index a6f824ca2..398470bef 100644
--- a/py/builtin.h
+++ b/py/builtin.h
@@ -65,6 +65,13 @@ MP_DECLARE_CONST_FUN_OBJ_KW(mp_builtin_open_obj);
 #endif
 
 mp_obj_t mp_builtin___import__(size_t n_args, const mp_obj_t *args);
+
+// A port can extend mp_builtin___import__ to handle cases where builtin import
+// can't find the requested module.
+#if MICROPY_MODULE_BUILTIN_IMPORT_EXTRA
+mp_obj_t mp_builtin_import_extra(size_t n_args, const mp_obj_t *args);
+#endif
+
 mp_obj_t mp_micropython_mem_info(size_t n_args, const mp_obj_t *args);
 
 MP_DECLARE_CONST_FUN_OBJ_VAR(mp_builtin___build_class___obj);
diff --git a/py/builtinimport.c b/py/builtinimport.c
index cd9636ccd..c6d601144 100644
--- a/py/builtinimport.c
+++ b/py/builtinimport.c
@@ -591,6 +591,14 @@ mp_obj_t mp_builtin___import__(size_t n_args, const mp_obj_t *args) {
     }
     #endif
 
+    #if MICROPY_MODULE_BUILTIN_IMPORT_EXTRA
+    // Call port-specific import extension.
+    module_obj = mp_builtin_import_extra(n_args, args);
+    if (module_obj != MP_OBJ_NULL) {
+        return module_obj;
+    }
+    #endif
+
     // Couldn't find the module, so fail
     #if MICROPY_ERROR_REPORTING <= MICROPY_ERROR_REPORTING_TERSE
     mp_raise_msg(&mp_type_ImportError, MP_ERROR_TEXT("module not found"));
diff --git a/py/mpconfig.h b/py/mpconfig.h
index d70d39ae9..7a20b47c9 100644
--- a/py/mpconfig.h
+++ b/py/mpconfig.h
@@ -834,6 +834,12 @@ typedef double mp_float_t;
 #define MICROPY_MODULE_BUILTIN_INIT (MICROPY_CONFIG_ROM_LEVEL_AT_LEAST_EXTRA_FEATURES)
 #endif
 
+// Hook to run port-specific import handler if standard builtin import fails.
+// This is only used if MICROPY_ENABLE_EXTERNAL_IMPORT is disabled.
+#ifndef MICROPY_MODULE_BUILTIN_IMPORT_EXTRA
+#define MICROPY_MODULE_BUILTIN_IMPORT_EXTRA (0)
+#endif
+
 // Whether to support module-level __getattr__ (see PEP 562)
 #ifndef MICROPY_MODULE_GETATTR
 #define MICROPY_MODULE_GETATTR (MICROPY_CONFIG_ROM_LEVEL_AT_LEAST_CORE_FEATURES)

Another option would be to add a hook that gets called just prior to raising the ImportError. Ports can then fill in the gap as needed.

If done this way then for it to be general there would need to be a hook at the start and end of the import function. I think best to allow the port to override mp_builtin___import__(), but also for it to have access to the default one to call if it wants.

Eg have a mp_builtin___import___default() which is aliased to mp_builtin___import__() unless a board overrides the latter.

I think best to allow the port to override mp_builtin___import__(), but also for it to have access to the default one to call if it wants. (...) Eg have a mp_builtin___import___default() which is aliased to mp_builtin___import__() unless a board overrides the latter.

If I'm understanding this right, this is now done with the updated version. Just let me know if you intended something else.

This approach works fine for our use case as well.

Being more general, it can now be used even if external imports are enabled (even if that isn't all that useful).

but also for it to have access to the default one to call if it wants

One minor downside to this approach (compared to the hook approach mentioned above) is that if you just want to extend the default behavior, you have to catch the exception raised by the default handler before you can proceed.

But this is not much of a downside if external imports are disabled (which is the main use case anyway), because then mp_builtin___import___default is small enough that replicating it in full is not really an issue.

I think the approach here is fine, it should be rare that a port wants to override this import behaviour.

Thanks for updating. Merged in d8ad878