Move build tasks into SDK and add cross-platform resource parser#5
Merged
Conversation
nfbot
added
Type: enhancement
|
Warning Review limit reached
More reviews will be available in 41 minutes and 21 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 ignored due to path filters (2)
📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThis PR replaces the cross-platform ChangesnanoFramework Resource Pipeline and Build Tasks Overhaul
Sequence Diagram(s)sequenceDiagram
participant MSBuild
participant _NanoInterceptResxItems
participant _NanoCheckResourceManagerRef
participant GenerateNanoResourceTask
participant ProcessResourceFiles
participant NanoResxResourceReader
participant NanoResourceWriter
MSBuild->>_NanoInterceptResxItems: move .resx from EmbeddedResource to _NanoResxItems
_NanoInterceptResxItems->>_NanoCheckResourceManagerRef: verify nanoFramework.ResourceManager ref
_NanoCheckResourceManagerRef->>GenerateNanoResourceTask: invoke with Sources=@(_NanoResxItems)
GenerateNanoResourceTask->>GenerateNanoResourceTask: DeserializeCache / ShouldRebuildResgenOutputFile
GenerateNanoResourceTask->>ProcessResourceFiles: Run(log, assemblies, inputs, outputs)
ProcessResourceFiles->>NanoResxResourceReader: ReadResources(.resx)
NanoResxResourceReader-->>ProcessResourceFiles: resource entries
ProcessResourceFiles->>NanoResourceWriter: AddResource / Close → .nanoresources
NanoResourceWriter-->>ProcessResourceFiles: file written
ProcessResourceFiles-->>GenerateNanoResourceTask: UnsuccessfullyCreatedOutFiles
GenerateNanoResourceTask->>GenerateNanoResourceTask: WriteStateFile / RecordFilesWritten
GenerateNanoResourceTask-->>MSBuild: FilesWritten, OutputResources
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
14 tasks
- Copy 6 build-task sources from `Tools.BuildTasks-2019` into `src\nanoFramework.Tools.BuildTasks\`. - Remove cross-repo `<Compile>` links and comment block from `nanoFramework.Tools.BuildTasks.csproj`. - Delete `GenerateNanoResourceTask.Stub.cs`. - Add `ProcessResourceFiles.CrossPlatform.cs`: replacing `System.Drawing`, `AppDomain`, `BinaryFormatter`, and WinForms `ResXResourceReader` with `SixLabors.ImageSharp`, `System.Resources.Extensions`, `System.Text.Json`, and direct XML parsing. - Add `PackageReference`s for `SixLabors.ImageSharp`, `System.Resources.Extensions`, `System.CodeDom` . - Pack new runtime deps under `tasks\net8.0\` in `nanoFramework.NET.Sdk.csproj`. - Update `nanoFramework.Resources.targets` wiring. - Add `BuildTasks.Tests` with unit tests for `NanoResxResourceReader`, `ProcessResourceFiles`, and RGB565 conversion. - Add `SmokeTest` resources (`.resx`, bitmap, binary blob) for end-to-end validation. Assisted by Claude Opus 4.8
josesimoes
force-pushed
the
move-buildtasks-resources-to-sdk
branch
from
422b1ea to
3d8d133
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 `@nanoFramework.NET.Sdk/Sdk/nanoFramework.Resources.targets`:
- Around line 77-85: The emptiness check is currently on the
NanoGenerateResources target, which can prevent the dependent setup targets from
running. Remove the target-level Condition from NanoGenerateResources and move
the non-empty guard onto the GenerateNanoResourceTask invocation so
_NanoInterceptResxItems and _NanoCheckResourceManagerRef still execute. Keep the
task call in NanoGenerateResources the same otherwise, and ensure the task only
runs when @(_NanoResxItems) has items.
In `@nanoFramework.Tools.BuildTasks/GenerateBinaryOutputTask.cs`:
- Line 52: The PE path derivation in GenerateBinaryOutputTask should not use
substring replacement on the full path because it can alter directory names and
misses uppercase extensions. Update the AssemblyReferences selection that builds
peCollection to derive each path from GetMetadata("FullPath") using
Path.ChangeExtension with a .pe extension, keeping the logic localized to
GenerateBinaryOutputTask.
- Around line 67-75: The PE-file copy logic in GenerateBinaryOutputTask
currently relies on a single FileStream.Read call, which may not fill the entire
buffer before binFile.Write is called. Update the read path in the PE copy block
to ensure the buffer is fully populated before writing, either by looping until
all bytes are read or by using a read-to-completion helper if available, so the
assembly image written by GenerateBinaryOutputTask is complete and not partially
zero-filled.
In `@nanoFramework.Tools.BuildTasks/GenerateNanoResourceTask.cs`:
- Around line 843-848: Gate the LoadReferences() call in
GenerateNanoResourceTask so it only runs under NETFRAMEWORK, since the
cross-platform path does not use assemblyList but still hits
AssemblyName.GetAssemblyName() on unresolved @(ReferencePath) entries. Update
the assemblyList initialization in the resource generation flow to use
LoadReferences() on NETFRAMEWORK and an empty array otherwise, keeping the rest
of the logic in GenerateNanoResourceTask unchanged.
In `@nanoFramework.Tools.BuildTasks/ProcessResourceFiles.cs`:
- Around line 2112-2117: Update the .nanoresources write path in
ProcessResourceFiles so rewriting an existing file does not preserve stale
trailing bytes; the current File.Open(..., FileMode.OpenOrCreate) in the
serialization block can leave old data behind when the new output is smaller.
Change the file creation mode to overwrite/truncate the target when writing the
serialized resource data, and keep the change localized to the file write logic
around fileStream/BinaryWriter in this method.
- Around line 137-142: The failure tracking in ProcessResourceFiles.ProcessFiles
is adding the output task item itself instead of the output path string, which
breaks later enumeration in GenerateNanoResourceTask. Update the ProcessFile
failure branch to store outFiles[i].ItemSpec (or the equivalent string path) in
UnsuccessfullyCreatedOutFiles so it remains a string list and preserves the
original resource error.
- Around line 1839-1845: The resource file layout in the
serialization/deserialization path is still interleaving each Resource header
with its payload instead of matching the declared table-then-data format. Update
ProcessResourceFiles so the logic around Resource.header.Serialize,
writer.Write(resource.data), and the matching Deserialize path writes all
headers first, then all resource data blocks; keep Header.OffsetOfResourceData
consistent with the new order so multi-resource .nanoresources files are encoded
correctly.
In `@nanoFramework.Tools.BuildTasks/Utilities/DebuggerHelper.cs`:
- Around line 23-42: The debugger helper currently reads the flag with a
user-scoped environment variable and then calls Debugger.Break unconditionally,
which breaks on Unix-like systems and after timeout even when no debugger is
attached. Update DebuggerHelper to use the process-scoped
Environment.GetEnvironmentVariable overload for the debug flag, and in the wait
loop path only call Debugger.Break when Debugger.IsAttached is true so the
timeout can exit cleanly without interrupting execution.
In `@test/BuildTasks.Tests/Infrastructure/TestBuildEngine.cs`:
- Around line 42-46: Initialize the TestTask.BuildEngine property in
TestBuildEngine so it is usable by default, since TaskLoggingHelper construction
can fail when BuildEngine is left null. Update the TestTask helper to provide a
non-null default implementation or stub for IBuildEngine, keeping TestTask and
its Execute method unchanged so tests can instantiate it without extra setup.
In `@test/BuildTasks.Tests/NanoResxResourceReaderTests.cs`:
- Around line 35-40: Avoid disposing the backing StringReader before
NanoResxResourceReader is consumed; ParseResx currently creates the reader with
a using scope, so the source is closed too early and the tests depend on eager
parsing. Update ParseResx and any related test helpers in
NanoResxResourceReaderTests to keep the StringReader alive for the full lifetime
of the returned NanoResxResourceReader, so enumeration can happen after
construction without relying on implementation details.
In `@test/BuildTasks.Tests/ProcessResourceFilesTests.cs`:
- Around line 27-31: Update the test class comment in ProcessResourceFilesTests
so it matches the actual compile-time guard and runtime coverage. The note above
ReadResources/WriteResources currently says both net8.0 and net472 are
exercised, but this file is wrapped in NETFRAMEWORK only, so revise the wording
to describe framework-only compilation/coverage accurately. Use the
ProcessResourceFilesTests and ReadResources/WriteResources symbols to locate the
comment and keep the explanation consistent with the conditional compilation.
🪄 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: e83d5fc5-26d7-49b8-8f32-5590349aef8c
⛔ Files ignored due to path filters (2)
test/SmokeTest/data.binis excluded by!**/*.bintest/SmokeTest/logo.bmpis excluded by!**/*.bmp
📒 Files selected for processing (20)
azure-pipelines.ymlnanoFramework.NET.Sdk.slnnanoFramework.NET.Sdk/Sdk/nanoFramework.Resources.targetsnanoFramework.NET.Sdk/nanoFramework.NET.Sdk.csprojnanoFramework.Tools.BuildTasks/GenerateBinaryOutputTask.csnanoFramework.Tools.BuildTasks/GenerateNanoResourceTask.Stub.csnanoFramework.Tools.BuildTasks/GenerateNanoResourceTask.csnanoFramework.Tools.BuildTasks/ProcessResourceFiles.CrossPlatform.csnanoFramework.Tools.BuildTasks/ProcessResourceFiles.csnanoFramework.Tools.BuildTasks/ResolveRuntimeDependenciesTask.csnanoFramework.Tools.BuildTasks/TasksConstants.csnanoFramework.Tools.BuildTasks/Utilities/DebuggerHelper.csnanoFramework.Tools.BuildTasks/nanoFramework.Tools.BuildTasks.csprojtest/BuildTasks.Tests/BuildTasks.Tests.csprojtest/BuildTasks.Tests/Infrastructure/TestBuildEngine.cstest/BuildTasks.Tests/NanoResxResourceReaderTests.cstest/BuildTasks.Tests/ProcessResourceFilesTests.cstest/BuildTasks.Tests/Rgb565ConversionTests.cstest/SmokeTest/Resources.resxtest/SmokeTest/SmokeTest.csproj
💤 Files with no reviewable changes (1)
- nanoFramework.Tools.BuildTasks/GenerateNanoResourceTask.Stub.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Tools.BuildTasks-2019intosrc\nanoFramework.Tools.BuildTasks\.<Compile>links and comment block fromnanoFramework.Tools.BuildTasks.csproj.GenerateNanoResourceTask.Stub.cs.ProcessResourceFiles.CrossPlatform.cs: replacingSystem.Drawing,AppDomain,BinaryFormatter, and WinFormsResXResourceReaderwithSixLabors.ImageSharp,System.Resources.Extensions,System.Text.Json, and direct XML parsing.PackageReferences forSixLabors.ImageSharp,System.Resources.Extensions,System.CodeDom.tasks\net8.0\innanoFramework.NET.Sdk.csproj.nanoFramework.Resources.targetswiring.BuildTasks.Testswith unit tests forNanoResxResourceReader,ProcessResourceFiles, and RGB565 conversion.SmokeTestresources (.resx, bitmap, binary blob) for end-to-end validation.Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist: