Skip to content

py/builtinimport: Allow overriding of mp_builtin___import__. #9026

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

py/builtinimport: Allow overriding of mp_builtin___import__. #9026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions py/builtin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,13 @@ MP_DECLARE_CONST_FUN_OBJ_KW(mp_builtin_open_obj);

#endif

// A port can provide its own import handler by defining mp_builtin___import__.
#ifndef mp_builtin___import__
#define mp_builtin___import__ mp_builtin___import___default
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use a static inline definition here, it's more robust than a #define

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and rebased. It does increase the code size by a few bytes (+4 on minimal embedded port).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! So the reason is because of this line:

MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(mp_builtin___import___obj, 1, 5, mp_builtin___import__);

which forces both the default and static-inline wrapper function to be linked in to the firmware (instead of just the default one).

I guess we need to go back to using the #define to make it efficient...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I was wondering about that. I reverted it and rebased.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to go back and forth on this.... We don't really have a consistent way to override an arbitrary function in the core, but there is some precedence set by py/mphal.h. Following the way it does things, we could do the following here:

#ifndef mp_builtin___import__
#define mp_builtin___import__ mp_builtin___import___default
#endif

That would be pretty clean, and doesn't require an entry in py/mpconfig.h. It's also how ports/stm32/boardctrl.h does things.

Then in your port you do:

mp_obj_t custom_import(size_t n_args, const mp_obj_t *args);
#define mp_builtin___import__ custom_import

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's one of the first approaches I tried, but I got a bit stuck on where to do this exactly.

I'm able to make this work following the same mphal approach (the ifndef in py/mphal.h and custom_import in mphalport.h), but I figured this one doesn't belong there since it doesn't concern hardware.

It seems like the custom implementation would be provided via mpconfigport.h, but I'm not sure how since the declaration relies on mp_obj_t which isn't defined at that point.

What would be the right places to put the suggested snippets above?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should go here in py/builtin.h.

How about this:

#ifndef mp_builtin___import__
#define mp_builtin___import__ mp_builtin___import___default
#endif
mp_obj_t mp_builtin___import__(size_t n_args, const mp_obj_t *args);
mp_obj_t mp_builtin___import__default(size_t n_args, const mp_obj_t *args);

?

It's a bit tricky but should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, that works! Updated and rebased.

#endif
mp_obj_t mp_builtin___import__(size_t n_args, const mp_obj_t *args);
mp_obj_t mp_builtin___import___default(size_t n_args, const mp_obj_t *args);

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);
Expand Down
4 changes: 2 additions & 2 deletions py/builtinimport.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ STATIC mp_obj_t process_import_at_level(qstr full_mod_name, qstr level_mod_name,
return module_obj;
}

mp_obj_t mp_builtin___import__(size_t n_args, const mp_obj_t *args) {
mp_obj_t mp_builtin___import___default(size_t n_args, const mp_obj_t *args) {
#if DEBUG_PRINT
DEBUG_printf("__import__:\n");
for (size_t i = 0; i < n_args; i++) {
Expand Down Expand Up @@ -566,7 +566,7 @@ mp_obj_t mp_builtin___import__(size_t n_args, const mp_obj_t *args) {

#else // MICROPY_ENABLE_EXTERNAL_IMPORT

mp_obj_t mp_builtin___import__(size_t n_args, const mp_obj_t *args) {
mp_obj_t mp_builtin___import___default(size_t n_args, const mp_obj_t *args) {
// Check that it's not a relative import
if (n_args >= 5 && MP_OBJ_SMALL_INT_VALUE(args[4]) != 0) {
mp_raise_NotImplementedError(MP_ERROR_TEXT("relative import"));
Expand Down