Skip to content

top: Replace setup.py/metadata.txt with manifest.py. #506

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.

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 4 commits into from

Conversation

jimmo
Copy link
Member

@jimmo jimmo commented Jul 15, 2022

This is the first step towards updating MicroPython's package management and deployment system in a way that combines freezing, on-device-install and host-based-install. This uses the new manifest.py features: require(), package(), and module().

More details at the corresponding PR in the main: repo micropython/micropython#8914.

@andrewleech
Copy link
Contributor

andrewleech commented Jul 29, 2022

I don't think split packages work in freezing currently, trying to freeze with a manifest that includes os.path results in

CC build-dev/frozen_content.c
build-dev/frozen_content.c:131541:27: error: redefinition of ‘const_qstr_table_data_os___init__’
131541 | static const qstr_short_t const_qstr_table_data_os___init__[4] = {
       |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
build-dev/frozen_content.c:70201:27: note: previous definition of ‘const_qstr_table_data_os___init__’ was here
70201 | static const qstr_short_t const_qstr_table_data_os___init__[4] = {
      |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
build-dev/frozen_content.c:131548:33: error: redefinition of ‘frozen_module_os___init__’
131548 | static const mp_frozen_module_t frozen_module_os___init__ = {
       |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~
build-dev/frozen_content.c:70208:33: note: previous definition of ‘frozen_module_os___init__’ was here
70208 | static const mp_frozen_module_t frozen_module_os___init__ = {
      |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~
build-dev/frozen_content.c:70208:33: error: ‘frozen_module_os___init__’ defined but not used [-Werror=unused-const-variable=]
cc1: all warnings being treated as errors

edit: Ignore this. the problem here was caused by my (scripted) manifest including both unix-ffi/os and python-stdlib/os

jimmo added 2 commits August 10, 2022 22:57
These are unused and will be replaced with manifest.py.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This is unmaintained and not the one installed by default on
boards (see github.com/micropython/micropython/blob/master/tools/upip.py)

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
@jimmo jimmo force-pushed the lib-manifest branch 2 times, most recently from 4130ab6 to 324f632 Compare August 10, 2022 14:44
@jimmo
Copy link
Member Author

jimmo commented Aug 10, 2022

Rebased and updated for recent changes.

edit: Ignore this. the problem here was caused by my (scripted) manifest including both unix-ffi/os and python-stdlib/os

@andrewleech Yeah this was on the TODO list... basically the problem is we need the whole thing to be one namespace. Initially I considered renaming unix-ffi/os to unix-ffi/os-unix, but this ends up leading to other problems, and I don't think it really makes sense.

I have tried to address this with an extra feature in micropython/micropython#8914 and by an update that I've just pushed to this branch that essentially silos out the unix-ffi directory so that:

  • By default, require() will not get you a unix-ffi library.
  • If you specify unix=True in requre(), then the unix-ffi version will take precedence.

@andrewleech
Copy link
Contributor

  • If you specify unix=True in requre(), then the unix-ffi version will take precedence.

Personally I'd prefer it to be ffi=True as calling the arg Unix might make people think they need that set to use at all on Unix port.

@jimmo
Copy link
Member Author

jimmo commented Aug 10, 2022

Personally I'd prefer it to be ffi=True as calling the arg Unix might make people think they need that set to use at all on Unix port.

@andrewleech I considered that, but the unix-ffi modules are more than just ffi. Some of them just use features that only work or make sense on unix. I agree "unix" isn't the best name for this arg though.

Can we solve this with documentation instead? The rule is pretty simple, especially given that the default behaviour (don't pass the arg) is what you want almost all of the time. The primary goal is to make impossible to accidentally require() a unix module from a microcontroller build.

There is nothing stopping you doing this

require("os")
require ("os, unix=True)

either directly or transitively.

Although if we go ahead with options 2 or 4 from micropython/micropython#9018 then this problem goes away because the modules will be renamed to "os_ext" and "os_ext_ffi".

I also considered removing manifest.py for everything in unix-ffi (i.e. it's just an unmaintained contrib directory of files you can use however you like)... but this seems over the top.

@jimmo
Copy link
Member Author

jimmo commented Aug 10, 2022

What if I renamed the arg.to unix_ffi to align it with the fact that it's literally (modulo underscore vs hyphen) the unix-ffi directory?

@andrewleech
Copy link
Contributor

That's a good / unambigious argument.

@jimmo
Copy link
Member Author

jimmo commented Aug 11, 2022

OK thanks! Renamed to unix_ffi

jimmo added 2 commits August 12, 2022 09:43
Uses the new require()/package()/module() functions from manifestfile.py.

Add manifest.py for iperf3 and pyjwt.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
With the dependencies captured in manifest.py, several packages in
python-stdlib were still unix-only due direct or transitive dependencies
on unix-only or ffi modules. Or they just make no sense to run on
microcontroller targets.

In a few cases (e.g. base64) where possible, the unix dependency could be
removed.

Updates manifest.py to use the `unix=True` arg to `require()` for these
libraries.

Rename re-pcre to re now that unix-ffi is effectively its own namespace.

Update unix-ffi/README.md, and strengthen the wording that the unix
libraries are unmaintained.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
@dpgeorge
Copy link
Member

dpgeorge commented Sep 5, 2022

Merged in ecef7a5 through f3cfc52

@dpgeorge dpgeorge closed this Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants