Re-host MDP pipeline as single GenerateNanoPeFile target#4
Conversation
|
Warning Review limit reached
More reviews will be available in 42 minutes and 8 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: nanoframework/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR replaces the two-target MDP build pipeline ( ChangesMDP Pipeline Unification and Output Rewire
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
|
Pipeline failing as expected with missing files to build the tasks. |
- Collapse 3 MetaDataProcessorTask calls into one GenerateNanoPeFile target (AfterTargets=CoreCompile); regular/core split in properties. - Replace *DependsOn chain redefinitions with appends; drop _NanoCopyAssemblyToOutDir; rewire output/copy targets via AfterTargets. - Add Inputs/Outputs for incrementality; surface NativeChecksum as $(NanoNativeMethodsChecksum) via dedicated NanoReportChecksum target. - Gate NanoGenerateBinaryOutput on NanoIsCoreLibrary != True (match legacy). - Fix dead core "no-stubs" branch; dump-exports, dependency-map and strings now actually emitted for core builds. - Drop undefined _NanoMdpStub* params and empty Dat/DependencyMap placeholder targets. - SmokeTest: add NativeLibrary.cs InternalCall fixture, AllowUnsafeBlocks, AssemblyNativeVersion, and _NanoMdpGenerateStubs. Assisted by Claude Opus 4.8
35c5370 to
92ef12f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/nanoFramework.NET.Sdk/Sdk/nanoFramework.Mdp.targets`:
- Around line 238-239: The checksum export in the NanoReportChecksum flow is
tied too closely to GenerateNanoPeFile, so it is skipped whenever the PE step is
up-to-date. Update the NanoReportChecksum/GenerateNanoPeFile targets in
nanoFramework.Mdp.targets so the checksum is persisted independently, either by
writing NanoNativeMethodsChecksum to an intermediate file or by making the
reporting target always run; use the existing GenerateNanoPeFile and
NanoReportChecksum symbols to keep the fix localized.
- Around line 217-218: The GenerateNanoPeFile incremental tracking only includes
the main .pe and .pdbx outputs, so side artifacts from the same generation can
be missed and the target may be incorrectly skipped. Update the
GenerateNanoPeFile Outputs in nanoFramework.Mdp.targets to also include the
generated DumpExports, GenerateDependency, SaveStrings, and stub skeleton files,
using the existing core/stub generation symbols and conditions where appropriate
so all emitted artifacts are tracked.
In `@src/nanoFramework.NET.Sdk/Sdk/nanoFramework.Output.targets`:
- Line 53: The PE output path in the nanoFramework output targets is still
hardcoded through _NanoIntermediateAssembly, which can drift from the shared
compile output path. Update the AssemblyPE assignment in the relevant target to
reuse the existing _NanoMdpCompileOutput symbol defined by GenerateNanoPeFile so
both places stay aligned if intermediate assembly handling changes.
- Around line 69-70: The Azure Pipelines logging command in the Message task is
using default verbosity, so it may be suppressed before the variable is set.
Update the Message element that emits the `##vso[task.setvariable ...]` command
in nanoFramework.Output.targets to use high importance, keeping the existing
condition and checksum expression intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5ad77732-0d01-434f-b35e-dd0f8f512d80
📒 Files selected for processing (7)
.gitignoresrc/nanoFramework.NET.Sdk/Sdk/nanoFramework.Mdp.targetssrc/nanoFramework.NET.Sdk/Sdk/nanoFramework.Output.targetstest/SmokeTest/AssemblyInfo.cstest/SmokeTest/HelloNano.cstest/SmokeTest/NativeLibrary.cstest/SmokeTest/SmokeTest.csproj
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist: