8000 esp32: Fix component manager issue with "make submodules" by projectgus · Pull Request #16581 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

esp32: Fix component manager issue with "make submodules" #16581

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 3 commits into from
Jan 29, 2025
Merged

esp32: Fix component manager issue with "make submodules" #16581

merged 3 commits into from
Jan 29, 2025

Conversation

projectgus
Copy link
Contributor
@projectgus projectgus commented Jan 14, 2025 •
edited
Loading

Summary

This is an alternative version of #16549. I was testing it at @hmaerki's request and ended up with a slightly different approach.

The underlying bug is that some older versions of IDF Component Manager (definitely 1.4.2, maybe others) will leave the build and managed_components directories in an inconsistent state if CMake exits early. Running make submodules causes this early exit from CMake. The make submodules step appears to succeed, but a subsequent build with the same build directory fails with an error similar to:

CMake Error at /opt/esp/idf/tools/cmake/build.cmake:544 (message):
  ERROR: Some components (espressif/tinyusb) in the "managed_components"
  directory were modified on the disk since the last run of the CMake.
  Content of this directory is managed automatically.

  If you want to keep the changes, you can move the directory with the
  component to the "components"directory of your project.

  I.E.  for "espressif__tinyusb" run:

  mv /micropython/ports/esp32/managed_components/espressif__tinyusb
  /micropython/ports/esp32/components/espressif__tinyusb

  Or, if you want to discard the changes remove the ".component_hash" file
  from the component's directory.

  I.E.  for "espressif__tinyusb" run:

  rm
  /micropython/ports/esp32/managed_components/espressif__tinyusb/.component_hash


Call Stack (most recent call first):
  /opt/esp/idf/tools/cmake/project.cmake:605 (idf_build_process)
  CMakeLists.txt:68 (project)

Fix implemented as three commits:

  1. Disable the component manager during the run of idf.py that collects the GIT_SUBMODULES list. This still completes successfully, and is a bit less heavy-handed (and faster) than deleting the managed_components directory afterwards.
  2. Don't add the TinyUSB source files to a build which is only echoing submodules. This check is already applied elsewhere, was missed here.
  3. As @hmaerki suggested in the linked PR, when the GIT_SUBMODULES run of CMake fails then it's possible for execution to appear to complete cleanly, but with an empty list of submodules. Then it fails at some later step. Add a flag to have it fail immediately in these cases.

Testing

The latest IDF component manager doesn't exhibit the bug, so similar to @hmaerki I ran the build in a clean container. For speed I cross-mounted my local .git directory and then cloned from it:

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 BRANCHNAME /gitdir micropython && make -j16 -C micropython/mpy-cross && make -C micropython/ports/esp32 submodules BOARD=ESP32_GENERIC_S3 V=1 && make -C micropython/ports/esp32 BOARD=ESP32_GENERIC_S3"

This command fails with the error above when BRANCHNAME is master but succeeds for this branch.

@projectgus
Copy link
Contributor Author

@hmaerki I was testing your PR and ended up finding another approach. Can you please check if this works for you?

@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

dpgeorge
dpgeorge reviewed Jan 14, 2025
View reviewed changes
@projectgus
esp32: Disable component manager when running 'make submodules'.
be3359e 
- ECHO_SUBMODULES=1 exits CMake early. With idf_component_manager 1.x this
  seems to leave the managed_components directory in a state that causes
  later builds to fail.

- Looks like the component manager isn't needed for this step, so disable
  it. This invocation logs a warning (not visible in normal output) but
  completes successfully and returns the correct list of submodules.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
@projectgus
esp32: Don't add TinyUSB files to an ECHO_SUBMODULES build.
d40c0ea 
Similar to other places, CMake will error out if this file
doesn't exist yet but we don't want this if we're only getting
the list of submodules.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
@projectgus
py/mkrules: Add GIT_SUBMODULES_FAIL_IF_EMPTY flag for CMake ports.
06c0de6 
The way CMake gathers the submodule list, it can quietly be empty
if the previous step fails. This makes it an explicit error.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
@projectgus projectgus force-pushed the bugfix/esp32_components_submodules branch from 6a959bc to 06c0de6 Compare January 14, 2025 05:42
@codecov Codecov
Copy link
codecov bot commented Jan 14, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.58%. Comparing base (1b4c969) to head (06c0de6).
Report is 47 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master   #16581   +/-   ##
=======================================
  Coverage   98.58%   98.58%           
=======================================
  Files         167      167           
  Lines       21596    21596           
=======================================
  Hits        21291    21291           
  Misses        305      305

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

@hmaerki
Copy link
Contributor
hmaerki commented Jan 23, 2025

@hmaerki I was testing your PR and ended up finding another approach. Can you please check if this works for you?

I like your PR - thanks a lot!

I successfully tested (with and without your fix):

  • docker run -t docker.io/espressif/idf:v5.2.2 bash -c "git clone https://github.com/projectgus/micropython.git && git -C micropython checkout bugfix/esp32_components_submodules && make -C micropython/mpy-cross && make -C micropython/ports/esp32 submodules BOARD=ESP32_GENERIC_S3 && make -C micropython/ports/esp32 BOARD=ESP32_GENERIC_S3"
  • mpbuild build ESP32_GENERIC_S3

I will close now my initial #16549 in favor of this PR.

@hmaerki hmaerki mentioned this pull request Jan 23, 2025
Closed
dpgeorge
dpgeorge approved these changes Jan 23, 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.

Thanks @hmaerki for testing this PR. Let's merge it.

@dpgeorge dpgeorge added this to the release-1.25.0 milestone Jan 23, 2025
@projectgus projectgus merged commit 22353e9 into micropython:master Jan 29, 2025
64 checks passed
@projectgus projectgus deleted the bugfix/esp32_components_submodules branch January 29, 2025 00:41
@kdschlosser kdschlosser mentioned this pull request Feb 28, 2025
Closed
This was referenced Mar 11, 2025
Closed
Merged
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

Assignees
No one assigned
Labels
port-esp32
Projects
None yet
Milestone
release-1.25.0
Development

Successfully merging this pull request may close these issues.
3 participants
@projectgus @hmaerki @dpgeorge
0