Re: [Buildroot] [PATCH] package/micropython: make use of recent micropython-lib

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Abilio, All,

On 2024-01-28 10:37 -0800, Abilio Marques spake thusly:
> Until now, micropython-lib was a package that installed v1.9.3,
> which is more than 6 years old. This was acceptable since micropython
> never made any other official release of the library until v1.20.
> 
> Meanwhile, the libraries underwent a reorganization, and they are now
> available in a directory structure that cannot be copied directly into
> the target. This is my theory on why v1.9.3 is still present in the
> current day buildroot (which comes with micropython v1.22).
> 
> This commit introduces an auxiliary script to collect those libraries
> and reorder them into a structure that can then be copied into
> /usr/lib/micropython. The script utilizes a module from the tools
> directory of the micropython repo.
> 
> As part of the changes made by the micropython project, the libraries
> are now released together with the interpreter. They are cloned as a
> submodule into the lib/micropython-lib directory.

Thank you for the detailed explanations! 👍

> These are the 2 main reasons why the old buildroot micropython-lib
> package is being removed.
> 
> With this commit, micropython-lib is installed (optionally) as part
> of micropython. The original config variable name was retained for
> backwards compatibility.

Minor mitpick about the phrasing:

    ... The original config variable name was retained as it fits with
    the micropython package 'namespace", and thus this is backward
    compatible and no legacy handling is needed.

> This commit also ensures that the libraries in micropython-lib will
> be updated together with newer versions of micropython.
> 
> Signed-off-by: Abilio Marques <abiliojr@gmail.com>

I think the idea of this commit is very interesting, and the commit is
already quite good; still, I have a few comments and questions, see
below.

[--SNIP--]
> diff --git a/package/micropython/Config.in b/package/micropython/Config.in
> index 30161c8b70..2c2ae7a4df 100644
> --- a/package/micropython/Config.in
> +++ b/package/micropython/Config.in
> @@ -9,5 +9,15 @@ config BR2_PACKAGE_MICROPYTHON
>  
>  	  http://micropython.org
>  
> +config BR2_PACKAGE_MICROPYTHON_LIB
> +	bool "micropython-lib"
> +	default y
> +	depends on BR2_PACKAGE_MICROPYTHON
> +	select BR2_PACKAGE_PCRE # runtime
> +	help
> +	  Core Python libraries ported to MicroPython.
> +
> +	  http://micropython.org

Minor nitpick: no need to duplicate the homepage here.

[--SNIP--]
> diff --git a/package/micropython/collect_micropython_lib.py b/package/micropython/collect_micropython_lib.py
> new file mode 100755
> index 0000000000..778a1c4c7d
> --- /dev/null
> +++ b/package/micropython/collect_micropython_lib.py
> @@ -0,0 +1,90 @@
> +#!/usr/bin/env python3

As Chris hinted,maybe this should be a micropython script, so that we do
not need to play tricks with the module loading.

Note that I have not entirely reviewed this script for now; this is a
quick-ish review, there might be some mor ecomments on it later...

[--SNIP--]
> +def get_library_name_type(manifest_path: str) -> tuple[str, bool]:
> +    split = manifest_path.split("/")
> +    return (split[-2], "unix-ffi" in split)  # -1: "manifest.py", -2: library name
> +
> +
> +def get_all_libraries(mpy_lib_dir: str):
> +    # reuse manifestfile module capabilities to scan the micropython-lib directory
> +
> +    collected_list = manifestfile.ManifestFile(
> +        manifestfile.MODE_FREEZE, {"MPY_LIB_DIR": mpy_lib_dir}
> +    )
> +    collected_list.freeze(mpy_lib_dir)
> +
> +    for file in collected_list.files():
> +        if file.target_path.endswith("manifest.py"):
> +            yield get_library_name_type(file.full_path)
> +
> +
> +def copy_file(src: str, target: str, destination: str):
> +    s = target.split("/")
> +    s.pop()
> +    d = f"{destination}/{'/'.join(s)}"
> +    if not os.path.exists(d):
> +        os.makedirs(d)

No need to test before creating; just use exist_ok=False;

    os.makedirs(d, exist_ok=False)

> +    shutil.copy(src, f"{destination}/{target}")
> +
> +
> +def copy_libraries(manifest, destination: str):
> +    for f in manifest.files():
> +        copy_file(f.full_path, f.target_path, destination)
> +
> +
> +def process_cmdline_args():
> +    parser = argparse.ArgumentParser(
> +        description="Prepare micropython-lib to be installed"
> +    )
> +    parser.add_argument("micropython", help="Path to micropython source directory")
> +    parser.add_argument("destination", help="Destination directory")
> +
> +    args = parser.parse_args()
> +    return os.path.abspath(args.micropython), os.path.abspath(args.destination)
> +
> +
> +def load_manifestfile_module(micropython_dir: str):
> +    global manifestfile
> +    sys.path.append(f"{micropython_dir}/tools")
> +    manifestfile = importlib.import_module("manifestfile")

I don't think there is a need to set a global variable. Just return the
module you found. Except that, if this script becomes a micropython
script, then the loading could be done with a mere 'import' anyway.

> +def main():
> +    micropython_dir, destination_dir = process_cmdline_args()
> +    mpy_lib_dir = f"{micropython_dir}/lib/micropython-lib"
> +
> +    # load manifest file after knowing where it is
> +    load_manifestfile_module(micropython_dir)
> +
> +    manifest = manifestfile.ManifestFile(
> +        manifestfile.MODE_FREEZE, {"MPY_LIB_DIR": mpy_lib_dir}
> +    )
> +
> +    for library, is_ffi in get_all_libraries(mpy_lib_dir):
> +        manifest.require(library, unix_ffi=is_ffi)
> +
> +    copy_libraries(manifest, destination_dir)
> +
> +
> +main()
> diff --git a/package/micropython/micropython.mk b/package/micropython/micropython.mk
> index 37c148da94..a9757e266c 100644
> --- a/package/micropython/micropython.mk
> +++ b/package/micropython/micropython.mk
> @@ -47,6 +47,8 @@ define MICROPYTHON_BUILD_CMDS
>  	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D)/mpy-cross
>  	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D)/ports/unix \
>  		$(MICROPYTHON_MAKE_OPTS)
> +	$(EXTRA_ENV) package/micropython/collect_micropython_lib.py \
> +		$(@D) $(@D)/.built_pylib

No need to collect the libraries if BR2_PACKAGE_MICROPYTHON_LIB is not
set (see below).

>  endef
>  
>  define MICROPYTHON_INSTALL_TARGET_CMDS
> @@ -57,4 +59,14 @@ define MICROPYTHON_INSTALL_TARGET_CMDS
>  		install
>  endef
>  
> +define MICROPYTHON_INSTALL_LIBS
> +	$(INSTALL) -d -m 0755 $(TARGET_DIR)/usr/lib/micropython
> +	cp -a $(@D)/.built_pylib/* $(TARGET_DIR)/usr/lib/micropython
> +endef
> +
> +ifeq ($(BR2_PACKAGE_MICROPYTHON_LIB),y)
> +MICROPYTHON_POST_INSTALL_TARGET_HOOKS += MICROPYTHON_INSTALL_LIBS
> +endif

We define the hook and assign it in the if-block:

    ifeq ($(BR2_PACKAGE_MICROPYTHON_LIB),y)
    define MICROPYTHON_COLLECT_LIBS
        ...
    endef
    define MICROPYTHON_INSTALL_LIBS
        ...
    endef
    MICROPYTHON_POST_BUILD_HOOKS += MICROPYTHON_COLLECT_LIBS
    MICROPYTHON_POST_INSTALL_TARGET_HOOKS += MICROPYTHON_INSTALL_LIBS
    endif

> +
> +
>  $(eval $(generic-package))

So, as Chris suggested, maybe we want to build this as a host package,
so that we can use micropython at build time, for example to generate
the mpy files, or to use it as interpreter to the lib-install script.

Thank you!

Regards,
Yann E. MORIN.

> -- 
> 2.43.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot