Skip to content
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

Introduce a mechanism for specifying a "manifest" for frozen files #5084

Open
wants to merge 7 commits into
base: master
from

Conversation

@dpgeorge
Copy link
Member

commented Sep 9, 2019

This is an initial attempt to improve how frozen modules are specified. Instead of using symlinks (which can be unreliable on Windows, and not that powerful) the approach presented here uses a manifest file (a Python script) to specify what scripts to freeze. Some of the advantages of doing it this way are (not all are currently implemented):

  • can specify arbitrary file locations for the scripts to be frozen
  • can specify a different frozen name vs the actual file name
  • can specify different mpy-cross options (eg optimisations) per file
  • can specify to freeze via a string or compiled mpy file (currently FROZEN_DIR vs FROZEN_MPY_DIR)
  • can eventually include resources/data in the manifest
  • can eventually include read-only/memzip/initial filesystem in the manifest

Included in this PR is an example of how it would work for the unix port (freezing upip).

There could eventually be a hierarchy of manifests: one for the port, one for the particular board, and one for the user application (each could override the defaults if necessary).

The idea would be that this mechanism completely replaces the existing way to specify frozen files.

The implementation here is a bit rough (eg it unconditionally rebuilds each time), comments/discussion/etc are welcome.

@nevercast

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

Please provide a sugar function add_frozen_dir('../../freeze_everything', **kwargs) that walks the path and adds all the *.py files, keeping the manifest small in comparison to just pointing FROZEN_MPY_DIR to my own folder.

@dpgeorge

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

Please provide a sugar function add_frozen_dir('../../freeze_everything', **kwargs) that walks the path and adds all the *.py files,

Yes, good idea.

keeping the manifest small in comparison to just pointing FROZEN_MPY_DIR to my own folder.

It is convenient with the existing mechanism to simply point to a directory to freeze. With a manifest file you'll need both the directory and the manifest. Even if the manifest is a single line it's still extra work, an extra file to keep track of.

Maybe the manifest description could be placed in the directory to freeze, and then there's some code which automatically skips the manifest when freezing an entire directory.

Another option is to allow to pass a directory as the manifest to the makemanifest.py script (instead of a Python script with the manifest list), and it detects that it's a directory and just freezes the entire thing with defaults.

@nevercast

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

Another option is to allow to pass a directory as the manifest to the makemanifest.py script (instead of a Python script with the manifest list), and it detects that it's a directory and just freezes the entire thing with defaults.

I think I like this option the most. But its less simple. Is it error prone to bad paths, i.e. poor handling of spaces? Possibly. e.g.

./makemanifest.py /home/user/path/to/micropython/my application/manifest.py

Observe the space in my application. Since we now accept a path that doesn't end in manifest.py is this a problem?

Can the argument be sufficiently quoted so this cannot happen? Alternatively, assert that only one argument was passed to makemanifest.py

Maybe I'm being overly cautious.

@UnexpectedMaker

This comment has been minimized.

Copy link

commented Sep 10, 2019

I honestly don't mind how it's implemented... Just being able to select what additional libraries are frozen for TinyPICO and my future boards would be an amazing feature! Thanks @dpgeorge and @jimmo for starting to look into this!

@dpgeorge dpgeorge force-pushed the dpgeorge:frozen-manifest branch from 2128352 to 5f42256 Sep 10, 2019
for p in path.split('/')[:-1]:
cur_path += p + '/'
try:
os.mkdir(cur_path)

This comment has been minimized.

Copy link
@andrewleech

andrewleech Sep 16, 2019

Contributor

This function could be replaced by os.makedirs(path, exist_ok=True), unless python2 support is still desired?
https://docs.python.org/3.5/library/os.html#os.makedirs

This comment has been minimized.

Copy link
@dpgeorge

dpgeorge Sep 16, 2019

Author Member

Thanks, I missed that.

But I do think it's important to have Python 2 support, since all the other scripts have it and on some embedded systems Python 2 is all they have.

This comment has been minimized.

Copy link
@nevercast

nevercast Sep 16, 2019

Contributor

I would hope we really really wont be writing any Python2 code ever again. Its dead!

This comment has been minimized.

Copy link
@andrewleech

andrewleech Sep 16, 2019

Contributor

I do agree with Damien here, there's plenty of 'stable' systems that are Python2 still. I personally won't be writing ugly code again for the sake of compatibility, but this here is a particularly minor concession.

This comment has been minimized.

Copy link
@nevercast

nevercast Sep 16, 2019

Contributor

Why does "What embedded systems have" impact the build process at all?

This comment has been minimized.

Copy link
@nevercast

nevercast Sep 16, 2019

Contributor

And for what it is worth, this makemanifest.py file is not Python2 compatible.

This comment has been minimized.

Copy link
@dpgeorge

dpgeorge Sep 16, 2019

Author Member

Why does "What embedded systems have" impact the build process at all?

Eg building MPY on an old RPi or similar SoM running Linux, or in a virtualbox image that is old and you can't update for some reason.

@andrewleech

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

Another option is to allow to pass a directory as the manifest to the makemanifest.py script (instead of a Python script with the manifest list), and it detects that it's a directory and just freezes the entire thing with defaults.

Wouldn't this be basically the same as the existing FROZEN_MPY_DIR make arg?

@dpgeorge

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2019

Wouldn't this be basically the same as the existing FROZEN_MPY_DIR make arg?

Yes it would, it's trying to provide "backwards compatibility". Also convenience for the case where you just want to freeze a whole directory tree.

@peterhinch

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

Freezing a tree is very useful for large projects implemented as Python packages.

@pmp-p

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2019

Using python hooks for anything related to user mod or freezing is very useful, as users - to automate their python build - may do not want to deal with Makefile language or local filesystem

eg. some could prefer venv + cmake, does that solution allow to just call cmake for a temporary access to implementation of the not implemented here features ?

@dpgeorge

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2019

does that solution allow to just call cmake for a temporary access to implementation of the not implemented here features

If you mean the features listed in the first post of this issue, then no it doesn't include anyway to hook in additional things.

@pmp-p

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2019

Yes i did mean "first post of this issue" as "here" and sorry for phrasing i now hear how it sounds like "not invented here" when said out loud.

no it doesn't include anyway to hook in additional things.

i see that as a source of problem, regardless of quality and benefits of the work already done "here" and future ( completion of missing features ).

I mean by that : without hooks you push people toward "embedding" build system instead of "extending" and of course going back to their prefered (bloated?) solution to do that and that probably does not leave much room for contributing.

now for not here if i may :

As a matter of fact I already felt that at first contact with the build system and multiple times not only on C side but on a fork requiring frozen modules that i incidently already use on wasm and cpython https://github.com/littlevgl/lv_micropython/tree/lvgl_javascript/ports/javascript/modules ( those pointed of course are minimalistic safe,verbose versions of them )

That's why i renamed the "over complicated" above linked PR fixing cpp subject to "Makefile overhaul". hoping for hooks to write python code instead of Makefile lang (which does not make sense using at frreeze time, when you have already minimal host python at hand )

if not hookS please close and wait for emscripten fixes i'm quite sure @embeddedt will integrate them nicely if not someone else. Because i can't endorse a module integrator that does not allow to scan files for potential threats except for non wifi boards or is laying out only for its own way.

@embeddedt

This comment has been minimized.

Copy link

commented Sep 26, 2019

i'm quite sure @embeddedt will integrate them nicely if not someone else.

Still waiting on the upstream Emscripten issue, but yes, I intend to solve the problem cleanly once there is consensus on the solution.

@nevercast

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

Let's not let hooks get in the way of this feature for the moment though. Let's keep this pull request purely focused on manifest and frozen files.

As it stands this PR needs some extra work, the scripts aren't Python2 compatible and have we agreed on the format of the macros and the manifest file yet? Last I looked, I liked it.

@nevercast

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

While cleaning up symlinks in favour of a manifest can we specifically remove symlinks that point to micropython-lib. It makes micropython-lib a surprise dependency.

@embeddedt

This comment has been minimized.

Copy link

commented Oct 9, 2019

can we specifically remove symlinks that point to micropython-lib.

Please do this (or at least, begin to consider an alternative).

It makes it difficult to host a MicroPython fork on GitHub Pages (which won't allow dead symlinks, and cannot symlink across repositories).

For LittlevGL we ended up creating a separate branch for Pages and manually removing symlinks like this one there. That shouldn't be necessary, in my opinion.

@dpgeorge dpgeorge force-pushed the dpgeorge:frozen-manifest branch 2 times, most recently from b2f036d to b5cffed Oct 10, 2019
@dpgeorge

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2019

I've updated this PR. Changes since the initial revision:

  • unix, stm32, esp8266, esp32, qemu-arm ports all converted to use the new FROZEN_MANIFEST option
  • all symlinks removed from those ports directories, everything now specified in a manifest.py
  • support for including a manifest in another
  • support for freezing a whole directory
  • support for specifying multiple files in the same directory
  • support for the "magic" path :MPY: which expands to the top of the MicroPython repo tree
  • support for freezing via strings (not .mpy)

Examples use of all features can be found in various ports.

@dpgeorge

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2019

While cleaning up symlinks in favour of a manifest can we specifically remove symlinks that point to micropython-lib. It makes micropython-lib a surprise dependency.

I agree, this is not good. In the latest version here all of these symlinks are now removed.

if not hookS please close and wait for emscripten fixes i'm quite sure @embeddedt will integrate them nicely if not someone else. Because i can't endorse a module integrator that does not allow to scan files for potential threats except for non wifi boards or is laying out only for its own way.

@pmp-p can you please explain in more detail (eg with examples, even better with a PR showing proof of concept...) exactly what you'd like to see in terms of "hooks"? What exactly do you want to hook into? Why are Makefile's no good? You can already extend the build system simply by making your own GNUmakefile and taking control of the build system that way.

@dpgeorge dpgeorge force-pushed the dpgeorge:frozen-manifest branch from b5cffed to 6ca4e79 Oct 10, 2019
@pmp-p

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

making your own GNUmakefile

@dpgeorge i know, that's why i have precised for users that "may do not want to deal with Makefile language or local filesystem"

for example emscripten has a specific cmake plugin and android sdk is also moving toward cmake instead of ndk-build ( which is yet another "makefile" flavour ).

Makefile when it's related to remote git tree and archive downloading + checksumming is getting a bit overwhelmed. And since anyway micropython relies on a working host python, why bother people with less powerfull tools since cmake is a "pip install" away ( very clean when in a venv ) ?

eg to checking a full cpython in cmake would be

ExternalProject_Add(
    python3
    DEPENDS openssl
    DEPENDS libffi
    DEPENDS bz2
    GIT_REPOSITORY https://github.com/python/cpython.git
    GIT_TAG 4082f600a5bd69c8f4a36111fa5eb197d7547756 # 3.7.5rc1
    CONFIGURE_COMMAND ""
    BUILD_COMMAND ""
    INSTALL_COMMAND ""
)

that seems far from easy to do that with a bare GNUmakefile, not to mention depends resolution ...

Note that i did not read that PR in depth, and yeah there are certainly ways to hook manually, but it certainly would be nice to have at least 1 supported and documented way

related, i think user mod system should not enforce specfic build tools or assume mods are written in a specific language ( like C or MicroPython-ready Python ). Or that only only one build system tool will be used in the same firmware build pass.

@nevercast

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

@pmp-p Its still not clear to me what you want to hook, and perhaps if I don't understand it others will not. I'm assuming during the build you want to run some code between steps. What are these steps and what are you trying to do? Perhaps if you describe your usecase as a user story of what you want to achieve (instead of how you would achieve it) I could better understand your needs.

@mcauser

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

Hi @dpgeorge, I noticed a slight difference between the esp8266, esp32, stm32, nrf and cc3200 ports:

On the esp32, esp8266 and stm32 ports, /modules contains frozen .py and the .c modules are in the root of the port dir.
On the nrf port /modules contain .c modules and frozen .py is in /frozen.
On the cc3200 port, /mods contain .c modules along with a bunch of other dirs.

  1. On esp32, esp8266 and stm32 should dir /modules be renamed to /frozen then nest .c modules in /modules to match the nrf dir structure?

  2. On the cc3200 port, rename /mods to /modules? What about those other modules dirs, ftp, hal etc? Also nest them (/modules/ftp)?

@dpgeorge

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2019

I noticed a slight difference between the esp8266, esp32, stm32, nrf and cc3200 ports:

Yes, they do have different source layouts. But I don't think that should be addressed here, it's a bit out of scope and a large issue. Although it would be good to at least have in mind what to do about your point (1), if modules/ should be renamed to frozen/.

In the end I think most of the scripts in, eg, esp8266/modules should go elsewhere (eg in top level lib/ or drivers/) so they can be reused by all ports. Then this directory could go. In this PR it's already gone for stm32.

@dpgeorge

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2019

why bother people with less powerfull tools since cmake is a "pip install" away

Ok, so are you advocating to change the whole build system to cmake? That's quite a tangent to this PR here. And even if cmake was used instead of standard (GNU) make, this frozen-manifest would still be a component in a cmake system.

I'm not convinced that cmake is the right way to go. It's simple for simple things but can get very complex when you want to do more. Make is at least bounded in its complexity and most people understand it. I would much rather write a custom, tailored build system directly in Python than use cmake.

and yeah there are certainly ways to hook manually, but it certainly would be nice to have at least 1 supported and documented way

Please do describe what "hook" means and some use cases that you see for it.

@pmp-p

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

@nevercast i don't have personnal needs, i'm trying to set my point of view from users whose knowledge may be restricted to Python only, also i can think of various cases or ones i have already encountered when helping people :

user may not want to:

  • use/learn makefile
  • use/learn offered build system

user may want:

  • download and freeze remote packages in a secure way ( maybe via a menu ? )
  • insert source code filters to detect supply chain attacks, or insert transpilers ( cpython -> mp )
  • patch micropython ( LittlevGL case )
  • patch build system ( emscripten , android , wasi .... )
  • use something he already knows or has documentation for ( cmake , gradle, whatever ... )

imho frozen code / user modules are bit out of scope of core/port build system so a minimal default as offered here is good, but total liberty should be left to multiples concurrent extensions mechanisms at the various key point stages of firmware build.

@dpgeorge no i'm not suggesting cmake , i just took it as example because it does all sort of things and available for free.
maybe it could be as simple as scanning a "frozen.conf.d" folder and running what's found in there
i don't suggest to replace anything but more suggesting to offer the possibility of "stacking" solutions from various sources.

@nevercast

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

@pmp-p If we change the build system to something else, we have a different sum of people that don't like that particular build system and would prefer to use something else.

I don't think its realistic for a project to cater to people's preferences in build systems, how could that ever work? What would it look like to have MicroPython be immediately compatible with Gradle for example?

If a developer wants to write patches for MicroPython or modify the way a port is built, they are going to have to have an indepth understanding of how MicroPython works. In that process they will learn a build system as that is a small portion of the project complexity.

Since you don't have usercases of your own, I think its important that we backburner the idea of hooks for now until we can collect some use cases from the wider community. Maybe a forum post on the subject would be a good place to discuss this, see if some users have ideas about where they would like hooks, and its really really important that people say what they want to achieve, not how they would achieve it.

@andrewleech

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

@pmp-p Pretty much the entire point of frozen files and frozen manifest system is so people don't need to learn or edit the makefiles.

The docs for the existing system are here: http://docs.micropython.org/en/latest/reference/packages.html?highlight=frozen#cross-installing-packages-with-freezing
This PR is about making this system easier and more flexible.

But basically you just clone the repo on a computer with the toolchain setup, then either:

  • copy your python modules into existing frozen module folder for your port and run make
  • update the ports existing manifest.py file to point to your modules and run make
  • create your own manifest.py file pointing to your modules then run make FROZEN_MANIFEST=/path/to/manifest.py

End users never need to understand how make works at all, just the format of the new simple frozen manifest.py file.

@pmp-p

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

@nevercast seriously please tell me where i said "change the build system" ? it seems @dpgeorge too thought i want to replace something, from the start i said choose "extending" instead of "embedding".

if a developer wants to write patches (...) of the project complexity.

i totally agree, but what is the link with the user experience of a ready made patch/build system made by this dev. and offered to community ?

you don't have usercases of your own

How could i have any on mainline ? chicken-egg problem.
There's nothing out of the box for extending current build system apart from rewriting main Makefile or shadowing build system (which i personnaly do since i work on a different VM model)

make FROZEN_MANIFEST=/path/to/manifest.py

@andrewleech that is "replacing" not extending. and that's situations like that hooks are expected to avoid.

@dpgeorge dpgeorge force-pushed the dpgeorge:frozen-manifest branch from 6ca4e79 to b66cd5b Oct 11, 2019
@embeddedt

This comment has been minimized.

Copy link

commented Oct 11, 2019

seriously please tell me where i said "change the build system" ?

It might not have been your intent, but that is the meaning someone might get out of this:

user may not want to:

  • use/learn makefile
  • use/learn offered build system

I think that if one is going to adopt an open-source project, they have to be willing to learn something about it. I can't come to MicroPython and ask them to rewrite the entire codebase in C++ simply because that's easier for users.

It's not exactly clear to me what improvements you want to make. Perhaps you could give a brief description of how you would change the MicroPython build system to be more extensible and then there could be a discussion around that.

Personally, I think the conversation has gone a bit off track. The PR was originally about adjusting the way frozen modules work, but now it's become a broad discussion about the proper way to extend MicroPython.

patch micropython ( LittlevGL case )

At a high level, that meant adding a step to py/py.mk to generate the MicroPython module using amirgon's script, and adding glue logic to each port (for drivers, etc.).

Using that as an example, it would be better, in my opinion, to suggest that some hook be added for that specific use case (giving that as an example) instead of trying to improve the build system without a clear idea of where the improvements need to be.

Of course, I'm not a MicroPython developer (I've just gotten familiar with it from working on the LittlevGL simulator) so feel free to ignore my comment. 🙂

@pmp-p

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

I can't come to MicroPython and ask them to rewrite the entire codebase in C++ simply because that's easier for users.

@embeddedt the specific step discussed here is about third party Python modules integration and freezing. of course LittlevGL is not the best example because it's C patching not Python patching but for lv_micropython javascript you did have to integrate third party modules not provided by micropython-lib and i've put a link documenting that.

You are right third parties should not have to come and ask for anything - but i had to for helping on import third party module problem ( which was not mine i had import since day 1 ).

but now it's become a broad discussion about the proper way to extend MicroPython.

if third party Python modules are not the primary way to extend MicroPython, then please notify and ignore me.

instead of trying to improve the build system without a clear idea of where the improvements need to be.

The above reply to @dpgeorge about "maybe it could be as simple as scanning a "frozen.conf.d" folder and running what's found in there" does not seems a over-complicated solution to add in makefile along FROZEN_MANIFEST handling but i'm not a makefile specialist, so i would take your advice on that ( i would have cowardly put the hooks in a freezing python script that default to handle FROZEN_MANIFEST ) .

dpgeorge added 6 commits Sep 9, 2019
Removes symlinks in modules directory, all is now specified by manifest.py.
All symlinks are removed, files now referenced via boards/manifest.py.
All symlinks are removed.  boards/manifest.py is used as a default, can
optionally use boards/manifest_release.py for more scripts.
@dpgeorge dpgeorge force-pushed the dpgeorge:frozen-manifest branch from b66cd5b to 94d3e2e Oct 12, 2019
@dpgeorge

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2019

This is now pretty close to being mergeable. The main way to freeze code is to just have freeze(dir) or freeze(base_dir, script) in the manifest file. But you can also get away without having a manifest file by passing the freeze command directly to the make variable, like:

FROZEN_MANIFEST = "freeze('mydir')"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.