Update gcc 14.2 - fix [workaround] for mbedTLS #3197
Conversation
WalkthroughThe changes update the ARM GNU GCC toolchain version from 13.3.rel1 to 14.2.rel1 across multiple Dockerfiles, build scripts, and CI templates. Several base Docker image tags are also incremented. Additionally, a compiler flag is added for the MbedTLS build, and a new code signing signature block is introduced in a PowerShell script. Minor C++ cast style improvements were made in CLR interop code. Changes
Sequence Diagram(s)sequenceDiagram
participant DevContainer as Dev Container Build
participant ARMDownload as ARM Toolchain Server
DevContainer->>ARMDownload: Download ARM GCC toolchain (14.2.rel1)
ARMDownload-->>DevContainer: Provide toolchain archive
DevContainer->>DevContainer: Extract and install GCC 14.2.rel1
sequenceDiagram
participant CI as CI Pipeline
participant ARMDownload as ARM Toolchain Server
CI->>ARMDownload: Download ARM GCC toolchain (14.2.rel1)
ARMDownload-->>CI: Provide toolchain archive
CI->>CI: Cache and install GCC 14.2.rel1
sequenceDiagram
participant Build as CMake Build
participant MbedTLS as mbedcrypto Target
Build->>MbedTLS: Compile with -fomit-frame-pointer flag
MbedTLS-->>Build: Build output with new compiler option
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
networkfusion
changed the title
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
networkfusion
added
Area: Config-and-Build
Area: Dev-Containers
and removed
invalid
labels
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
.devcontainer/All/Dockerfile.All.SRC (1)
12-18: Add integrity checking for the downloaded toolchain.Bumping to GCC 14.2 is fine, but the Dockerfile still pipes an unauthenticated
curl | tarsequence.
For reproducible and secure builds, download the SHA-256 file Arm publishes and
verify it (or embed the digest in the Dockerfile and usesha256sum -c).&& curl -L -o /tmp/dc-downloads/gcc-arm.tar.xz $GCC_URI \ +&& echo "<expected_SHA256> /tmp/dc-downloads/gcc-arm.tar.xz" | sha256sum -c - \.devcontainer/TI/Dockerfile.TI.SRC (1)
14-17: Fail fast on bad downloads
curlcurrently writes whatever it receives (including a 404 HTML page) and the build only explodes later attar.
Add standard fail/redirect flags so the layer aborts immediately when the URL is wrong.- && curl -o /tmp/dc-downloads/gcc-arm.tar.xz $GCC_URI \ + && curl --fail --location --silent --show-error \ + -o /tmp/dc-downloads/gcc-arm.tar.xz "$GCC_URI" \
♻️ Duplicate comments (3)
.devcontainer/AzureRTOS/Dockerfile.AzureRTOS.SRC (1)
12-18: Same integrity-check concern as in Dockerfile.AllThe download sequence is identical — please apply the SHA-256 verification
suggested in the main comment..devcontainer/ChibiOS/Dockerfile.ChibiOS.SRC (1)
12-18: Same integrity-check concern as in Dockerfile.AllThe download sequence is identical — please apply the SHA-256 verification
suggested in the main comment..devcontainer/FreeRTOS-NXP/Dockerfile.FreeRTOS-NXP.SRC (1)
12-18: Same integrity-check concern as in Dockerfile.AllThe download sequence is identical — please apply the SHA-256 verification
suggested in the main comment.
🧹 Nitpick comments (1)
azure-pipelines-templates/download-install-arm-gcc-toolchain.yml (1)
14-15: Nit: unify cache-key quotingOther pipeline templates use single quotes for literal cache keys (see retrieved learning). Swapping to single quotes avoids YAML escaping surprises.
key: 'gcc-14_2__rel1 | gccUpdateKey'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.devcontainer/All/Dockerfile.All(1 hunks).devcontainer/All/Dockerfile.All.SRC(1 hunks).devcontainer/AzureRTOS/Dockerfile.AzureRTOS(1 hunks).devcontainer/AzureRTOS/Dockerfile.AzureRTOS.SRC(1 hunks).devcontainer/ChibiOS/Dockerfile.ChibiOS(1 hunks).devcontainer/ChibiOS/Dockerfile.ChibiOS.SRC(1 hunks).devcontainer/FreeRTOS-NXP/Dockerfile.FreeRTOS-NXP(1 hunks).devcontainer/FreeRTOS-NXP/Dockerfile.FreeRTOS-NXP.SRC(1 hunks).devcontainer/TI/Dockerfile.TI.SRC(1 hunks)CMake/binutils.common.cmake(1 hunks)azure-pipelines-templates/download-install-arm-gcc-toolchain.yml(1 hunks)install-scripts/install-arm-gcc-toolchain.ps1(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
.devcontainer/AzureRTOS/Dockerfile.AzureRTOS (2)
Learnt from: AdrianSoundy
PR: nanoframework/nf-interpreter#3152
File: targets/ESP32/ESP32_P4/nanoCLR/nanoFramework.Graphics/Graphics_Memory.cpp:0-0
Timestamp: 2025-05-01T03:31:34.528Z
Learning: In the nanoFramework ESP32 codebase, assertions are preferred over graceful error handling for critical memory allocations (like graphics memory) where the system cannot function without the allocated resources.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3028
File: src/nanoFramework.Runtime.Native/nf_rt_native.h:49-49
Timestamp: 2024-10-10T01:50:45.514Z
Learning: In the nanoFramework project, naming conventions such as `nanoFrameworkRuntimeNativeRebootOption` follow an established pattern and are automatically generated.
CMake/binutils.common.cmake (8)
Learnt from: frobijn
PR: nanoframework/nf-interpreter#3023
File: CMake/Modules/FindNF_NativeAssemblies.cmake:95-106
Timestamp: 2024-10-08T15:52:09.445Z
Learning: The `AddCorLibAssemblyVersion` macro in `CMake/Modules/FindNF_NativeAssemblies.cmake` does not require additional error handling for regex failures because the associated *.cpp files contain warnings against modification. Any changes to these files should be accompanied by corresponding updates to the CMake file.
Learnt from: frobijn
PR: nanoframework/nf-interpreter#3023
File: CMake/Modules/FindNF_NativeAssemblies.cmake:95-106
Timestamp: 2024-09-23T17:56:37.570Z
Learning: The `AddCorLibAssemblyVersion` macro in `CMake/Modules/FindNF_NativeAssemblies.cmake` does not require additional error handling for regex failures because the associated *.cpp files contain warnings against modification. Any changes to these files should be accompanied by corresponding updates to the CMake file.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3026
File: src/CLR/Helpers/nanoprintf/nanoprintf.h:279-283
Timestamp: 2024-10-08T15:52:09.445Z
Learning: In `nanoprintf.h`, the `FEATURE_FLAGS` macro configuration is intended and should not be modified.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3026
File: src/CLR/Helpers/nanoprintf/nanoprintf.h:279-283
Timestamp: 2024-09-30T10:59:45.392Z
Learning: In `nanoprintf.h`, the `FEATURE_FLAGS` macro configuration is intended and should not be modified.
Learnt from: frobijn
PR: nanoframework/nf-interpreter#3023
File: CMake/Modules/FindNF_NativeAssemblies.cmake:88-109
Timestamp: 2024-10-08T15:52:09.445Z
Learning: In `CMake/Modules/FindNF_NativeAssemblies.cmake`, when similar macros like `AddNativeAssemblyVersion` and `AddCorLibAssemblyVersion` have different logic, prefer keeping them separate to maintain readability rather than refactoring into a shared function.
Learnt from: frobijn
PR: nanoframework/nf-interpreter#3023
File: CMake/Modules/FindNF_NativeAssemblies.cmake:88-109
Timestamp: 2024-09-23T17:59:04.080Z
Learning: In `CMake/Modules/FindNF_NativeAssemblies.cmake`, when similar macros like `AddNativeAssemblyVersion` and `AddCorLibAssemblyVersion` have different logic, prefer keeping them separate to maintain readability rather than refactoring into a shared function.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3026
File: src/CLR/Helpers/nanoprintf/nanoprintf.h:225-304
Timestamp: 2024-10-08T15:52:09.445Z
Learning: In `nanoprintf.h`, the `NANOFRAMEWORK_SET` configuration is intended to include all features as defined, even if some features are currently unused. Do not suggest removing or disabling unused features from `NANOFRAMEWORK_SET`.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3026
File: src/CLR/Helpers/nanoprintf/nanoprintf.h:225-304
Timestamp: 2024-10-03T16:00:20.409Z
Learning: In `nanoprintf.h`, the `NANOFRAMEWORK_SET` configuration is intended to include all features as defined, even if some features are currently unused. Do not suggest removing or disabling unused features from `NANOFRAMEWORK_SET`.
azure-pipelines-templates/download-install-arm-gcc-toolchain.yml (1)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3066
File: azure-pipelines-templates/checkout-idf.yml:0-0
Timestamp: 2025-01-16T15:12:49.859Z
Learning: In Azure DevOps pipelines, cache keys should use single quotes and literal values (e.g., 'esp-idf | 5_2_3') rather than direct variable interpolation. The syntax `'esp-idf | $(IDF_TAG)'` is incorrect for cache keys.
⏰ Context from checks skipped due to timeout of 90000ms (41)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
- GitHub Check: build-target (ST_STM32F769I_DISCOVERY, Debug, All)
- GitHub Check: build-target (TI_CC1352R1_LAUNCHXL, Debug, All, 915)
- GitHub Check: build-target (NXP_MIMXRT1060_EVK, Debug, All)
- GitHub Check: build-target (TI_CC1352R1_LAUNCHXL, Debug, TI, 915)
- GitHub Check: build-target (NXP_MIMXRT1060_EVK, Debug, FreeRTOS-NXP)
- GitHub Check: build-target (ESP32_H2_THREAD, Debug, ESP32)
- GitHub Check: build-target (ESP32_C6_THREAD, Debug, ESP32)
- GitHub Check: build-target (ESP32_C3, Debug, ESP32)
- GitHub Check: build-target (ESP32_S2_USB, Debug, ESP32)
- GitHub Check: build-target (ESP32_S3, Debug, ESP32)
- GitHub Check: build-target (ESP_WROVER_KIT, Debug, ESP32)
- GitHub Check: build-target (ST_STM32F769I_DISCOVERY, Debug, ChibiOS)
- GitHub Check: build-target (M5Core2, Debug, ESP32)
- GitHub Check: build-target (SL_STK3701A, Debug, All)
- GitHub Check: build-target (SL_STK3701A, Debug, AzureRTOS)
- GitHub Check: nf-interpreter (Nightly build) (Report_Build_Failure)
- GitHub Check: nf-interpreter (Build_Azure_RTOS_targets SL_STK3701A)
- GitHub Check: nf-interpreter (Build_WIN32_nanoCLR)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_ETHERNET_KIT_1.2)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_S3_ALL)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_H2_THREAD)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_C6_THREAD)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_C3)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_BLE_REV0)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F769I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_NXP_targets NXP_MIMXRT1060_EVK)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_PSRAM_REV0)
- GitHub Check: nf-interpreter (Build_TI_SimpleLink_targets TI_CC1352R1_LAUNCHXL_915)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F429I_DISCOVERY)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (8)
CMake/binutils.common.cmake (1)
652-653: Approve: Retain-fomit-frame-pointerworkaround until upstream fixThe mbedTLS issue #9875 (Error building with recent ARM GCC 14.2.Rel1) remains open as of July 5, 2025, so keeping this temporary workaround is appropriate.
• File: CMake/binutils.common.cmake (lines 652–653) – the
-fomit-frame-pointerflag on thembedcryptotarget
• Action: Monitor Mbed-TLS/mbedtls#9875 and remove this flag once the issue is resolved.devcontainer/All/Dockerfile.All (1)
1-1: LGTM! Version bump aligns with GCC toolchain update.The base image version increment from
v2.57tov2.58is consistent with the overall GCC 14.2 toolchain upgrade described in the PR objectives..devcontainer/AzureRTOS/Dockerfile.AzureRTOS (1)
1-1: LGTM! Consistent version update for AzureRTOS container.The base image version increment from
v1.34tov1.35is consistent with the coordinated container updates for the GCC 14.2 upgrade..devcontainer/ChibiOS/Dockerfile.ChibiOS (1)
1-1: LGTM! Consistent version update for ChibiOS container.The base image version increment from
v1.35tov1.36follows the coordinated update pattern for the GCC 14.2 toolchain upgrade..devcontainer/FreeRTOS-NXP/Dockerfile.FreeRTOS-NXP (1)
1-1: LGTM! Final coordinated version update for FreeRTOS-NXP container.The base image version increment from
v1.07tov1.08completes the coordinated container updates for the GCC 14.2 toolchain upgrade.install-scripts/install-arm-gcc-toolchain.ps1 (1)
15-16: No action needed: lowercase “rel1” URL is valid
Verified viacurl -Ithat the hard-coded URL using$Version = "14.2.rel1"returns an HTTP 302 redirect to the actual ZIP file, confirming the lowercase path works correctly..devcontainer/TI/Dockerfile.TI.SRC (1)
12-17: Download URI is correct – no changes neededVerified via a HEAD request that the existing lowercase
rel1URL returns HTTP/1.1 200 OK, so the download step will succeed as is.azure-pipelines-templates/download-install-arm-gcc-toolchain.yml (1)
7-8: Confirm archive casing for Windows buildThe subsequent steps expect
arm-gnu-toolchain-${{ parameters.gccArmVersion }}-mingw-w64-i686-arm-none-eabi.zip.
For 14.2 the official filename usesRel1(capital R). A mismatch will break both the download PS script and theExtractFilestask. Please double-check.
Speculative fix (may need improvement).
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Swapping to single quotes avoids YAML escaping surprises.
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
.devcontainer/ChibiOS/Dockerfile.ChibiOS.SRC (1)
12-18: Add checksum verification for the downloaded archive.Blindly piping the tarball into
tarprovides no integrity guarantee and leaves the build susceptible to upstream compromise or corruption. A simple SHA-256 check avoids this.- && curl -o /tmp/dc-downloads/gcc-arm.tar.xz $GCC_URI \ + && curl -o /tmp/dc-downloads/gcc-arm.tar.xz $GCC_URI \ + && echo "e0d... /tmp/dc-downloads/gcc-arm.tar.xz" | sha256sum -c - \(Replace the hash with the official value from the ARM release notes.)
.devcontainer/TI/Dockerfile.TI.SRC (1)
12-18: Add archive hash verification as well (TI image).Mirrors the earlier integrity advice for ChibiOS.
♻️ Duplicate comments (2)
.devcontainer/AzureRTOS/Dockerfile.AzureRTOS.SRC (1)
12-18: Same GCC version duplication noted in.devcontainer/AllSee earlier comment; the refactor applies here too.
.devcontainer/FreeRTOS-NXP/Dockerfile.FreeRTOS-NXP.SRC (1)
12-18: Same GCC version duplication noted in.devcontainer/AllSee earlier comment; the refactor applies here too.
🧹 Nitpick comments (4)
install-scripts/install-arm-gcc-toolchain.ps1 (1)
104-344: Signature blob churn – keep commit size minimalOnly the GNU-GCC version changed, yet the whole Authenticode block was replaced.
If this is unintended, re-sign only when the script logic actually changes to keep diffs readable..devcontainer/All/Dockerfile.All.SRC (1)
12-18: Parameterise GCC version once instead of duplicating per Dockerfile
GCC_VERSION=14.2.rel1is now copied across ~5 Dockerfiles.
Centralising this (e.g. via an ARG or build-time--build-arg GCC_VERSIONpassed from CI) avoids future N-way edits.Example (high-level):
-ARG GCC_VERSION=14.2.rel1 +ARG GCC_VERSION=${GCC_VERSION:-14.2.rel1}Then set
--build-arg GCC_VERSIONin the build matrix.azure-pipelines-templates/download-install-arm-gcc-toolchain.yml (2)
7-15: Hard-coding the cache key will require another manual edit next time.Derive the key from the parameter instead of embedding the version string. This keeps the template single-source-of-truth and avoids accidental version/key drift.
- key: "gcc-14_2__rel1 | gccUpdateKey" + # Convert dots to underscores for the cache key + key: $'gcc-${{ replace(parameters.gccArmVersion, ".", "_") }} | gccUpdateKey'
14-15: Follow pipeline cache-key quoting conventions.Prior guidance from the repo (see learning note) recommends single quotes for literal keys. Switching avoids YAML parsing edge cases with the pipe
|.- key: "gcc-14_2__rel1 | gccUpdateKey" + key: 'gcc-14_2__rel1 | gccUpdateKey'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.devcontainer/All/Dockerfile.All(1 hunks).devcontainer/All/Dockerfile.All.SRC(1 hunks).devcontainer/AzureRTOS/Dockerfile.AzureRTOS(1 hunks).devcontainer/AzureRTOS/Dockerfile.AzureRTOS.SRC(1 hunks).devcontainer/ChibiOS/Dockerfile.ChibiOS(1 hunks).devcontainer/ChibiOS/Dockerfile.ChibiOS.SRC(1 hunks).devcontainer/FreeRTOS-NXP/Dockerfile.FreeRTOS-NXP(1 hunks).devcontainer/FreeRTOS-NXP/Dockerfile.FreeRTOS-NXP.SRC(1 hunks).devcontainer/TI/Dockerfile.TI.SRC(1 hunks)CMake/binutils.common.cmake(1 hunks)azure-pipelines-templates/download-install-arm-gcc-toolchain.yml(1 hunks)install-scripts/install-arm-gcc-toolchain.ps1(3 hunks)src/CLR/Core/CLR_RT_Interop.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
.devcontainer/AzureRTOS/Dockerfile.AzureRTOS (2)
Learnt from: AdrianSoundy
PR: nanoframework/nf-interpreter#3152
File: targets/ESP32/ESP32_P4/nanoCLR/nanoFramework.Graphics/Graphics_Memory.cpp:0-0
Timestamp: 2025-05-01T03:31:34.528Z
Learning: In the nanoFramework ESP32 codebase, assertions are preferred over graceful error handling for critical memory allocations (like graphics memory) where the system cannot function without the allocated resources.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3028
File: src/nanoFramework.Runtime.Native/nf_rt_native.h:49-49
Timestamp: 2024-10-10T01:50:45.514Z
Learning: In the nanoFramework project, naming conventions such as `nanoFrameworkRuntimeNativeRebootOption` follow an established pattern and are automatically generated.
CMake/binutils.common.cmake (8)
Learnt from: frobijn
PR: nanoframework/nf-interpreter#3023
File: CMake/Modules/FindNF_NativeAssemblies.cmake:95-106
Timestamp: 2024-10-08T15:52:09.445Z
Learning: The `AddCorLibAssemblyVersion` macro in `CMake/Modules/FindNF_NativeAssemblies.cmake` does not require additional error handling for regex failures because the associated *.cpp files contain warnings against modification. Any changes to these files should be accompanied by corresponding updates to the CMake file.
Learnt from: frobijn
PR: nanoframework/nf-interpreter#3023
File: CMake/Modules/FindNF_NativeAssemblies.cmake:95-106
Timestamp: 2024-09-23T17:56:37.570Z
Learning: The `AddCorLibAssemblyVersion` macro in `CMake/Modules/FindNF_NativeAssemblies.cmake` does not require additional error handling for regex failures because the associated *.cpp files contain warnings against modification. Any changes to these files should be accompanied by corresponding updates to the CMake file.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3026
File: src/CLR/Helpers/nanoprintf/nanoprintf.h:279-283
Timestamp: 2024-10-08T15:52:09.445Z
Learning: In `nanoprintf.h`, the `FEATURE_FLAGS` macro configuration is intended and should not be modified.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3026
File: src/CLR/Helpers/nanoprintf/nanoprintf.h:279-283
Timestamp: 2024-09-30T10:59:45.392Z
Learning: In `nanoprintf.h`, the `FEATURE_FLAGS` macro configuration is intended and should not be modified.
Learnt from: frobijn
PR: nanoframework/nf-interpreter#3023
File: CMake/Modules/FindNF_NativeAssemblies.cmake:88-109
Timestamp: 2024-10-08T15:52:09.445Z
Learning: In `CMake/Modules/FindNF_NativeAssemblies.cmake`, when similar macros like `AddNativeAssemblyVersion` and `AddCorLibAssemblyVersion` have different logic, prefer keeping them separate to maintain readability rather than refactoring into a shared function.
Learnt from: frobijn
PR: nanoframework/nf-interpreter#3023
File: CMake/Modules/FindNF_NativeAssemblies.cmake:88-109
Timestamp: 2024-09-23T17:59:04.080Z
Learning: In `CMake/Modules/FindNF_NativeAssemblies.cmake`, when similar macros like `AddNativeAssemblyVersion` and `AddCorLibAssemblyVersion` have different logic, prefer keeping them separate to maintain readability rather than refactoring into a shared function.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3026
File: src/CLR/Helpers/nanoprintf/nanoprintf.h:225-304
Timestamp: 2024-10-08T15:52:09.445Z
Learning: In `nanoprintf.h`, the `NANOFRAMEWORK_SET` configuration is intended to include all features as defined, even if some features are currently unused. Do not suggest removing or disabling unused features from `NANOFRAMEWORK_SET`.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3026
File: src/CLR/Helpers/nanoprintf/nanoprintf.h:225-304
Timestamp: 2024-10-03T16:00:20.409Z
Learning: In `nanoprintf.h`, the `NANOFRAMEWORK_SET` configuration is intended to include all features as defined, even if some features are currently unused. Do not suggest removing or disabling unused features from `NANOFRAMEWORK_SET`.
azure-pipelines-templates/download-install-arm-gcc-toolchain.yml (1)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3066
File: azure-pipelines-templates/checkout-idf.yml:0-0
Timestamp: 2025-01-16T15:12:49.859Z
Learning: In Azure DevOps pipelines, cache keys should use single quotes and literal values (e.g., 'esp-idf | 5_2_3') rather than direct variable interpolation. The syntax `'esp-idf | $(IDF_TAG)'` is incorrect for cache keys.
src/CLR/Core/CLR_RT_Interop.cpp (3)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-09-25T11:28:38.536Z
Learning: When working with `nanoCLR_GetNativeAssemblyInformation`, fixed-size assembly names are required, so code that deals with variable-length names cannot be used.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-10-12T19:00:39.000Z
Learning: When working with `nanoCLR_GetNativeAssemblyInformation`, fixed-size assembly names are required, so code that deals with variable-length names cannot be used.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3190
File: src/CLR/Core/TypeSystem.cpp:0-0
Timestamp: 2025-06-26T09:16:55.184Z
Learning: In nanoFramework's CLR attribute parsing (src/CLR/Core/TypeSystem.cpp), the sentinel value 0xFFFF in string tokens represents a null string. When encountered, this should result in a true null reference (using SetObjectReference(nullptr)) rather than an empty string instance, and the boxing operation should be skipped via early return.
⏰ Context from checks skipped due to timeout of 90000ms (23)
- GitHub Check: nf-interpreter (Report_Build_Failure)
- GitHub Check: nf-interpreter (Run_UnitTests_mscorlib)
- GitHub Check: nf-interpreter (Build_nanoCLR_CLI)
- GitHub Check: nf-interpreter (Nightly build) (Report_Build_Failure)
- GitHub Check: nf-interpreter (Build_WIN32_nanoCLR)
- GitHub Check: nf-interpreter (Build_Azure_RTOS_targets SL_STK3701A)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_ETHERNET_KIT_1.2)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_S3_ALL)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_H2_THREAD)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_C6_THREAD)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_C3)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_BLE_REV0)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F769I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_NXP_targets NXP_MIMXRT1060_EVK)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F429I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_TI_SimpleLink_targets TI_CC1352R1_LAUNCHXL_915)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_PSRAM_REV0)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (11)
CMake/binutils.common.cmake (1)
652-653: Temporary workaround properly documented and tracked.The addition of
-fomit-frame-pointerflag with a clear comment referencing the upstream issue is a good implementation of the temporary fix. The workaround is appropriately scoped to only thembedcryptotarget.src/CLR/Core/CLR_RT_Interop.cpp (3)
894-894: Improved type safety with explicit C++ cast.The replacement of C-style cast with
reinterpret_castfollowed by dereference improves type safety and resolves GCC 14.2 compilation errors. This is the correct approach for reinterpreting memory layout of 64-bit numeric types.
928-928: Consistent cast style improvement.Same improvement as UINT64 field - using explicit
reinterpret_castfor better type safety and GCC 14.2 compatibility.
950-950: Double field cast improvement.The pattern is consistently applied to the double field accessor, maintaining type safety across all 64-bit field access functions.
.devcontainer/ChibiOS/Dockerfile.ChibiOS (1)
1-1: Routine base image version update.The version bump from v1.35 to v1.36 aligns with the broader development container environment updates and GCC toolchain upgrade.
.devcontainer/AzureRTOS/Dockerfile.AzureRTOS (1)
1-1: Base image version update for consistency.The version bump from v1.34 to v1.35 maintains consistency with the development environment updates and GCC toolchain upgrade.
.devcontainer/All/Dockerfile.All (1)
1-1: Coordinated development environment update.The version bump from v2.57 to v2.58 completes the coordinated upgrade across all development containers, ensuring consistent GCC 14.2 toolchain support.
.devcontainer/FreeRTOS-NXP/Dockerfile.FreeRTOS-NXP (1)
1-1: Unable to verifyv1.08on GHCR (HTTP 401)
GHCR’s tag API requires authentication, so our automated check againstnanoframework/dev-container-freertos-nxp:v1.08returned a 401 and is inconclusive. A missing or yanked tag will break all CI/dev-container spins.• File:
.devcontainer/FreeRTOS-NXP/Dockerfile.FreeRTOS-NXP(line 1)FROM ghcr.io/nanoframework/dev-container-freertos-nxp:v1.08• Please manually confirm that the
v1.08tag is published in GitHub Container Registry.
• For extra stability, consider pinning to a digest, for example:FROM ghcr.io/nanoframework/dev-container-freertos-nxp@sha256:<digest>install-scripts/install-arm-gcc-toolchain.ps1 (1)
14-16: 32-bit (i686) Windows build is available for v14.2.rel1—original warning is incorrectWe’ve verified the hard-coded URL for
arm-gnu-toolchain-14.2.rel1-mingw-w64-i686-arm-none-eabi.zipreturns HTTP 200, so the 32-bit Windows package is indeed published. No changes to the install script are required; you can disregard the 32-bit host build concern.Likely an incorrect or invalid review comment.
.devcontainer/ChibiOS/Dockerfile.ChibiOS.SRC (1)
12-18: ✅ ARM toolchain URL verifiedHEAD request to
https://armkeil.blob.core.windows.net/developer/Files/downloads/gnu/14.2.rel1/binrel/arm-gnu-toolchain-14.2.rel1-x86_64-arm-none-eabi.tar.xzreturned HTTP/1.1 200 OK. Safe to merge..devcontainer/TI/Dockerfile.TI.SRC (1)
12-18: Same URL-existence concern applies here.Please run the same HEAD-request check (see previous comment) for the TI image to ensure the 14.2 toolchain URL is live.
It still seems a problem.
for CMSIS
networkfusion
force-pushed
the
update-gcc-14-2-tls
branch
2 times, most recently
from
b253270 to
c96135f
Compare
As of GCC 14.2.rel1 the top level structure from the windows zip has been removed.
networkfusion
force-pushed
the
update-gcc-14-2-tls
branch
from
c96135f to
ef14df8
Compare
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
.devcontainer/AzureRTOS/Dockerfile.AzureRTOS.SRC (1)
12-18: Pin GCC toolchain checksum and confirm strip-componentsThe 14.2.rel1 tarball does include a single top-level folder (
arm-gnu-toolchain-14.2.rel1-x86_64-arm-none-eabi), so you should keep--strip-components 1. You should also pin the download with its SHA-256 to prevent silent CI breakage.• SHA256:
62a63b981fe391a9cbad7ef51b17e49aeaa3e7b0d029b36ca1e9c3b2a9b78823Please update
.devcontainer/AzureRTOS/Dockerfile.AzureRTOS.SRCaccordingly:ARG GCC_URI=https://armkeil.blob.core.windows.net/developer/Files/downloads/gnu/$GCC_VERSION/binrel/arm-gnu-toolchain-$GCC_VERSION-x86_64-arm-none-eabi.tar.xz RUN mkdir -p /tmp/dc-downloads /tmp/dc-extracted/gcc \ - && curl -o /tmp/dc-downloads/gcc-arm.tar.xz $GCC_URI \ + && curl -L --fail -o /tmp/dc-downloads/gcc-arm.tar.xz $GCC_URI \ + && echo "62a63b981fe391a9cbad7ef51b17e49aeaa3e7b0d029b36ca1e9c3b2a9b78823 /tmp/dc-downloads/gcc-arm.tar.xz" | sha256sum -c - \ && xz -d /tmp/dc-downloads/gcc-arm.tar.xz \ && tar -xvf /tmp/dc-downloads/gcc-arm.tar -C /tmp/dc-extracted/gcc --strip-components 1 \ && rm -rf /tmp/dc-extracted/gcc/share/doc/ /tmp/dc-extracted/gcc/share/gcc-arm-none-eabi/samples/
♻️ Duplicate comments (4)
.devcontainer/ChibiOS/Dockerfile.ChibiOS.SRC (1)
12-18: Same checksum/strip-components concerns as AzureRTOS DockerfileEverything said for the AzureRTOS container applies here verbatim (14.2 archive layout & missing checksum).
Please apply the same hardening..devcontainer/TI/Dockerfile.TI.SRC (1)
12-18: Repeat the checksum & archive-layout safeguardsFor consistency across all dev-containers, add SHA-256 verification and confirm whether
--strip-components 1is still correct for GCC 14.2..devcontainer/FreeRTOS-NXP/Dockerfile.FreeRTOS-NXP.SRC (1)
12-18: GCC 14.2: add integrity check & verify tar depthSame rationale as previous Dockerfiles—add
--fail -Land SHA-256 guard, and re-evaluate--strip-components.azure-pipelines-nightly.yml (1)
745-746: Same Ninja duplication in Azure RTOS jobConsider the same removal or version-pinning decision here to avoid redundant work.
🧹 Nitpick comments (6)
.devcontainer/ChibiOS/Dockerfile.ChibiOS (1)
1-1: Consider pinning the base image by digest.
Bumping tov1.36is fine, but tags alone can mutate over time. Pinning to a SHA-256 digest locks reproducibility and thwarts supply-chain surprises..devcontainer/AzureRTOS/Dockerfile.AzureRTOS (1)
1-1: Same reproducibility concern – tag → digest.If deterministic builds are required, pin the
v1.35image to its immutable digest..devcontainer/All/Dockerfile.All (1)
1-1: Reproducibility / SBOM accuracy.
v2.58may shift under you. Pin by digest (or sign) to guarantee the exact toolchain & dependencies during future rebuilds.azure-pipelines-nightly.yml (1)
305-306: Ninja already present onwindows-latestrunners
windows-latestimages ship with Ninja 1.11 pre-installed inC:\ProgramData\chocolatey\lib\ninja; the extra download costs ~15 s per job.Unless you need a newer version or the template also sets caching variables, you can drop this step and shave minutes across 50+ matrix jobs.
.devcontainer/All/Dockerfile.All.SRC (1)
87-92: Nice split of FreeRTOS / CMSIS clones, but remember LFS pull
GIT_LFS_SKIP_SMUDGE=1keeps the image small, yet the required binary blobs (DSP headers, etc.) must be fetched during build or at runtime. Make sure your CMake flows callgit lfs pullwhen the paths are actually consumed, otherwise the build will fail in CI where these files are referenced.azure-pipelines-templates/download-install-arm-gcc-toolchain.yml (1)
14-15: Cache key spelling nitThe double underscore in
gcc-14_2__rel1is harmless but inconsistent with past keys (gcc-13_3_rel1). Consider a single underscore for readability:- key: 'gcc-14_2__rel1 | gccUpdateKey' + key: 'gcc-14_2_rel1 | gccUpdateKey'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.devcontainer/All/Dockerfile.All(1 hunks).devcontainer/All/Dockerfile.All.SRC(2 hunks).devcontainer/AzureRTOS/Dockerfile.AzureRTOS(1 hunks).devcontainer/AzureRTOS/Dockerfile.AzureRTOS.SRC(1 hunks).devcontainer/ChibiOS/Dockerfile.ChibiOS(1 hunks).devcontainer/ChibiOS/Dockerfile.ChibiOS.SRC(1 hunks).devcontainer/FreeRTOS-NXP/Dockerfile.FreeRTOS-NXP(1 hunks).devcontainer/FreeRTOS-NXP/Dockerfile.FreeRTOS-NXP.SRC(2 hunks).devcontainer/TI/Dockerfile.TI.SRC(1 hunks).github/workflows/devcontainer-all.yaml(1 hunks).github/workflows/devcontainer-azurertos.yaml(1 hunks).github/workflows/devcontainer-chibios.yaml(1 hunks).github/workflows/devcontainer-esp32.yml(1 hunks).github/workflows/devcontainer-freertos-nxp.yaml(1 hunks).github/workflows/devcontainer-ti.yaml(1 hunks)CMake/binutils.common.cmake(1 hunks)azure-pipelines-nightly.yml(2 hunks)azure-pipelines-templates/download-install-arm-gcc-toolchain.yml(2 hunks)azure-pipelines.yml(3 hunks)install-scripts/install-arm-gcc-toolchain.ps1(4 hunks)src/CLR/Core/CLR_RT_Interop.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
.devcontainer/AzureRTOS/Dockerfile.AzureRTOS (2)
Learnt from: AdrianSoundy
PR: nanoframework/nf-interpreter#3152
File: targets/ESP32/ESP32_P4/nanoCLR/nanoFramework.Graphics/Graphics_Memory.cpp:0-0
Timestamp: 2025-05-01T03:31:34.528Z
Learning: In the nanoFramework ESP32 codebase, assertions are preferred over graceful error handling for critical memory allocations (like graphics memory) where the system cannot function without the allocated resources.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3028
File: src/nanoFramework.Runtime.Native/nf_rt_native.h:49-49
Timestamp: 2024-10-10T01:50:45.514Z
Learning: In the nanoFramework project, naming conventions such as `nanoFrameworkRuntimeNativeRebootOption` follow an established pattern and are automatically generated.
CMake/binutils.common.cmake (8)
Learnt from: frobijn
PR: nanoframework/nf-interpreter#3023
File: CMake/Modules/FindNF_NativeAssemblies.cmake:95-106
Timestamp: 2024-10-08T15:52:09.445Z
Learning: The `AddCorLibAssemblyVersion` macro in `CMake/Modules/FindNF_NativeAssemblies.cmake` does not require additional error handling for regex failures because the associated *.cpp files contain warnings against modification. Any changes to these files should be accompanied by corresponding updates to the CMake file.
Learnt from: frobijn
PR: nanoframework/nf-interpreter#3023
File: CMake/Modules/FindNF_NativeAssemblies.cmake:95-106
Timestamp: 2024-09-23T17:56:37.570Z
Learning: The `AddCorLibAssemblyVersion` macro in `CMake/Modules/FindNF_NativeAssemblies.cmake` does not require additional error handling for regex failures because the associated *.cpp files contain warnings against modification. Any changes to these files should be accompanied by corresponding updates to the CMake file.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3026
File: src/CLR/Helpers/nanoprintf/nanoprintf.h:279-283
Timestamp: 2024-09-30T10:59:45.392Z
Learning: In `nanoprintf.h`, the `FEATURE_FLAGS` macro configuration is intended and should not be modified.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3026
File: src/CLR/Helpers/nanoprintf/nanoprintf.h:279-283
Timestamp: 2024-10-08T15:52:09.445Z
Learning: In `nanoprintf.h`, the `FEATURE_FLAGS` macro configuration is intended and should not be modified.
Learnt from: frobijn
PR: nanoframework/nf-interpreter#3023
File: CMake/Modules/FindNF_NativeAssemblies.cmake:88-109
Timestamp: 2024-09-23T17:59:04.080Z
Learning: In `CMake/Modules/FindNF_NativeAssemblies.cmake`, when similar macros like `AddNativeAssemblyVersion` and `AddCorLibAssemblyVersion` have different logic, prefer keeping them separate to maintain readability rather than refactoring into a shared function.
Learnt from: frobijn
PR: nanoframework/nf-interpreter#3023
File: CMake/Modules/FindNF_NativeAssemblies.cmake:88-109
Timestamp: 2024-10-08T15:52:09.445Z
Learning: In `CMake/Modules/FindNF_NativeAssemblies.cmake`, when similar macros like `AddNativeAssemblyVersion` and `AddCorLibAssemblyVersion` have different logic, prefer keeping them separate to maintain readability rather than refactoring into a shared function.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3026
File: src/CLR/Helpers/nanoprintf/nanoprintf.h:225-304
Timestamp: 2024-10-08T15:52:09.445Z
Learning: In `nanoprintf.h`, the `NANOFRAMEWORK_SET` configuration is intended to include all features as defined, even if some features are currently unused. Do not suggest removing or disabling unused features from `NANOFRAMEWORK_SET`.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3026
File: src/CLR/Helpers/nanoprintf/nanoprintf.h:225-304
Timestamp: 2024-10-03T16:00:20.409Z
Learning: In `nanoprintf.h`, the `NANOFRAMEWORK_SET` configuration is intended to include all features as defined, even if some features are currently unused. Do not suggest removing or disabling unused features from `NANOFRAMEWORK_SET`.
src/CLR/Core/CLR_RT_Interop.cpp (5)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-10-12T19:00:39.000Z
Learning: When working with `nanoCLR_GetNativeAssemblyInformation`, fixed-size assembly names are required, so code that deals with variable-length names cannot be used.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-09-25T11:28:38.536Z
Learning: When working with `nanoCLR_GetNativeAssemblyInformation`, fixed-size assembly names are required, so code that deals with variable-length names cannot be used.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3190
File: src/CLR/Core/TypeSystem.cpp:0-0
Timestamp: 2025-06-26T09:16:55.184Z
Learning: In nanoFramework's CLR attribute parsing (src/CLR/Core/TypeSystem.cpp), the sentinel value 0xFFFF in string tokens represents a null string. When encountered, this should result in a true null reference (using SetObjectReference(nullptr)) rather than an empty string instance, and the boxing operation should be skipped via early return.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-09-25T11:28:38.536Z
Learning: In `nanoCLR_GetNativeAssemblyInformation`, there is no need to return the number of bytes written, as the memory buffer is zeroed, making the string buffer terminated.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-10-08T15:52:09.445Z
Learning: In `nanoCLR_GetNativeAssemblyInformation`, there is no need to return the number of bytes written, as the memory buffer is zeroed, making the string buffer terminated.
azure-pipelines-templates/download-install-arm-gcc-toolchain.yml (1)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3066
File: azure-pipelines-templates/checkout-idf.yml:0-0
Timestamp: 2025-01-16T15:12:49.859Z
Learning: In Azure DevOps pipelines, cache keys should use single quotes and literal values (e.g., 'esp-idf | 5_2_3') rather than direct variable interpolation. The syntax `'esp-idf | $(IDF_TAG)'` is incorrect for cache keys.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: nf-interpreter (Report_Build_Failure)
- GitHub Check: nf-interpreter (Run_UnitTests_mscorlib)
- GitHub Check: nf-interpreter (Build_nanoCLR_CLI)
- GitHub Check: nf-interpreter (Build_WIN32_nanoCLR)
- GitHub Check: nf-interpreter (Build_Azure_RTOS_targets SL_STK3701A)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_ETHERNET_KIT_1.2)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_S3_ALL)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_H2_THREAD)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_C6_THREAD)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_C3)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_BLE_REV0)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F769I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_TI_SimpleLink_targets TI_CC1352R1_LAUNCHXL_915)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_PSRAM_REV0)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F429I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_NXP_targets NXP_MIMXRT1060_EVK)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (15)
CMake/binutils.common.cmake (1)
652-653: LGTM! Temporary workaround properly documented.The addition of
-fomit-frame-pointerto the mbedcrypto target compilation options is a reasonable temporary workaround for the GCC 14.2 compatibility issue. The comment clearly indicates this is temporary and references the specific issue that needs to be resolved.src/CLR/Core/CLR_RT_Interop.cpp (3)
894-894: Excellent improvement to use explicitreinterpret_castfor type conversion.This change replaces a C-style cast with an explicit
reinterpret_cast, which is the correct C++ approach for this low-level casting operation. The explicit cast makes the intention clear and addresses GCC 14.2 compilation warnings about casting user-defined numeric types.
928-928: Excellent improvement to use explicitreinterpret_castfor type conversion.This change replaces a C-style cast with an explicit
reinterpret_cast, which is the correct C++ approach for this low-level casting operation. The explicit cast makes the intention clear and addresses GCC 14.2 compilation warnings about casting user-defined numeric types.
950-950: Excellent improvement to use explicitreinterpret_castfor type conversion.This change replaces a C-style cast with an explicit
reinterpret_cast, which is the correct C++ approach for this low-level casting operation. The explicit cast makes the intention clear and addresses GCC 14.2 compilation warnings about casting user-defined numeric types..github/workflows/devcontainer-all.yaml (1)
55-57: Compatibility Confirmed:build-push-action@v6
All checks passed—no lingering v5-specific behavior detected:
- No references to
docker/build-push-action@v5in any workflow- No job-level
needs.*.outputs.*usages- No step-level
steps.*.outputs.*usagesNo further action required.
.github/workflows/devcontainer-esp32.yml (1)
55-57: Same note as for the “all” workflow – double-check v6 migration.Ensure that the new major version’s default settings (cache, SBOM, provenance) do not change the resulting image semantics or CI timings.
.github/workflows/devcontainer-freertos-nxp.yaml (1)
55-62: Pindocker/build-push-actionto a commit SHA and double-check v6 input compatibilityJumping from
v5tov6is fine, but:
- Upstream strongly recommends pinning by SHA to avoid supply-chain surprises (
docker/build-push-action@v6@<sha>).v6introduced minor input tweaks (e.g., defaultprovenance,sbom,cache-tochanges). Your current inputs look unchanged; please confirm this still yields identical images.[ suggest_nitpick ][ request_verification ]
.github/workflows/devcontainer-chibios.yaml (1)
55-62: Same SHA-pinning & v6-inputs concern as noted in devcontainer-freertos-nxp workflow – please apply the earlier feedback here as well.[ duplicate_comment ]
.github/workflows/devcontainer-azurertos.yaml (1)
55-62: Repeats the un-pinneddocker/build-push-action@v6pattern – see first comment for rationale and suggested fix.[ duplicate_comment ]
.github/workflows/devcontainer-ti.yaml (1)
55-62: Same action-pinning recommendation applies – please refer to the initial comment.[ duplicate_comment ]
.devcontainer/FreeRTOS-NXP/Dockerfile.FreeRTOS-NXP (1)
1-1: Verify the new base image tag and consider digest pinning
v1.08brings the GCC 14.2 toolchain – great.
However, relying only on the mutable tag means future rebuilds could pick up a different image. Pinning to its immutable digest keeps builds deterministic. Also ensure.github/workflows/devcontainer-freertos-nxp.yamlstill parses the version correctly (regex '(?<=:)(?:[v]\d+\.\d+)'will continue to work).[ suggest_nitpick ][ request_verification ]
azure-pipelines.yml (3)
418-424: 👍 Good call adding Ninja right after GCC installPlacing the
download-install-ninja.ymltemplate immediately after the GCC tool-chain step keeps all build-essential tools grouped and avoids extra PATH juggling later. No issues spotted.
620-624: Same improvement replicated for NXP laneConsistency across jobs is appreciated – keeps the container images symmetric.
770-774: Azure RTOS job aligns with the restNothing to flag; step ordering remains correct (GCC → Ninja → Hex/DFU/SRecord).
.devcontainer/All/Dockerfile.All.SRC (1)
12-18: Tool-chain bump looks correctURL & strip-components = 1 still match the upstream archive structure for 14.2.rel1.
No further action.
| # Clone FreeRTOS | ||
| RUN git clone --branch V10.4.1-kernel-only https://github.com/FreeRTOS/FreeRTOS-Kernel.git --depth 1 ./sources/FreeRTOS \ | ||
| && git clone --branch 5.5.1 https://github.com/ARM-software/CMSIS_5.git --depth 1 ./sources/CMSIS_5 | ||
| RUN git clone --branch V10.4.1-kernel-only https://github.com/FreeRTOS/FreeRTOS-Kernel.git --depth 1 ./sources/FreeRTOS | ||
|
|
||
| # Clone CMSIS | ||
| RUN GIT_LFS_SKIP_SMUDGE=1 git clone --branch 5.5.1 https://github.com/ARM-software/CMSIS_5.git --depth 1 ./sources/CMSIS_5 | ||
|
|
There was a problem hiding this comment.
Skipping Git-LFS smudge may break later builds
CMSIS_5 hosts several mandatory .zip/binary blobs under LFS pointers (e.g. DSP lib archives). With GIT_LFS_SKIP_SMUDGE=1 those blobs remain as 100-byte pointers; any CMake that expects real archives will fail at build time.
Options:
- Run
git lfs pull --include="CMSIS/DSP/**"right after the clone. - Clone normally and rely on the LFS cache layer in CI.
-RUN GIT_LFS_SKIP_SMUDGE=1 git clone --branch 5.5.1 https://github.com/ARM-software/CMSIS_5.git --depth 1 ./sources/CMSIS_5
+RUN git clone --branch 5.5.1 https://github.com/ARM-software/CMSIS_5.git --depth 1 ./sources/CMSIS_5 \
+ && git -C ./sources/CMSIS_5 lfs pull --include="CMSIS/DSP/**"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Clone FreeRTOS | |
| RUN git clone --branch V10.4.1-kernel-only https://github.com/FreeRTOS/FreeRTOS-Kernel.git --depth 1 ./sources/FreeRTOS \ | |
| && git clone --branch 5.5.1 https://github.com/ARM-software/CMSIS_5.git --depth 1 ./sources/CMSIS_5 | |
| RUN git clone --branch V10.4.1-kernel-only https://github.com/FreeRTOS/FreeRTOS-Kernel.git --depth 1 ./sources/FreeRTOS | |
| # Clone CMSIS | |
| RUN GIT_LFS_SKIP_SMUDGE=1 git clone --branch 5.5.1 https://github.com/ARM-software/CMSIS_5.git --depth 1 ./sources/CMSIS_5 | |
| # Clone FreeRTOS | |
| RUN git clone --branch V10.4.1-kernel-only https://github.com/FreeRTOS/FreeRTOS-Kernel.git --depth 1 ./sources/FreeRTOS | |
| # Clone CMSIS | |
| RUN git clone --branch 5.5.1 https://github.com/ARM-software/CMSIS_5.git --depth 1 ./sources/CMSIS_5 \ | |
| && git -C ./sources/CMSIS_5 lfs pull --include="CMSIS/DSP/**" |
🤖 Prompt for AI Agents
In .devcontainer/FreeRTOS-NXP/Dockerfile.FreeRTOS-NXP.SRC around lines 64 to 69,
the clone command for CMSIS_5 uses GIT_LFS_SKIP_SMUDGE=1 which leaves LFS
pointers instead of actual binary files, causing build failures. To fix this,
remove the GIT_LFS_SKIP_SMUDGE=1 from the clone command and add a command
immediately after to run git lfs pull --include="CMSIS/DSP/**" to fetch the
required LFS binary blobs, ensuring all necessary files are present for the
build.
| # update tool path to include versioned toolchain folder | ||
| $toolPath = $toolPath + "\arm-gnu-toolchain-" + $Version + "-mingw-w64-i686-arm-none-eabi" | ||
|
|
||
| # unzip toolchain | ||
| Expand-Archive $output -DestinationPath $toolPath > $null | ||
|
|
There was a problem hiding this comment.
Extraction path now nests twice – binaries will end up one directory too deep
$toolPath already ends with \GNU_Tools_ARM_Embedded\<version>.
Appending \arm-gnu-toolchain-<version>… and un-zipping a package that itself contains that same root folder yields
…\<version>\arm-gnu-toolchain-<version>\arm-gnu-toolchain-<version>\bin, breaking ARM_GCC_PATH.
Fix by extracting straight into the <version> directory and let the archive create the sub-folder:
-# update tool path to include versioned toolchain folder
-$toolPath = $toolPath + "\arm-gnu-toolchain-" + $Version + "-mingw-w64-i686-arm-none-eabi"
-
-# unzip toolchain
-Expand-Archive $output -DestinationPath $toolPath > $null
+# unzip toolchain; archive already contains the versioned root folder
+Expand-Archive $output -DestinationPath $toolPath > $null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # update tool path to include versioned toolchain folder | |
| $toolPath = $toolPath + "\arm-gnu-toolchain-" + $Version + "-mingw-w64-i686-arm-none-eabi" | |
| # unzip toolchain | |
| Expand-Archive $output -DestinationPath $toolPath > $null | |
| # unzip toolchain; archive already contains the versioned root folder | |
| Expand-Archive $output -DestinationPath $toolPath > $null |
🤖 Prompt for AI Agents
In install-scripts/install-arm-gcc-toolchain.ps1 around lines 73 to 78, the
extraction path is causing the toolchain to be nested twice because $toolPath
already includes the versioned folder and the archive contains the same root
folder. To fix this, remove the appended "\arm-gnu-toolchain-" + $Version +
"-mingw-w64-i686-arm-none-eabi" from $toolPath before extraction and extract the
archive directly into the existing version directory so the archive creates the
sub-folder itself, preventing the extra nesting.
|
@josesimoes , I am done playing here. All the above needs testing (and potentially fixing), but hopefully give the correct way forward. |
Description
It unlocks the new issues :
(speculative fix applied):
(not fixed yet):
Motivation and Context
GCC updates. #3061
How Has This Been Tested?
The CI.
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
Summary by CodeRabbit