diff --git a/.github/workflows/build-mp.yml b/.github/workflows/build-mp.yml index 0c49de21..52f358e3 100644 --- a/.github/workflows/build-mp.yml +++ b/.github/workflows/build-mp.yml @@ -1,5 +1,6 @@ # build_mp.py's own unit tests: the decisions it makes before any build, such -# as when a build dir has to be wiped. No toolchain, no MicroPython checkout. +# as when a build dir has to be wiped. No toolchain, no MicroPython checkout, +# except for the one job that asks make where it finds a module's sources. name: build_mp.py tests on: @@ -8,11 +9,15 @@ on: paths: - 'build_mp.py' - 'test_build_mp.py' + - 'UPSTREAM' + - 'patches/micropython/**' - '.github/workflows/build-mp.yml' pull_request: paths: - 'build_mp.py' - 'test_build_mp.py' + - 'UPSTREAM' + - 'patches/micropython/**' - '.github/workflows/build-mp.yml' workflow_dispatch: @@ -27,3 +32,29 @@ jobs: - uses: actions/checkout@v7 - name: Unit tests run: python3 -m unittest test_build_mp -v + + vpath: + # make must find each user C module's sources in its own directory, not in + # a directory of the same name beside another module (patches/micropython/ + # 0040). The test asks the unix port's Makefile where it finds one; nothing + # is compiled. It skips without a prepared checkout, so here it is required. + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v7 + - name: Read the pinned upstream from UPSTREAM + id: pin + run: echo "ref=$(cat UPSTREAM)" >> "$GITHUB_OUTPUT" + - name: Check out the pinned upstream + uses: actions/checkout@v7 + with: + repository: micropython/micropython + ref: ${{ steps.pin.outputs.ref }} + path: micropython + fetch-depth: 1 + - name: Lay the overlay on it + run: python3 patches/apply_patches.py micropython + - name: Each module's sources come from its own directory + env: + PYDEVICES_REQUIRE_MICROPYTHON: '1' + run: python3 -m unittest test_build_mp.VpathSearchesEachModuleFirst -v diff --git a/README.md b/README.md index 9a6c9d49..a855bef3 100644 --- a/README.md +++ b/README.md @@ -79,7 +79,7 @@ New here? The [newcomer's guide](docs/newcomers.md) explains the model; the - [`ppa/`](modules/ppa/): scale, rotate, convert and fill pixel buffers, on the ESP32-P4's Pixel Processing Accelerator or in portable C elsewhere. - [`tsmux/`](modules/tsmux/): MPEG-TS and HLS segments from H.264. -- [`patches/micropython/`](patches/micropython/) — our patch series (`0001-…` to `0039-…`; there +- [`patches/micropython/`](patches/micropython/) — our patch series (`0001-…` to `0040-…`; there is no `0026`, whose fix moved into `patches/tinyusb/`), each justified in its own header; [`patches/apply_patches.py`](patches/apply_patches.py) applies it, plus each module's own MicroPython patches, once, as one local commit. diff --git a/build_mp.py b/build_mp.py index ef23a19d..a4452646 100755 --- a/build_mp.py +++ b/build_mp.py @@ -498,6 +498,57 @@ def at(row): return moved +def foreign_sources(build, roots): + """Objects a make build compiled from outside the module they belong to: + (dependency file, source) for each, or [] when every one came from its + own module. + + make ports name a module's sources /.c and find them through + vpath, in the parent directories of the build's modules. When modules come + from different places, a directory of the module's name beside another + module's parent (an older checkout of it, say) can be searched first, and + its copy is compiled, silently. Our py.mk patch searches each module's own + directory first; this reads what the compiler was actually handed, from + each object's dependency file (BUILD//.P, whose first rule + names the source first), so it catches the next way it goes wrong too. + + `roots` are the build's module directories (and anything else the build + is meant to compile from, such as lvgl-bindings): a source counts as its + module's when the directory it was found under is inside one of them. + CMake ports name sources by absolute path and write no .P files.""" + roots = [Path(os.path.realpath(r)) for r in roots] + found = [] + for dep in sorted(Path(build).rglob("*.P")): + rel = dep.relative_to(build).with_suffix("").as_posix() + if "/" not in rel: + continue # the port's own files, built at the top of BUILD + rule = [] + try: + with open(dep, errors="replace") as f: + for line in f: + rule.append(line.rstrip("\r\n").rstrip("\\")) + if not line.rstrip("\r\n").endswith("\\"): + break + except OSError: + continue + _, colon, deps = " ".join(rule).partition(": ") + if not colon or not deps.split(): + continue + src = deps.split()[0].replace("\\", "/") + stem = src.rpartition(".")[0] + # Found through vpath as /, the source ends with the + # object's path under BUILD. One relative to the port (../../py/gc.c) + # or absolute all the way (BUILD/home/...) was never searched for. + absolute = src.startswith("/") or re.match(r"[A-Za-z]:/", src) + if not absolute or not stem.endswith("/" + rel) or stem == "/" + rel: + continue + module = rel.split("/", 1)[0] + found_in = Path(os.path.realpath(stem[: -len(rel)] + module)) + if not any(found_in == r or r in found_in.parents for r in roots): + found.append((dep, src)) + return found + + # ---- esp32: the generated board directory, the fragments, autosize -------- def esp32_chip(base): @@ -1258,6 +1309,16 @@ def shell(cmd, log=None): out.append(chunk) return p.wait(), "".join(out) + # make keeps an object whose source it now finds somewhere else, if the + # object is newer, so one compiled from a stray copy by a build made before + # py.mk searched each module's own directory first would stay in. Drop + # those, and this build compiles them again from their modules. + roots = module_dirs + ([bindings] if bindings else []) + for dep, src in foreign_sources(build, roots): + say(f"compiling {dep.relative_to(build).with_suffix('.o').as_posix()} again: it was compiled from {src}") + dep.with_suffix(".o").unlink(missing_ok=True) + dep.unlink() + rc, out = 0, "" tinyusb = None try: @@ -1343,6 +1404,12 @@ def shell(cmd, log=None): if rc != 0: hint = failure_hint(port, out) die(f"the build failed (make exit {rc})" + (f"\n{hint}" if hint else "")) + stray = [f"{dep.relative_to(build).with_suffix('.o').as_posix()} was compiled from {src}" + for dep, src in foreign_sources(build, roots)] + if stray: + shown = "\n ".join(stray[:10]) + (f"\n ... and {len(stray) - 10} more" if len(stray) > 10 else "") + die(f"some of this build's objects were compiled from outside their module:\n {shown}\n" + "A directory of the module's name beside another module was searched before the module's own.") rec = dict(want, complete=True) if port == "rp2" and rp2_storage: rec["rp2_storage_bytes"] = rp2_storage diff --git a/modules/audioif/micropython.mk b/modules/audioif/micropython.mk index b5395cf1..cd3b72d4 100644 --- a/modules/audioif/micropython.mk +++ b/modules/audioif/micropython.mk @@ -21,15 +21,6 @@ $(error audioif: audiodsp not found (looked in USER_C_MODULES and at $(AUDIODSP_ endif CFLAGS_USERMOD += -I$(AUDIODSP_DIR)/src -# py.mk names these sources audioif/.c and finds them through vpath, -# whose list holds the PARENT directory of every module in the build, sorted. -# A module linked in from elsewhere (audiodsp beside this repository) puts -# that elsewhere on the list, and if it also holds an audioif/ -- an older -# standalone checkout of this module -- its copy of _audioif.c is the one -# compiled, silently, with this directory's new files linked against it. -# make searches vpath directives in the order they appear, and this one -# comes before py/mkrules.mk's, so these files are found here first. -vpath audioif/%.c $(abspath $(AUDIOPUMP_MOD_DIR)/..) SRC_USERMOD_C += $(AUDIOPUMP_MOD_DIR)/_audioif.c SRC_USERMOD_C += $(AUDIOPUMP_MOD_DIR)/audiobusio.c diff --git a/patches/apply_patches.py b/patches/apply_patches.py index 73610899..f34bb618 100755 --- a/patches/apply_patches.py +++ b/patches/apply_patches.py @@ -153,11 +153,13 @@ def prepare(mp, upstream, patches, refresh=False): head = git(mp, "log", "-1", "--format=%h", capture=True) print(f"overlay already applied: {head} on {upstream}", flush=True) else: - dirty = [ - line - for line in git(mp, "status", "--porcelain", "--untracked-files=no", capture=True).splitlines() - if not line.startswith(" m") - ] + # A submodule with edits inside it doesn't count (micropython-lib carries + # ours, taken off and put back below; a port's build can touch its own). + # --porcelain spells that " M", like a submodule moved to another + # commit, so it is git that leaves them out, not a match on the letter. + dirty = git( + mp, "status", "--porcelain", "--untracked-files=no", "--ignore-submodules=dirty", capture=True + ).splitlines() if dirty: sys.exit(f"{mp} has uncommitted changes; commit or discard them first") if git(mp, "describe", "--tags", "--exact-match", check=False, capture=True) != upstream: diff --git a/patches/micropython/0040-micropython-py-mk-search-each-user-c-module-in-its-own-directory-first.patch b/patches/micropython/0040-micropython-py-mk-search-each-user-c-module-in-its-own-directory-first.patch new file mode 100644 index 00000000..76e4e992 --- /dev/null +++ b/patches/micropython/0040-micropython-py-mk-search-each-user-c-module-in-its-own-directory-first.patch @@ -0,0 +1,56 @@ +From f1459480609fefd9b8b9452102dc3725d0626378 Mon Sep 17 00:00:00 2001 +From: Brad Barnett <127794626+bdbarnett@users.noreply.github.com> +Date: Mon, 19 Oct 2026 14:06:26 -0500 +Subject: [PATCH] py/py.mk: search each user C module's own directory first. + +py.mk names a user C module's sources /.c and mkrules.mk +finds them through vpath, whose list holds the parent directory of every +module in the build, sorted. When the modules come from different places, +that list can put another directory first that also holds a directory of +the module's name, such as an older checkout of the module beside one of +the others. Its copy of a file is then the one compiled, with no warning, +and linked against the module's own headers and its other files. + +Seen with micropython-pydevices: its displayif lives in modules/displayif, +and audiodsp is a sibling checkout linked in, so audiodsp's parent, which +also held a standalone displayif checkout from before the module moved in, +sorted first. Three of displayif's files came from the old checkout. A +planted #error in a stray copy shows it in a few seconds. + +Each module now gets a vpath directive of its own, /%.c (and .cpp, +.S, .s) in its own parent directory, before the shared one in mkrules.mk; +make searches directives in the order they appear. A build whose modules +all share one parent resolves exactly as before. CMake ports name module +sources by absolute path and never had the problem. + +Unchanged upstream as of b70ae1aca. +--- + py/py.mk | 12 ++++++++++++ + 1 file changed, 12 insertions(+) + +diff --git a/py/py.mk b/py/py.mk +index 558f37d7d..fd55cf806 100644 +--- a/py/py.mk ++++ b/py/py.mk +@@ -89,6 +89,18 @@ $(foreach _MDIR, $(USERMOD_DIRS), \ + # cexample/foo.c (produced by the patsubst above) resolve to source files. + USERMOD_DIR_PARENTS := $(sort $(foreach _MDIR, $(USERMOD_DIRS), $(patsubst %/,%,$(dir $(_MDIR))))) + ++# Each module's own directory first. The search list mkrules.mk gives vpath ++# holds every module's parent directory, sorted, so when modules come from ++# different places, a directory of the same name beside another module's ++# parent (an older checkout of the module, say) can come first, and its copy ++# of a file is the one compiled, with no warning. make searches vpath ++# directives in the order they appear, and these come before mkrules.mk's. ++$(foreach _MDIR, $(USERMOD_DIRS), \ ++ $(foreach _EXT, c cpp S s, \ ++ $(eval vpath $(notdir $(_MDIR))/%.$(_EXT) $(patsubst %/,%,$(dir $(_MDIR)))) \ ++ ) \ ++) ++ + CFLAGS += $(CFLAGS_USERMOD) + CXXFLAGS += $(CXXFLAGS_USERMOD) + LDFLAGS += $(LDFLAGS_USERMOD) +-- +2.43.0 + diff --git a/test_build_mp.py b/test_build_mp.py index bd6747fd..aa603e49 100644 --- a/test_build_mp.py +++ b/test_build_mp.py @@ -6,6 +6,7 @@ import contextlib import io import json +import os import subprocess import tempfile import unittest @@ -198,6 +199,100 @@ def test_variant_manifests(self): build_mp.VARIANTS_DIR, build_mp.MODULES_DIR = saved +class ForeignSources(unittest.TestCase): + """foreign_sources() names an object compiled from outside its module, and only that.""" + + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.root = Path(self.tmp.name) + self.build = self.root / "build" + self.module = self.root / "repo" / "modules" / "displayif" + (self.module / "src").mkdir(parents=True) + + def tearDown(self): + self.tmp.cleanup() + + def dep(self, obj, src): + p = self.build / obj + p.parent.mkdir(parents=True, exist_ok=True) + # What mkrules.mk leaves: gcc's rule, then a "header :" line per header. + p.write_text(f"{self.build}/{obj[:-2]}.o: \\\n {src} \\\n /usr/include/stdio.h\n/usr/include/stdio.h :\n") + + def test_own_module_core_and_absolute_objects_pass(self): + self.dep("displayif/src/a.P", self.module / "src" / "a.c") + self.dep("py/gc.P", "../../py/gc.c") + self.dep("main.P", "main.c") + # A source outside every module keeps its whole path under BUILD. + self.dep(f"{str(self.root).lstrip('/')}/bindings/lv.P", self.root / "bindings" / "lv.c") + self.assertEqual(build_mp.foreign_sources(self.build, [self.module]), []) + + def test_a_stray_copy_beside_another_module_is_named(self): + stray = self.root / "displayif" / "src" / "a.c" + self.dep("displayif/src/a.P", stray) + self.dep("displayif/src/b.P", self.module / "src" / "b.c") + found = build_mp.foreign_sources(self.build, [self.module, self.root / "audiodsp"]) + self.assertEqual(found, [(self.build / "displayif/src/a.P", str(stray))]) + + def test_a_module_reached_through_a_link_is_its_own(self): + link = self.root / "repo" / "modules" / "linked" + link.symlink_to(self.module, target_is_directory=True) + self.dep("displayif/src/a.P", self.module / "src" / "a.c") + self.assertEqual(build_mp.foreign_sources(self.build, [link]), []) + + +class VpathSearchesEachModuleFirst(unittest.TestCase): + """make finds a user C module's sources in its own directory, not in a + directory of the same name beside another module's parent (an older + checkout of it). Upstream's search list holds every module's parent, + sorted, so a parent that sorts first wins; patches/micropython/0040 puts + each module's own directory first. + + Needs micropython/ prepared with the overlay (any build does it); skipped + without one, unless PYDEVICES_REQUIRE_MICROPYTHON is set, as CI does.""" + + def mp(self): + mp = Path(build_mp.__file__).resolve().parent / "micropython" + why = None + if os.name == "nt": + why = "the unix port's Makefile on native Windows" + elif not (mp / "py" / "py.mk").is_file(): + why = "no micropython/ checkout" + elif "The PyDevices overlay applied" not in build_mp.git_out(mp, "log", "-1", "--format=%s"): + why = "micropython/ is not prepared with the overlay (run a build, or patches/apply_patches.py)" + if why: + if os.environ.get("PYDEVICES_REQUIRE_MICROPYTHON"): + self.fail(why) + self.skipTest(why) + return mp + + def resolve(self, mp, layout): + """Where make finds mod/x.c, in the unix port, for a build of `sib` and `repo/modules/mod`.""" + probe = layout / "probe.mk" + probe.write_text('include Makefile\nvpath-probe: mod/x.c\n\t@echo "PROBE $<"\n') + mods = f"{layout / 'sib'} {layout / 'repo' / 'modules' / 'mod'}" + out = subprocess.run(["make", "-s", "-C", str(mp / "ports" / "unix"), "-f", str(probe), + f"BUILD={layout / 'build'}", "FROZEN_MANIFEST=", f"USER_C_MODULES={mods}", + "vpath-probe"], capture_output=True, text=True, timeout=60) + got = [l[6:] for l in out.stdout.splitlines() if l.startswith("PROBE ")] + self.assertEqual(len(got), 1, out.stdout + out.stderr) + return Path(got[0]) + + def test_the_module_own_copy_wins(self): + mp = self.mp() + with tempfile.TemporaryDirectory() as tmp: + t = Path(tmp) + (t / "sib").mkdir() + (t / "sib" / "micropython.mk").write_text("# puts its parent on the search list\n") + ours = t / "repo" / "modules" / "mod" + ours.mkdir(parents=True) + (ours / "micropython.mk").write_text("SRC_USERMOD_C += $(USERMOD_DIR)/x.c\n") + (ours / "x.c").write_text("int ours;\n") + self.assertEqual(self.resolve(mp, t), ours / "x.c") # control: nothing stray + (t / "mod").mkdir() + (t / "mod" / "x.c").write_text("#error stray copy\n") # beside sib, sorted first + self.assertEqual(self.resolve(mp, t), ours / "x.c") + + class MicropythonLibPatches(unittest.TestCase): """patches/micropython-lib/ goes onto the submodule once, and a second pass leaves it be."""