Add support for generics <T> #3137
Conversation
***NO_CI***
…into generics-wip
…into generics-wip
…into generics-wip
…into generics-wip
…into generics-wip
…into generics-wip
(missed merge from upstream)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
azure-pipelines-templates/check-mdp-for-build.yml (4)
5-12: PowerShell Task: MDP Build Check Initialization
The first task correctly sets up a PowerShell script to check for an associated PR and builds an authorization header using a GitHub token. Verify that the substitution for$(GitHubToken)reliably injects the token and that error handling in case of token issues is appropriate.
13-20: PR Retrieval via GitHub API
The script usesInvoke-WebRequestto obtain PR details and converts the result from JSON. This technique is appropriate, but consider adding error handling in case the API call fails (for example, due to network issues or an invalid token).
21-25: PR Number Verification
The condition checking that the retrieved PR number matches the expected environment variable is clear and concise. It may be beneficial to include additional logging or error handling for unexpected mismatches in automated contexts.
26-31: Extracting PR Commit Message & Matching Build ID
The script extracts the commit message and uses a regex to search for the build ID pattern ([build with MDP buildId (\d+)]). This approach is clear; however, ensure that all commit messages adhere strictly to this format to reliably capture the build ID.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
azure-pipelines-nightly.yml(1 hunks)azure-pipelines-templates/check-mdp-for-build.yml(1 hunks)src/CLR/Core/Serialization/BinaryFormatter_stub.cpp(3 hunks)src/CLR/Debugger/Debugger.cpp(122 hunks)
✅ Files skipped from review due to trivial changes (1)
- azure-pipelines-nightly.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CLR/Core/Serialization/BinaryFormatter_stub.cpp
🧰 Additional context used
🪛 YAMLlint (1.35.1)
azure-pipelines-templates/check-mdp-for-build.yml
[error] 1-1: wrong new line character: expected \n
(new-lines)
⏰ Context from checks skipped due to timeout of 90000ms (29)
- 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_STM32_targets ST_STM32F769I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_BLE_REV0)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_PSRAM_REV0)
- 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 (Check_Code_Style)
- 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 (TI_CC1352R1_LAUNCHXL, Debug, All, 915)
- GitHub Check: build-target (NXP_MIMXRT1060_EVK, Debug, All)
- 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_S3, Debug, ESP32)
- GitHub Check: build-target (ESP32_S2_USB, Debug, ESP32)
- GitHub Check: build-target (ESP_WROVER_KIT, Debug, ESP32)
- GitHub Check: build-target (M5Core2, Debug, ESP32)
- GitHub Check: build-target (ST_STM32F769I_DISCOVERY, Debug, All)
- GitHub Check: build-target (SL_STK3701A, Debug, All)
🔇 Additional comments (14)
src/CLR/Debugger/Debugger.cpp (9)
23-23: Good modernization: Replacing NULL with nullptrThis is a good modernization of the codebase, replacing the C-style NULL with C++11's nullptr, which provides better type safety.
132-132: Consistent nullptr usage throughout the codebaseThe consistent replacement of NULL with nullptr throughout the codebase improves type safety and readability. This is a good modern C++ practice.
Also applies to: 171-171, 216-216, 237-237
304-304: Improved member variable naming conventionsRenaming from
m_flagstoflagsimproves code consistency, though make sure this naming convention is consistent across the entire codebase.Also applies to: 323-323
380-380: Type inference with auto keyword improves code readabilityUsing C++11's
autokeyword for type inference improves code readability where the type is obvious from the right-hand side of the assignment. This is especially helpful for complicated typenames.Also applies to: 431-431, 1001-1001, 1015-1015, 1031-1031, 1041-1041, 1065-1065, 1405-1405, 1548-1548, 1782-1782
2050-2061: Method signature update for consistencyGood update to the method signature, changing
CLR_IDX idxAssmtoCLR_INDEX indexAssmfor better naming consistency. This makes the parameter name match its type and improves code clarity.
1899-1902: Added support for generic instancesAdded handling for the
DATATYPE_GENERICINSTcase to properly support generic types, aligning with the PR objective to add support for generics.
2439-2440: Modified return pattern for GetValueThe code has been modified to return the result of
GetValue()directly instead of usingWP_ReplyToCommand()with a nullptr. This simplifies the code path by lettingGetValuehandle the reply to command.Also applies to: 2459-2460, 2872-2873, 2984-2985, 3035-3036, 3049-3050, 3073-3074, 3132-3133, 3164-3165, 3192-3193, 3409-3410
2796-2835: Added support for generic parameters in methodsImportant addition to handle generic parameters in both generic type definitions (
DATATYPE_VAR) and generic method definitions (DATATYPE_MVAR). This implementation correctly resolves the generic parameter positions and applies the appropriate data types.
2579-2587: Improved null safety in IsBlockEnumMaybeGood enhancement to check for null class handler before accessing its members, preventing potential null pointer dereference. The additional check makes the function more robust.
azure-pipelines-templates/check-mdp-for-build.yml (5)
32-42: Build ID Extraction & Variable Setting
The conditional extraction of the build ID and setting it as a pipeline variable (using the##vso[task.setvariable]command) is implemented correctly. It might improve robustness to add additional logging or alternate flows if the regex match fails.
43-45: Securing the GitHub Token
Defining the environment variableGITHUB_TOKENcorrectly ensures token availability in the script. Just a reminder to safeguard this token so it is not inadvertently exposed in logs or outputs.
46-63: Artifact Download Task Configuration
The second task uses conditions effectively to determine when to download the MDP artifact. Parameters such asbuildVersionToDownload,pipelineId,artifactName, and the specifieditemPatternare well defined. Ensure that theMDP_BUILDIDvariable is correctly populated from the previous step, as this is critical for the artifact download to function as expected.
64-75: Setting MDP Path with Inline Script
The final task sets theNF_MDP_MSBUILDTASK_PATHvariable using a succinct inline script. Verify that the path$(Pipeline.Workspace)/mdp/nanoFramework.Tools.MetadataProcessor.MsBuildTask.dllis correct in all intended pipeline contexts, and consider adding error checks if the file is not present after download.
1-3:✅ Verification successful
Header & Newline Characters Issue
The header is complete with appropriate license information. However, YAMLlint reported an issue with the newline character on line 1 (expected LF\n). Please ensure the file uses consistent LF newlines throughout to avoid potential linting issues.
#!/bin/bash # Check for CRLF characters in the file file="azure-pipelines-templates/check-mdp-for-build.yml" if grep -q $'\r' "$file"; then echo "CRLF newline characters detected in $file" else echo "LF newline characters confirmed in $file" fiLF Newline Consistency Verified
The fileazure-pipelines-templates/check-mdp-for-build.ymlhas been checked and confirmed to use consistent LF newline characters. This indicates that the YAMLlint warning regarding an expected LF newline on line 1 might be a misconfiguration or a false positive. Please review your YAMLlint settings if the warning persists.
- Verified newline consistency using a grep-based check (output: "LF newline characters confirmed").
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
azure-pipelines-templates/check-mdp-for-build.yml (2)
1-3: Newline Formatting IssueYAMLlint reported a newline character error. Please ensure that this file uses LF (
\n) line endings rather than CRLF to comply with YAML standards.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
4-43: PowerShell Task for Checking MDP BuildThe inline PowerShell script clearly retrieves PR details, constructs the authorization header, and extracts the MDP build ID from the commit message. However, to enhance robustness, consider adding error handling (e.g., try/catch blocks or checking the HTTP response status) to gracefully handle network issues or unexpected API responses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
azure-pipelines-templates/check-mdp-for-build.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
azure-pipelines-templates/check-mdp-for-build.yml
[error] 1-1: wrong new line character: expected \n
(new-lines)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: nf-interpreter (Check_Code_Style)
- 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 (TI_CC1352R1_LAUNCHXL, Debug, All, 915)
- GitHub Check: build-target (NXP_MIMXRT1060_EVK, Debug, All)
- 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_S3, Debug, ESP32)
- GitHub Check: build-target (ESP32_S2_USB, Debug, ESP32)
- GitHub Check: build-target (ESP_WROVER_KIT, Debug, ESP32)
- GitHub Check: build-target (M5Core2, Debug, ESP32)
- GitHub Check: build-target (ST_STM32F769I_DISCOVERY, Debug, All)
- GitHub Check: build-target (SL_STK3701A, Debug, All)
- GitHub Check: build-target (SL_STK3701A, Debug, AzureRTOS)
🔇 Additional comments (2)
azure-pipelines-templates/check-mdp-for-build.yml (2)
46-63: Download Pipeline Artifact Task ValidationThe artifact download task is well-configured with a clear condition that checks for both
DownloadMDPbeing true andMDP_BUILDIDnot being empty. Ensure that the upstream variable assignments are reliable so that this step triggers only under the intended circumstances.
64-75: Setting MDP Path VariableThe task setting the MDP path variable is implemented correctly using the Azure DevOps logging command. Verify that the use of a backslash (i.e.,
$(Pipeline.Workspace)\mdp) is intentional for the target environment. If not, consider using a forward slash for consistency on non‐Windows platforms.
…into generics-wip
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
azure-pipelines-templates/check-mdp-for-build.yml (1)
93-94:⚠️ Potential issueFix Mismatched Parenthesis in Copy-Item Command
On line 93 the path string contains an extraneous closing parenthesis (${env:Pipeline_Workspace)}) which will cause the Copy-Item command to fail. Please remove the extra parenthesis so that the variable is correctly referenced.Apply this diff:
- Copy-Item -Path "${env:Pipeline_Workspace)}\mdp" -Destination $msbuildPath -Recurse + Copy-Item -Path "${env:Pipeline_Workspace}\mdp" -Destination $msbuildPath -Recurse
🧹 Nitpick comments (1)
azure-pipelines-templates/check-mdp-for-build.yml (1)
1-1: Correct Newline Formatting
The static analysis tool (YAMLlint) has flagged an issue on line 1 regarding the newline character. Please ensure that the file is using Unix-style (LF) newlines rather than Windows-style.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
azure-pipelines-templates/check-mdp-for-build.yml(1 hunks)azure-pipelines.yml(5 hunks)src/CLR/Debugger/Debugger.cpp(122 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CLR/Debugger/Debugger.cpp
🧰 Additional context used
🪛 YAMLlint (1.35.1)
azure-pipelines-templates/check-mdp-for-build.yml
[error] 1-1: wrong new line character: expected \n
(new-lines)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
- GitHub Check: build-target (TI_CC1352R1_LAUNCHXL, Debug, All, 915)
- GitHub Check: build-target (NXP_MIMXRT1060_EVK, Debug, All)
- 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_S3, Debug, ESP32)
- GitHub Check: build-target (ESP32_S2_USB, Debug, ESP32)
- GitHub Check: build-target (ESP_WROVER_KIT, Debug, ESP32)
- GitHub Check: build-target (M5Core2, Debug, ESP32)
- GitHub Check: build-target (ST_STM32F769I_DISCOVERY, Debug, All)
- GitHub Check: build-target (SL_STK3701A, Debug, All)
- GitHub Check: build-target (SL_STK3701A, Debug, AzureRTOS)
🔇 Additional comments (3)
azure-pipelines.yml (3)
1268-1270: Ensure Complete Dependency Specification in Unit Test Job
In theRun_UnitTests_mscorlibjob, a new dependency onCheck_Build_Optionshas been added (line 1270) alongside the existing dependency onBuild_nanoCLR_CLI. This is a good step to guarantee that the outputs from the build options are available. Please verify that the order and overall job dependencies continue to reflect the intended build flow.
1279-1281: Introduce MDP Download Flag via Dependency Output
The new variableDownloadMDPis assigned on line 1280 from the output of theCheck_Build_Optionsjob (BuildOptions.USE_SPECIFIC_MDP). This addition will help conditionally trigger downstream logic for downloading the metadata processor artifact. Please ensure that the output variable name is correctly set inCheck_Build_Optionsand that downstream templates or tasks correctly consume this flag.
1307-1308: Integrate MDP Build Check Template
A new template inclusion forcheck-mdp-for-build.ymlhas been added on line 1307 in theRun_UnitTests_mscorlibjob. This change aims to verify if a specific MDP build is to be used. Please confirm that all required variables (such asMDP_BUILDIDandDownloadMDP) are set appropriately and that the template’s logic integrates seamlessly with the rest of the job.
josesimoes
force-pushed
the
generics-wip
branch
from
68620e2 to
828d8e3
Compare
- Required because VS tries to load MDP from the directory and then all the other DLLs are missing. - Should be reviewed upstream in order to pack all the directory.
josesimoes
force-pushed
the
generics-wip
branch
from
828d8e3 to
29f16f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
azure-pipelines-templates/check-mdp-for-build.yml (3)
1-3: File Header and Line Endings
The header and copyright/licensing block appear standard. Note that YAMLlint reported a new line character issue—please ensure the file uses LF (\n) rather than CRLF.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
4-42: PowerShell Task: Check MDP for Build – PR Details & Build ID Extraction
This task defines an inline PowerShell script that:
- Computes an authorization header using a Base64-encoded token.
- Retrieves PR details from the GitHub API.
- Checks that the retrieved PR number matches the current pull request.
- Extracts the PR commit message and looks for a build ID pattern (
[build with MDP buildId NNN]).The logic correctly sets the
MDP_BUILDIDvariable when a match is found.
Consider adding error handling for the web requests (e.g. in case the GitHub API call fails) to avoid unexpected pipeline errors.
64-92: PowerShell Task: Copy MDP to Build Tool
This inline PowerShell script locates the Visual Studio installation via vswhere and then copies the MDP DLL to the MSBuild location.
- The script correctly builds paths using the output of vswhere.
- One suggestion is to add error handling around the vswhere command (in case the executable is missing or returns an unexpected result).
azure-pipelines.yml (1)
95-138: Enhanced PR Build Handling & Metadata Detection in Check_Build_Options
Within theCheck_Build_Optionsjob, the newly added PowerShell block (lines 95–138) enhances the handling for PR builds by:
- Recomputing the authorization header.
- Retrieving commit details to skip builds for version update commits.
- Fetching the PR details and commit message.
- Setting variables based on the commit message:
- It enables running mscorlib tests when the commit message contains either
[run mscorlib tests]or a reference tonanoframework/CoreLibrary#....- It sets a flag to use a specific MDP build when the PR commit message matches
\[build with MDP buildId (\d+)\].Nitpick:
The message on line 129 currently reads:-Write-Host "##[command] >> mscorlib tests are to be runned" +Write-Host "##[command] >> mscorlib tests are to be run"Please update the phrasing to correct the grammatical error.
Overall, this new logic meets the PR objectives for improved metadata handling and dynamic build configuration.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 104-104: trailing spaces
(trailing-spaces)
[error] 108-108: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
azure-pipelines-templates/check-mdp-for-build.yml(1 hunks)azure-pipelines.yml(5 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
azure-pipelines-templates/check-mdp-for-build.yml
[error] 1-1: wrong new line character: expected \n
(new-lines)
⏰ Context from checks skipped due to timeout of 90000ms (29)
- 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_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 (Build_NXP_targets NXP_MIMXRT1060_EVK)
- GitHub Check: nf-interpreter (Check_Code_Style)
- 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 (TI_CC1352R1_LAUNCHXL, Debug, All, 915)
- GitHub Check: build-target (NXP_MIMXRT1060_EVK, Debug, All)
- 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_S3, Debug, ESP32)
- GitHub Check: build-target (ESP32_S2_USB, Debug, ESP32)
- GitHub Check: build-target (ESP_WROVER_KIT, Debug, ESP32)
- GitHub Check: build-target (M5Core2, Debug, ESP32)
- GitHub Check: build-target (ST_STM32F769I_DISCOVERY, Debug, All)
- GitHub Check: build-target (SL_STK3701A, Debug, All)
🔇 Additional comments (3)
azure-pipelines-templates/check-mdp-for-build.yml (2)
43-45: Environment Variable Configuration
The environment block setsGITHUB_TOKENappropriately from the pipeline variable. This is straightforward and appears correct.
46-63: DownloadPipelineArtifact Task Configuration
The DownloadPipelineArtifact task is well configured. It downloads the MDP artifact only when theDownloadMDPvariable is true andMDP_BUILDIDis set. All required inputs (e.g. project, definition, artifact name, and item pattern) are clearly specified.azure-pipelines.yml (1)
1270-1281: Mapping USE_SPECIFIC_MDP to DownloadMDP in Run_UnitTests_mscorlib Job
A new variable assignment in theRun_UnitTests_mscorlibjob maps the output fromCheck_Build_Optionsto the variableDownloadMDP:DownloadMDP: $[ dependencies.Check_Build_Options.outputs['BuildOptions.USE_SPECIFIC_MDP'] ]This enables subsequent steps to conditionally download the MDP artifact based on the PR commit message. Verify that downstream tasks correctly consume this variable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
azure-pipelines-templates/check-mdp-for-build.yml (3)
1-3: File Formatting & Newline Consistency
The file’s header lines indicate it’s a new YAML pipeline file. Please ensure that the file uses LF (\n) as the newline character (per YAMLlint’s expectations), so that the “wrong new line character” error is resolved.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
19-24: PR Retrieval Logic
The use ofInvoke-WebRequestto fetch the pull request details and convert the response from JSON is clear and straightforward. Consider wrapping this call in error handling (e.g. try/catch) to gracefully handle possible API failures or network issues in the pipeline.
64-93: Copy MDP Files Step
The PowerShell task to copy the MDP files from the artifact location to the MSBuild directory is well-structured. One suggestion would be to add error handling (or at least a conditional check) to verify thatvswhere.exeis available and that the Visual Studio installation paths are successfully retrieved before attempting the file copy.azure-pipelines.yml (4)
75-147: Enhanced PR Build Options Logic
Within theCheck_Build_Optionsjob, the added segments (e.g., printing "This is a PR build" at line 93, conditionally skipping builds for version update commits, and the blocks that setRUN_MSCORLIB_TESTSandUSE_SPECIFIC_MDPbased on patterns in the PR commit message) are logically sound and improve clarity in triggering build behaviors. Ensure that these conditions cover all edge cases (such as multiple matching patterns) and consider adding error handling for REST API calls as needed.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 85-85: trailing spaces
(trailing-spaces)
[error] 104-104: trailing spaces
(trailing-spaces)
[error] 108-108: trailing spaces
(trailing-spaces)
104-104: Trailing Whitespace Cleanup (Line 104)
YAMLlint has flagged trailing spaces on this line. Removing extraneous spaces will help avoid formatting issues.- # Some comment with trailing spaces··· + # Some comment with trailing spaces🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 104-104: trailing spaces
(trailing-spaces)
108-108: Trailing Whitespace Cleanup (Line 108)
Similarly, trailing spaces on this line should be removed to conform with YAML formatting best practices.- # Another line with trailing spaces··· + # Another line with trailing spaces🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 108-108: trailing spaces
(trailing-spaces)
1268-1281: MDP Integration for Unit Tests
In theRun_UnitTests_mscorlibjob, the new variable assignment:DownloadMDP: $[ dependencies.Check_Build_Options.outputs['BuildOptions.USE_SPECIFIC_MDP'] ]ensures that the subsequent steps can conditionally use the MDP artifacts. Please verify that all downstream tasks correctly handle this flag and that its value is as expected when the PR commit message includes the appropriate trigger.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
azure-pipelines-templates/check-mdp-for-build.yml(1 hunks)azure-pipelines.yml(4 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
azure-pipelines.yml
[error] 104-104: trailing spaces
(trailing-spaces)
[error] 108-108: trailing spaces
(trailing-spaces)
azure-pipelines-templates/check-mdp-for-build.yml
[error] 1-1: wrong new line character: expected \n
(new-lines)
⏰ Context from checks skipped due to timeout of 90000ms (29)
- 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_ESP32_targets ESP32_PSRAM_REV0)
- GitHub Check: nf-interpreter (Build_NXP_targets NXP_MIMXRT1060_EVK)
- 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 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)
- GitHub Check: build-target (TI_CC1352R1_LAUNCHXL, Debug, All, 915)
- GitHub Check: build-target (NXP_MIMXRT1060_EVK, Debug, All)
- 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_S3, Debug, ESP32)
- GitHub Check: build-target (ESP32_S2_USB, Debug, ESP32)
- GitHub Check: build-target (ESP_WROVER_KIT, Debug, ESP32)
- GitHub Check: build-target (M5Core2, Debug, ESP32)
- GitHub Check: build-target (ST_STM32F769I_DISCOVERY, Debug, All)
- GitHub Check: build-target (SL_STK3701A, Debug, All)
🔇 Additional comments (5)
azure-pipelines-templates/check-mdp-for-build.yml (3)
15-15: Authentication Header Calculation
The Base64 encoding of the GitHub token for constructing the authorization header (line 15) is implemented correctly.
27-38: Build ID Extraction from Commit Message
The logic that matches the commit message for the pattern\[build with MDP buildId (\d+)\]and sets theMDP_BUILDIDvariable is correct and clear.
46-63: Artifact Download Configuration
The task parameters for downloading the MDP artifact (usingDownloadPipelineArtifact@2) are well set up, with clear conditions ensuring that the artifact is downloaded only when required.azure-pipelines.yml (2)
1307-1307: MDP Check Template Inclusion
The inclusion of the newcheck-mdp-for-build.ymltemplate (line 1307) in the unit testing job is a nice modularization of the MDP-related functionality. Confirm that this template executes correctly in every scenario expected by the pipeline.
1-31: General Pipeline Configuration & Trigger Settings
The trigger and resource definitions (lines 1–31) adhere to the established CI/CD standards. There are no concerns here; the configurations are consistent and clear.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
azure-pipelines-templates/check-mdp-for-build.yml (1)
4-43: Verify PR Details Extraction and MDP Build ID Detection
The inline PowerShell script correctly obtains PR details from GitHub and uses a regular expression to extract the MDP build ID from the commit message. Consider adding error handling (e.g. try/catch) around the network call (Invoke-WebRequest) to gracefully handle scenarios when the API call fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
azure-pipelines-templates/check-mdp-for-build.yml(1 hunks)azure-pipelines.yml(4 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
azure-pipelines-templates/check-mdp-for-build.yml
[error] 1-1: wrong new line character: expected \n
(new-lines)
🔇 Additional comments (5)
azure-pipelines-templates/check-mdp-for-build.yml (2)
1-3: Ensure Correct Newline Characters
Static analysis detected that the newline characters do not match the expected LF (\n) style. Please convert the file’s line endings to LF for consistency with YAMLlint rules.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
43-45: Environment Variable Usage
Theenvblock properly sets theGITHUB_TOKENvariable from the pipeline variable. Ensure that the token is securely managed and that logging does not inadvertently expose its value.azure-pipelines.yml (3)
133-138: Detect and Enable Specific MDP Builds
The new logic in theCheck_Build_Optionsjob uses a regular expression to detect the pattern"[build with MDP buildId (\d+)]"in the PR commit message and sets theUSE_SPECIFIC_MDPvariable accordingly. This implementation aligns with the PR objectives. Consider adding error/logging steps in the event that the PR body structure changes or the expected match isn’t found.
1279-1281: Propagate MDP Build Configuration
Within theRun_UnitTests_mscorlibjob, the new variableDownloadMDPis derived from the output of theCheck_Build_Optionsjob (BuildOptions.USE_SPECIFIC_MDP). Please verify that this dependency output produces the intended boolean (or string) value so that downstream tasks correctly decide on downloading the specific MDP artifacts.
1-32: Overall Pipeline Configuration Quality
The YAML file is well structured with clearly defined triggers, resource definitions, and job dependencies. The integration of the new MDP-related steps (including artifact download and file copy steps) is consistent with the broader PR objectives. No major issues are noted in this section.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
…into generics-wip
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
azure-pipelines-templates/check-mdp-for-build.yml (5)
1-3: Newline Format Consistency
The file appears to be using a newline character different from the expected LF (\n), as indicated by the static analysis hint. Please ensure the file uses LF for consistency with the project standards.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
15-15: Authorization Header Construction
The inline PowerShell computes the authorization header correctly using Base64 encoding. Consider adding a check or error handling in case the$(GitHubToken)value is missing or empty.
21-25: Type Comparison in PR Verification
The condition checking if$pr.numberequals"$env:System_PullRequest_PullRequestNumber"may mix numeric and string types. Consider explicitly casting one side (or both) to avoid unintended behavior.
46-63: Hard-Coded Artifact Definition
TheDownloadPipelineArtifacttask is configured correctly. However, the artifact definition is hard-coded as'43'. For maintainability, consider parameterizing this value using a variable if it might change in the future.
64-94: MDP Copy Task Configuration
The final task correctly assembles the destination path and copies the MDP files. Verify that the environment variablePipeline_Workspaceis correctly set in the agent context. Also, consider adding error handling in case theCopy-Itemcommand fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
azure-pipelines-templates/check-mdp-for-build.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
azure-pipelines-templates/check-mdp-for-build.yml
[error] 1-1: wrong new line character: expected \n
(new-lines)
⏰ Context from checks skipped due to timeout of 90000ms (29)
- 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_NXP_targets NXP_MIMXRT1060_EVK)
- 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_PSRAM_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_STM32_targets ST_STM32F429I_DISCOVERY)
- GitHub Check: nf-interpreter (Check_Code_Style)
- 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 (TI_CC1352R1_LAUNCHXL, Debug, All, 915)
- GitHub Check: build-target (NXP_MIMXRT1060_EVK, Debug, All)
- 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_S3, Debug, ESP32)
- GitHub Check: build-target (ESP32_S2_USB, Debug, ESP32)
- GitHub Check: build-target (ESP_WROVER_KIT, Debug, ESP32)
- GitHub Check: build-target (M5Core2, Debug, ESP32)
- GitHub Check: build-target (ST_STM32F769I_DISCOVERY, Debug, All)
- GitHub Check: build-target (SL_STK3701A, Debug, All)
🔇 Additional comments (2)
azure-pipelines-templates/check-mdp-for-build.yml (2)
18-20: PR Retrieval Using Environment Variables
The script retrieves PR details via the GitHub API using environment variables (Build_Repository_NameandSystem_PullRequest_PullRequestNumber). Ensure these variables are always set in the build context to avoid runtime errors.
32-38: Extraction of Build ID
The regex pattern to extract the build ID from the PR commit message is clear and concise. Ensure that multiple occurrences are not expected, or if they are, document the behavior—this implementation picks only the first match.
josesimoes
force-pushed
the
generics-wip
branch
from
119886b to
b18de9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
azure-pipelines-templates/check-mdp-for-build.yml (2)
11-43: Review the MDP Build Check Script.
The inline PowerShell script for checking the build is well organized and follows the intended flow. However, consider adding error handling around theInvoke-WebRequestcall (line 19) to gracefully handle potential HTTP failures or API issues, ensuring that the script logs useful errors or takes corrective action when the GitHub API call fails.
64-93: Copy MDP to Build Tool – Variable Naming Consistency.
Within this inline script, the Visual Studio installer path is stored in the variable$VsWherePathat line 76, but at line 85 the code references$VSWherePath. Although PowerShell variable names are case-insensitive, using a consistent naming style (e.g., always$VsWherePath) will improve readability and reduce the risk of future mistakes. Additionally, consider verifying that the destination folder exists (or create it if necessary) before executing theCopy-Itemcommand to avoid potential runtime errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
azure-pipelines-templates/check-mdp-for-build.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
azure-pipelines-templates/check-mdp-for-build.yml
[error] 1-1: wrong new line character: expected \n
(new-lines)
⏰ Context from checks skipped due to timeout of 90000ms (31)
- 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_STM32_targets ST_STM32F769I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_BLE_REV0)
- GitHub Check: nf-interpreter (Build_TI_SimpleLink_targets TI_CC1352R1_LAUNCHXL_915)
- 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_ESP32_targets ESP32_PSRAM_REV0)
- GitHub Check: nf-interpreter (Check_Code_Style)
- 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 (TI_CC1352R1_LAUNCHXL, Debug, All, 915)
- GitHub Check: build-target (TI_CC1352R1_LAUNCHXL, Debug, TI, 915)
- GitHub Check: build-target (NXP_MIMXRT1060_EVK, Debug, All)
- 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_S3, Debug, ESP32)
- GitHub Check: build-target (ESP32_S2_USB, Debug, ESP32)
- GitHub Check: build-target (ESP_WROVER_KIT, Debug, ESP32)
- GitHub Check: build-target (M5Core2, Debug, ESP32)
- GitHub Check: build-target (ST_STM32F769I_DISCOVERY, Debug, All)
- GitHub Check: build-target (SL_STK3701A, Debug, All)
🔇 Additional comments (1)
azure-pipelines-templates/check-mdp-for-build.yml (1)
46-63: Artifact Download Step Verification.
The DownloadPipelineArtifact task is clearly defined with appropriate conditionals and parameters. Please verify that the artifact details (project, definition, artifact name, etc.) correctly correspond to the new MDP output and that any changes in the Metadata Definition Protocol are fully reflected here.
…into generics-wip
…eter into generics-wip
|
Ignoring failures in Dev Containers smoke tests. |
Description
NULLwithnullptrto address compiler warnings.Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
These changes improve maintainability and stability without altering the end-user functionality.