Skip to content

Re-host MDP pipeline as single GenerateNanoPeFile target#4

Merged
josesimoes merged 2 commits into
mainfrom
rehost-mdp-pipeline
Jun 24, 2026
Merged

Re-host MDP pipeline as single GenerateNanoPeFile target#4
josesimoes merged 2 commits into
mainfrom
rehost-mdp-pipeline

Conversation

@josesimoes

@josesimoes josesimoes commented Jun 24, 2026

Copy link
Copy Markdown
Member

Description

  • 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 with native implementation trhough InternalCall fixture.

Motivation and Context

  • Following SDK plan doc 04 §4.1–4.4: re-host the MDP stage with proper incrementality, ordering and surfaced outputs.
  • AfterTargets ordering makes the SDK robust to future Microsoft.NET.Sdk chain changes vs. the legacy wholesale redefinition approach.
  • Three task calls collapsed to one; NativeChecksum is groundwork for the optional §4.5 ABI gate.
  • Latent bugs from the NFProjectSystem legacy (undefined params, dead branch, empty placeholder targets) fixed in the same pass.
  • Testing stubs generation from SDK.
  • Related with SDK-style MSBuild project system for .NET nanoFramework Home#1784

How Has This Been Tested?

  • Local build of SDK and smoke test project.

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue with code or algorithm)
  • New feature (non-breaking change which adds functionality to code)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dependencies (update dependencies and changes associated, has no impact on code or features)
  • Unit Tests (add new Unit Test(s) or improved existing one(s), has no impact on code or features)
  • Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist:

  • My code follows the code style of this project (only if there are changes in source code).
  • My changes require an update to the documentation (there are changes that require the docs website to be updated).
  • I have updated the documentation accordingly (the changes require an update on the docs in this repo).
  • I have read the CONTRIBUTING document.
  • I have tested everything locally and all new and existing tests passed (only if there are changes in source code).
  • I have added new tests to cover my changes.

@nfbot nfbot added the Type: enhancement New feature or request label Jun 24, 2026
@josesimoes josesimoes requested a review from danielmeza June 24, 2026 12:53
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@josesimoes, we couldn't start this review because you've reached your PR review rate limit.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7048a0b5-eb66-49db-ac49-449fb97d86e5

📥 Commits

Reviewing files that changed from the base of the PR and between 35c5370 and f41f128.

📒 Files selected for processing (7)
  • .gitignore
  • src/nanoFramework.NET.Sdk/Sdk/nanoFramework.Mdp.targets
  • src/nanoFramework.NET.Sdk/Sdk/nanoFramework.Output.targets
  • test/SmokeTest/AssemblyInfo.cs
  • test/SmokeTest/HelloNano.cs
  • test/SmokeTest/NativeLibrary.cs
  • test/SmokeTest/SmokeTest.csproj
📝 Walkthrough

Walkthrough

The PR replaces the two-target MDP build pipeline (NanoMetadataProcessor + NanoMetadataProcessorCompile) with a single GenerateNanoPeFile target that invokes MetaDataProcessorTask once after CoreCompile. Output, copy, and checksum targets are rewired accordingly. The SmokeTest project gains native interop fixtures, unsafe block support, and stub-generation configuration to exercise the new pipeline.

Changes

MDP Pipeline Unification and Output Rewire

Layer / File(s) Summary
GenerateNanoPeFile: property defaults and unified MDP invocation
src/nanoFramework.NET.Sdk/Sdk/nanoFramework.Mdp.targets
Core-library property-group defaults reorganized; new _NanoMdpParseInput/_NanoMdpCompileOutput*/stub properties introduced; single GenerateNanoPeFile target replaces NanoMetadataProcessor and NanoMetadataProcessorCompile, running MetaDataProcessorTask once after CoreCompile with mode-controlled stub/dump/dat/xml outputs, artifact registration, and dependency-chain updates (ResolveRuntimeDependencies, NanoGenerateResources); legacy placeholder .dat/.xml target removed.
Output, copy, checksum, and clean target rewiring
src/nanoFramework.NET.Sdk/Sdk/nanoFramework.Output.targets
NanoGenerateBinaryOutput wired after GenerateNanoPeFile; new NanoReportChecksum target publishes NF_NATIVE_ASSEMBLY_CHECKSUM to GITHUB_ENV/TF_BUILD; NanoCopyOutputFiles and _NanoCopyReferencesToIntermediate updated to run after CopyFilesToOutputDirectory with DisableNanoMdp guards; _NanoCleanExtraFiles extended to delete $(_NanoMdpSaveStringsFile).
SmokeTest native interop fixtures and project configuration
test/SmokeTest/SmokeTest.csproj, test/SmokeTest/NativeLibrary.cs, test/SmokeTest/AssemblyInfo.cs, test/SmokeTest/HelloNano.cs, .gitignore
Project enables unsafe blocks and _NanoMdpGenerateStubs; new NativeLibrary class adds managed driver and [MethodImpl(InternalCall)] extern declarations covering ref, pointer, and static signatures; AssemblyNativeVersion attribute added; HelloNano.cs uses block-scoped namespace; generated Stubs directory added to .gitignore.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • nanoframework/nanoFramework.NET.Sdk#3: Makes earlier code-level changes to nanoFramework.Mdp.targets and nanoFramework.Output.targets covering MDP/output modularization and target restructuring that this PR directly extends and replaces.

Suggested labels

Type: Unit Tests, Area: Config-and-Build

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change and has no issue references or trailing period.
Description check ✅ Passed The description is directly related to the pipeline rework and smoke test additions in the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@josesimoes

Copy link
Copy Markdown
Member Author

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
@josesimoes josesimoes force-pushed the rehost-mdp-pipeline branch from 35c5370 to 92ef12f Compare June 24, 2026 13:03

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 1f7ba54 and 35c5370.

📒 Files selected for processing (7)
  • .gitignore
  • src/nanoFramework.NET.Sdk/Sdk/nanoFramework.Mdp.targets
  • src/nanoFramework.NET.Sdk/Sdk/nanoFramework.Output.targets
  • test/SmokeTest/AssemblyInfo.cs
  • test/SmokeTest/HelloNano.cs
  • test/SmokeTest/NativeLibrary.cs
  • test/SmokeTest/SmokeTest.csproj

Comment thread src/nanoFramework.NET.Sdk/Sdk/nanoFramework.Mdp.targets Outdated
Comment thread src/nanoFramework.NET.Sdk/Sdk/nanoFramework.Mdp.targets
Comment thread src/nanoFramework.NET.Sdk/Sdk/nanoFramework.Output.targets Outdated
Comment thread src/nanoFramework.NET.Sdk/Sdk/nanoFramework.Output.targets Outdated
@josesimoes josesimoes merged commit 81a498e into main Jun 24, 2026
3 of 5 checks passed
@josesimoes josesimoes deleted the rehost-mdp-pipeline branch June 24, 2026 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants