Skip to content

Fix NXP MCUXpresso CIFAR link failure - #24510

Merged
JacobSzwejbka merged 1 commit into
mainfrom
codex/main-fix-nxp-sys-info-20261103
Nov 3, 2026
Merged

JacobSzwejbka merged 1 commit into
mainfrom
codex/main-fix-nxp-sys-info-20261103

Conversation

@JacobSzwejbka

Copy link
Copy Markdown
Contributor

Main-branch counterpart of release/1.6 fix #24509. Fixes the NXP MCUXpresso failure seen on #24490; main has the same incomplete CMake target.

The failed job reaches the final CIFAR firmware link, then fails with undefined reference to SYS_INFO_PrintAll. The SDK example calls this helper, but our standalone CMake target omits examples/eiq_examples/common/sys_info/sys_info.c.

Add that source to the firmware target. The helper also needs MCUXSDK_VERSION_FULL_STR, which the SDK's normal build supplies through a generated header. Our standalone target now uses the SDK's mcux_version.cmake parser and sets the equivalent compile definition from the checked-out SDK version.

Verification

A focused host CMake build reused the actual example target's source list and include paths, the real SDK helper/header, and the SDK version parser. The board/runtime sources were replaced with empty fixtures and the application with a small C++ caller.

The original source list reproduced the unresolved SYS_INFO_PrintAll link error. Adding only the helper reproduced the missing MCUXSDK_VERSION_FULL_STR compile error. With both changes, the executable linked and printed MCUX SDK version: 2026.12.00-pvw1.

git diff --check passed. The full ARM firmware build and execution on hardware were not run locally; the NXP CI job must validate the complete build.

Only the example's CMakeLists.txt changes (eight added lines). The resulting file is byte-identical to the verified release fix in #24509. Land this on main; #24509 is the corresponding release/1.6 pick.

Prepared with OpenAI Codex.

Compile the SDK sys_info implementation called by the CIFAR example. Use the SDK version parser to provide the version-string macro that the SDK normally supplies through its generated build header.

Prepared with OpenAI Codex.
@pytorch-bot

pytorch-bot Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24510

Note: Links to docs will display an error until the docs builds have been completed.

⏳ No Failures, 108 Pending

As of commit 4ec4890 with merge base 7da27ca (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 3, 2026
@shoumikhin

Copy link
Copy Markdown
Contributor

@claude review effort=max

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@claude

claude Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Claude finished @shoumikhin's task in 16m 47s —— View job


The 8-line fix matches the link failure in CI, but the MCUXpresso SDK is still downloaded unpinned and this change ties the target more tightly to the current preview SDK. I'd approve once nxp-mcuxpresso-test passes, and pin the SDK in a follow-up.

Detailed review

PR Review: #24510

Summary

The PR adds the SDK's sys_info.c to the CIFAR firmware target and defines MCUXSDK_VERSION_FULL_STR using the SDK's mcux_version.cmake. That addresses the undefined reference to `SYS_INFO_PrintAll' at main.cpp:(.text.main+0x1a) in the failed job's log (line 37563).

The problem is the root cause, which this PR leaves in place: the SDK is downloaded unpinned. The next NXP change to its example can break the job again, and this PR adds a dependency on an internal CMake module of the SDK's build system.

What this review could not check:

  • The runner has no network access, so I couldn't read sys_info.c or mcux_version.cmake.
  • nxp-mcuxpresso-test for this PR was still running when I posted. It is the only real ARM build of this change.

Infrastructure

The SDK is unpinned, so this fixes today's break while tying the build more tightly to a moving target (examples/nxp/mcuxpresso/imxrt700/executorch_cifarnet/CMakeLists.txt:61, :88)

  • No pin. west init runs without --mr (test_build_from_scratch.sh:40, docs/source/backends/nxp/nxp-mcuxpresso-example.md:43), so CI builds whatever is on the manifest's default branch. The PR's own check printed 2026.12.00-pvw1, which looks like a preview build. The job log doesn't record which revisions update_board checked out (log lines 17671–17687).

  • The SDK decides what this target must compile. main.cpp comes from the SDK, but the list of sources to compile is kept here. Any NXP change to that example can break nxp-mcuxpresso-test again without any ExecuTorch change: a new helper, another macro from the SDK's generated header, or a moved file.

  • The blast radius is wide. The job runs on pull requests and on pushes to main and release/* (.github/workflows/pull.yml:3-8). release/1.6 gets this same file through [release/1.6] Fix NXP MCUXpresso CIFAR link failure #24509.

  • This PR adds two more SDK dependencies:

    • the common/sys_info/ path;
    • the internal build module cmake/extension/mcux_version.cmake and its MCUXSDK_VERSION_FULL variable. If NXP moves or renames that module, configure fails at line 88.
  • Older checkouts can now fail too. test_build_from_scratch.sh:38 reuses an existing mcuxpresso-sdk/ without running west update. If that checkout is missing either file, a build that used to link now fails with include could not find requested file (configure) or Cannot find source file (generate). I couldn't check which SDK releases include these files.

  • Suggested follow-up:

    • Pin with west init --mr <tested tag or SHA> in both the script and the doc. From then on, update the pin and this source list in the same PR.
    • If you'd rather not pin, at least run west list in the job so the next break can be bisected.
    • Note that examples/nxp/README.md:48 already says to use SDK v26.06.00, so the repo now points to two different SDK versions.

    Fix this →

Code Quality

  • docs/source/backends/nxp/build_flow.svg:116: the "Example App Sources" box still lists only main.cpp RegisterKernels.cpp timer.c under eiq_examples/executorch_cifarnet/, and it is now missing sys_info.c. It also shows timer.c in the wrong place: per CMakeLists.txt:60 it comes from eiq_examples/common/. Minor. Fix this →

Recommendation

Approve once nxp-mcuxpresso-test passes on this PR. Nothing in the 8 changed lines blocks merging. The SDK pin can wait for a follow-up, but it is the only thing that stops this from recurring on main and release/1.6.

Task list
  • Load /pr-review skill and repo guidelines
  • Read the diff, the full CMakeLists.txt, the build scripts, the nxp-mcuxpresso-test job, and the example docs
  • Check the failed job log from [release/1.6] Delete duplicate CUDA output handles once #24490
  • Run 3 independent reviewers
  • Merge duplicate findings and re-check each one against the code
  • Post review

What I didn't do: I couldn't read the SDK's sys_info.c or mcux_version.cmake because the runner has no network access. I didn't wait for this PR's nxp-mcuxpresso-test job, which was still running and took about 70 minutes on the failed run. I didn't do a local ARM build.

@JacobSzwejbka
JacobSzwejbka merged commit ee4b85b into main Nov 3, 2026
251 of 255 checks passed
@JacobSzwejbka
JacobSzwejbka deleted the codex/main-fix-nxp-sys-info-20261103 branch November 3, 2026 17:36

This branch was previously deployed

1 inactive deployment
cadence — 4ec48909 Deployed Nov 3, 2026 by JacobSzwejbka via hifi-op-test / hifi4 #36287
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants