8000 rp2,esp32,extmod: Implement submodule update inside CMake. by projectgus · Pull Request #16907 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

rp2,esp32,extmod: Implement submodule update inside CMake. #16907

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 27, 2025
Merged

rp2,esp32,extmod: Implement submodule update inside CMake. #16907

merged 2 commits into from
Mar 27, 2025

Conversation

projectgus
Copy link
Contributor
@projectgus projectgus commented Mar 11, 2025 •
edited
Loading

Summary

Alternative approach to #16901. Fixes #16870.

Rather than having make submodules run CMake to get the submodules list and then update git submodules, have CMake update submodules itself. This means any other build errors will be visible, rather than being masked (as currently the CMake submodules run has its output captured and parsed.)

This effectively reverts commit 22353e9 from #16581, as an empty submodule list is no longer a symptom of any deeper (masked) issue.

It's also possible to simplify out a lot of the checks for ECHO_SUBMODULES as CMake won't check if those files and directories exist until after the UPDATE_SUBMODULES pass finishes. A workaround is still needed for ESP32 as the ESP-IDF framework tests if paths exist when registering a new component, but the workaround can be contained in the esp32_common.cmake file.

This PR also contains a commit to fail with a helpful error message if the pico-sdk is missing. This was originally necessary because I was trying to run the UPDATE_SUBMODULES pass in the normal build directory (to save on running CMake twice), but that actually doesn't quite work as pico-sdk sets some other invalid cache values if directories are missing. However, the helpful error message is still helpful!

Testing

  • Ran some local rp2 and esp32 builds in a clean git clone, verified all successful.
  • Ran a build with the older IDF component manager in the IDF V5.2.2 docker image, verified still OK (but does still require the workaround from ec527a1). Command line: podman run --rm -t -v "/home/gus/ry/george/micropython/.git:/gitdir:ro" -t docker.io/espressif/idf:v5.2.2 bash -c "git clone -b $(git branch --show-current) /gitdir micropython && make -j -C micropython/mpy-cross && echo '@@@@@@@@@@ submodules' && make -C microp ython/ports/esp32 submodules V=1 BOARD=ESP32_GENERIC_S3 && echo '@@@@@@@@@@ main build' && make -C micropython/ports/esp32 BOARD=ESP32_GENERIC_S3"

Trade-offs and Alternatives

  • The make submodules output now includes a lot of CMake build output for a dummy run which never gets to actually build.
  • We could make this even simpler and hard-code the GIT_SUBMODULES value in rp2/Makefile and esp32/Makefile. The upside is a much simpler build stage. There are two downsides I can see:
    • Conceptually, this means each port has to know about implementation details (the submodule path) of all its external MP modules.
    • Practically, can no longer filter the list of submodules based on the board to reduce network traffic and disk usage. On ESP32 the submodule list is current the same for all boards so that doesn't have any current impact. On rp2 the list is much shorter for boards without Wi-Fi or networking, so some additional repos would be fetched unnecessarily. I'm not sure how much more traffic this would actually be, though.

@projectgus projectgus added port-esp32 extmod Relates to extmod/ directory in source port-rp2 labels Mar 11, 2025
@codecov Codecov
Copy link
codecov bot commented Mar 11, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.54%. Comparing base (2db0c02) to head (50da085).
Report is 2 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master   #16907   +/-   ##
=======================================
  Coverage   98.54%   98.54%           
=======================================
  Files         169      169           
  Lines       21890    21890           
=======================================
  Hits        21571    21571           
  Misses        319      319

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@projectgus projectgus force-pushed the bugfix/cmake_submodules branch from 535a3ac to 6cfa54b Compare March 11, 2025 07:06
@github-actions GitHub Actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

@projectgus projectgus force-pushed the bugfix/cmake_submodules branch 4 times, most recently from bc1d0c8 to 4169684 Compare March 11, 2025 07:50
@projectgus projectgus marked this pull request as ready for review March 11, 2025 23:36
@projectgus projectgus requested review from andrewleech and dpgeorge and removed request for andrewleech March 11, 2025 23:36
@dpgeorge dpgeorge added this to the release-1.25.0 milestone Mar 18, 2025
@projectgus @dpgeorge
rp2,esp32,extmod: Implement UPDATE_SUBMODULES in CMake.
cccac2c 
Rather than having Make calling CMake to generate a list of submodules and
then run a Make target (which is complex and prone to masking other
errors), implement the submodule update logic in CMake itself.

Internal CMake-side changes are that GIT_SUBMODULES is now a CMake list,
and the trigger variable name is changed from ECHO_SUBMODULES to
UPDATE_SUBMODULES.

The run is otherwise 100% a normal CMake run now, so most of the other
special casing can be removed.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
@projectgus @dpgeorge
rp2: Print an error message if pico-sdk submodule is missing.
50da085 
This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
@dpgeorge dpgeorge force-pushed the bugfix/cmake_submodules branch from 4169684 to 50da085 Compare March 27, 2025 06:52
dpgeorge
dpgeorge approved these changes Mar 27, 2025
View reviewed changes
Copy link
Member
@dpgeorge dpgeorge left a comment

Choose a reason for hiding this comment

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

This is nice improvement, definitely a lot cleaner this way (than printing the submodules and using make to updated them).

Tested with various rp2 boards, including ARDUINO_NANO_RP2040_CONNECT which needs mynewt-nimble.

@dpgeorge dpgeorge merged commit 50da085 into micropython:master Mar 27, 2025
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpgeorge dpgeorge dpgeorge approved these changes

@andrewleech andrewleech Awaiting requested review from andrewleech

Assignees
No one assigned
Labels
extmod Relates to extmod/ directory in source port-esp32 port-rp2
Projects
None yet
Milestone
release-1.25.0
Development

Successfully merging this pull request may close these issues.

make submodules fails in ports/rp2: Internal build error: The submodule list should not be empty.
2 participants
@projectgus @dpgeorge
0