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

Add make snippet for MicroPython #148

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add make snippet for MicroPython #148

wants to merge 1 commit into from

Conversation

JE-Archer
Copy link
Contributor

No description provided.

@JE-Archer JE-Archer requested a review from wom-bat January 13, 2025 03:15
Copy link
Contributor

@wom-bat wom-bat left a comment

Choose a reason for hiding this comment

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

Fix dependencies

MICROPYTHON_FROZEN_MANIFEST := manifest.py
include $(LIONSOS)/components/micropython/micropython.mk

manifest.py: webserver.py config.py
Copy link
Contributor

Choose a reason for hiding this comment

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

manifest.py doesn't get updated if webserver.py and config.py change. So it's not a dependency. you already have micropython.elf depending on these two files so if they change the recursive make will happen.

Copy link
Contributor Author

@JE-Archer JE-Archer Jan 14, 2025

Choose a reason for hiding this comment

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

Putting aside the fact that we are calling Make recursively and throwing away a bunch of information, the files referenced by manifest.py (such as config.py) are inputs into a file frozen_content.c. Ideally, they would be prerequisites of that file, as they need to exist before that file can be generated, and if they change, that file needs to be regenerated. But it's a bit impractical to make them dependencies of internal MicroPython targets, so making them transitive prerequisites (as MICROPYTHON_FROZEN_MANIFEST would be a prerequisite of frozen_content.c) ensures the required ordering, even if it's stricter than necessary. Making them prerequisites of micropython.elf is looser than necessary and doesn't capture the situation properly, because (if you are not calling the MicroPython makefile recursively and thereby enforcing a very coarse-grained ordering) they are not linker inputs and by the time you are building micropython.elf, frozen_content.c needs already to have been made.

micropython.mk is implemented as a recursive make hack, but webserver.mk and kitty.mk are written as if that weren't the case.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can fix this by adding:

# Micropython includes frozen versions of various python sources;
# note them here
micropython.elf: manifest.py webserver.py config.py

components/micropython/micropython.mk Outdated Show resolved Hide resolved
examples/kitty/kitty.mk Show resolved Hide resolved
include $(LIONSOS)/components/micropython/micropython.mk

manifest.py: webserver.py config.py
webserver.py: $(MICRODOT) config.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise, webserver.py doesn't change if config.py changes.

Signed-off-by: James Archer <[email protected]>
P
mpy-cross: FORCE $(LIONSOS)/dep/micropython/mpy-cross
make -C $(LIONSOS)/dep/micropython/mpy-cross BUILD=$(abspath ./mpy_cross)

FORCE: ;
Copy link
Contributor

Choose a reason for hiding this comment

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

If there's a 'FORCE' anywhere in any other snippet or in the top level Makefile this could cause an error to be thrown, depending on which version of Make you're using.
I suggest using

FORCE_MPY: ;
.PHONY: FORCE_MPY

and replace FORCE with FORCE_MPY in the rest of the snippet.


MICROPYTHON_DIR := $(realpath $(dir $(lastword $(MAKEFILE_LIST))))

micropython.elf: FORCE mpy-cross ${LIONSOS}/dep/libmicrokitco/Makefile $(MICROPYTHON_FROZEN_MANIFEST) $(MICROPYTHON_EXEC_MODULE)
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be better to depend not on the Makefile for libmicrokitco but on the generated library I think, if that's possible. We'll be moving to a snippet for libmicrokitco eventually I expect, so the Makefile will go away.
Right now, the Micropython component Makefile builds libmicrokitco, but I think we want to change that.

examples/kitty/kitty.mk Show resolved Hide resolved
MICROPYTHON_FROZEN_MANIFEST := manifest.py
include $(LIONSOS)/components/micropython/micropython.mk

manifest.py: webserver.py config.py
Copy link
Contributor

Choose a reason for hiding this comment

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

You can fix this by adding:

# Micropython includes frozen versions of various python sources;
# note them here
micropython.elf: manifest.py webserver.py config.py

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.

2 participants