Add macOS nanoCLR host target scaffold#3256
Add macOS nanoCLR host target scaffold#3256asp2286 wants to merge 9 commits intonanoframework:mainfrom
Conversation
- Adds targets/osx with a CMake+Ninja entrypoint - Includes minimal host glue and smoke CLI behavior - Adds GitHub Actions workflow for macOS build and smoke run - No shared CLR runtime integration in this initial scaffold Signed-off-by: Alexey Sokolov <asp2286@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a macOS host nanoCLR scaffold under Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In @.github/workflows/osx-nanoclr.yml:
- Around line 36-38: The "Smoke Run" step runs
"./build/osx/bin/nanoFramework.nanoCLR --version | tee build/osx/smoke.log" but
the pipe hides a non-zero exit from nanoFramework.nanoCLR; change the step so
the shell uses pipefail (e.g., run the pipeline under bash with -o pipefail or
run "set -o pipefail" before the pipeline) or explicitly check PIPESTATUS[0]
after the tee and exit with that status; update the "Smoke Run" command
accordingly so the step fails when the smoke executable returns non-zero.
In `@targets/osx/Include/target_platform.h`:
- Around line 6-13: The header target_platform.h currently defines macros
NANOCLR_HOST_POSIX, NANOCLR_HOST_OSX, and NANOCLR_HOST_WINDOWS which are also
injected by the build via target_compile_definitions (CMakeLists), causing macro
redefinition warnings; fix this by removing the three `#define` lines for
NANOCLR_HOST_POSIX, NANOCLR_HOST_OSX and NANOCLR_HOST_WINDOWS from
target_platform.h and keep the include guard only (or alternatively remove the
corresponding target_compile_definitions in the CMakeLists if you prefer the
header as the single source of truth).
In `@targets/osx/nanoCLR/CLRStartup.cpp`:
- Around line 13-21: The local CLR_SETTINGS struct definition is missing the
conditional compilation used in the canonical nanoCLR_Application.h, causing a
layout mismatch; update the CLR_SETTINGS typedef to wrap the
PerformGarbageCollection and PerformHeapCompaction fields with the same
conditional compilation by surrounding those two fields with `#if`
defined(VIRTUAL_DEVICE) ... `#endif` so their presence matches the canonical
definition and prevents memory corruption when the shared runtime expects the
conditional layout.
In `@targets/osx/nanoCLR/FileStore_OSX.cpp`:
- Around line 15-23: The function (nanoCLR_OSX_LoadFile) currently reads into
content using std::istreambuf_iterator but doesn't detect I/O errors; after
performing content.assign(...) check the stream state (e.g., in.fail() or
in.bad()) and ensure the failure wasn't only EOF — if a non-EOF read error
occurred, set error to a descriptive message, clear/rollback content, and return
false; reference the ifstream variable "in" and the content.assign(...) call to
locate where to insert this post-read validation.
In `@targets/osx/nanoCLR/Memory.cpp`:
- Around line 32-34: The code sets SizeInBytes to c_DefaultHeapSizeBytes even
when allocation fails (s_MemoryStart == nullptr), which lets callers assume a
valid non-zero buffer; change the logic in the allocation path that assigns
BaseAddress = s_MemoryStart and SizeInBytes = static_cast<unsigned
int>(c_DefaultHeapSizeBytes) so that SizeInBytes is only set when s_MemoryStart
is non-null (otherwise set SizeInBytes to 0), and apply the same fix/pattern to
the CustomHeapLocation allocation path; update the assignments around
BaseAddress, SizeInBytes, s_MemoryStart and c_DefaultHeapSizeBytes accordingly.
In `@targets/osx/nanoCLR/platform_heap.cpp`:
- Around line 8-16: The OSX platform implementation of platform_malloc and
platform_free is missing the nanoHAL_v2.h declarations; add `#include`
<nanoHAL_v2.h> at the top of targets/osx/nanoCLR/platform_heap.cpp so the extern
"C" signatures for platform_malloc and platform_free match other platforms and
prevent linkage/signature mismatches when used across translation units.
In `@targets/osx/nanoCLR/targetHAL_Time.cpp`:
- Around line 21-28: In HAL_Time_CurrentSysTicks(), replace the use of
CLOCK_REALTIME with CLOCK_MONOTONIC when calling clock_gettime to obtain a
monotonic tick source; update the call to clock_gettime(CLOCK_MONOTONIC, &ts)
and keep the existing conversion of ts.tv_sec and ts.tv_nsec to .NET ticks (the
return expression) unchanged so the function continues to return 100ns units.
In `@targets/osx/nanoCLR/targetPAL_Events.cpp`:
- Around line 24-37: Events_SetBoolTimer currently writes *timerCompleteFlag
from a detached thread causing a data race; fix by treating the pointed bool as
an atomic and perform atomic stores instead of plain writes: after the nullptr
check use
std::atomic_store_explicit(reinterpret_cast<std::atomic<bool>*>(timerCompleteFlag),
false, std::memory_order_release) and in the detached thread use
std::atomic_store_explicit(reinterpret_cast<std::atomic<bool>*>(timerCompleteFlag),
true, std::memory_order_release); keep the function name Events_SetBoolTimer and
parameter timerCompleteFlag unchanged, and add a comment noting callers must
pass a bool that is actually an std::atomic<bool> (or update callers to use
std::atomic<bool>) if you cannot change the API.
In `@targets/osx/nanoCLR/targetPAL_Time.cpp`:
- Around line 14-18: The Time_SetCompare definition has C linkage in the header
but is implemented without extern "C"; update the implementation of
Time_SetCompare to use C linkage (e.g., add extern "C" for the function or wrap
it in an extern "C" block) so the definition matches the declaration in
nanoPAL_Time.h and prevents linkage mismatches for Time_SetCompare.
In `@targets/osx/nanoCLR/targetRandom.cpp`:
- Around line 15-19: The function nanoCLR_OSX_NextRandomDouble can return 1.0
when randomDevice() yields 0xFFFFFFFFU; change the scaling so the result is in
[0,1) by dividing by 2^32 (e.g. use 0x100000000ULL or cast to double and divide
by pow(2,32)) or use std::uniform_real_distribution<double>(0.0,
std::nextafter(1.0, 0.0)) to ensure the upper bound is exclusive; update
nanoCLR_OSX_NextRandomDouble to use one of these approaches so callers never
receive exactly 1.0.
🧹 Nitpick comments (10)
targets/osx/Include/targetHAL_Power.h (1)
11-14:CPU_Reset()always exits with success code 0.If
CPU_Reset()is ever called in response to a fault or error condition,std::exit(0)will signal success to the OS/CI, potentially masking failures. Consider accepting an exit code parameter or using a non-zero default. For the current smoke-test scaffold this is acceptable, but worth noting for future runtime integration.targets/osx/nanoCLR/Target_HAL.cpp (1)
9-9: Parameter should beconst char *.
szTextis not modified by this function — it's only used to create a copy. The parameter type should beconst char *for const-correctness. Additionally, thenetcoretarget'sDebugPrintCallbacktypedef expectsconst char *, making this change consistent with that contract.Proposed fix
-void HAL_Windows_Debug_Print(char *szText) +void HAL_Windows_Debug_Print(const char *szText)targets/osx/nanoCLR/targetPAL_Time.cpp (1)
6-12: Missing include of the corresponding headertargetPAL_time.h.The AI summary mentions that
targets/osx/Include/targetPAL_time.hdeclares the public API for these functions. Including it here would ensure the definitions match the declarations (return types, linkage, signatures) and catch mismatches at compile time.`#include` <atomic> `#include` <cstdint> +#include "targetPAL_time.h".github/workflows/osx-nanoclr.yml (1)
3-11: Consider adding branch filters to avoid running on every push to every branch.Currently the workflow triggers on push to any branch that touches
targets/osx/**. If contributors push frequently to feature branches, this could consume CI minutes. Consider restricting push triggers to specific branches (e.g.,main,develop), while keeping thepull_requesttrigger broad.targets/osx/nanoCLR/CLRStartup.cpp (1)
31-36:ClrStartuptakes the struct by value — consider taking byconstreference or pointer.This matches the pattern used in other targets, but worth verifying. If the canonical signature uses a pointer or reference, this mismatch would cause link errors later.
targets/osx/README.md (2)
7-7: Replace PR-centric language with permanent documentation phrasing.Lines 7, 13, 28, etc. use "This PR provides…" / "This PR:" which becomes confusing after merge. Consider rephrasing to "This target provides…" or "This scaffold includes…".
41-43: Wrap build commands in fenced code blocks.The build commands aren't inside ``` fences, so they render as plain paragraphs rather than copyable command blocks in most Markdown renderers.
Proposed fix
-cmake -S targets/osx -B build/osx -G Ninja -DNANO_OSX_ARCH=arm64 -DNANO_OSX_ENABLE_SMOKE=ON -cmake --build build/osx -./build/osx/bin/nanoFramework.nanoCLR --version +```bash +cmake -S targets/osx -B build/osx -G Ninja -DNANO_OSX_ARCH=arm64 -DNANO_OSX_ENABLE_SMOKE=ON +cmake --build build/osx +./build/osx/bin/nanoFramework.nanoCLR --version +```targets/osx/nanoCLR/Various.cpp (2)
18-29: Global lock stubs are internally inconsistent.
HAL_Windows_AcquireGlobalLock()is a no-op, yetHAL_Windows_HasGlobalLock()always returnsfalse. If any caller acquires and then checks, the invariant is violated. Fine for a scaffold with no runtime wired, but worth a// TODOso this isn't forgotten when the runtime is integrated.
77-99: Use POSIXstrcasecmpinstead of hand-rolling case-insensitive compare.macOS provides
strcasecmpin<strings.h>, which handles locale-aware comparison and is well-tested. The manual loop is unnecessary on a POSIX platform.Proposed fix
+#include <strings.h> + int hal_stricmp(const char *dst, const char *src) { - int f = 0; - int l = 0; - - do - { - f = static_cast<unsigned char>(*(dst++)); - l = static_cast<unsigned char>(*(src++)); - - if (f >= 'A' && f <= 'Z') - { - f -= 'A' - 'a'; - } - - if (l >= 'A' && l <= 'Z') - { - l -= 'A' - 'a'; - } - } while (f && (f == l)); - - return f - l; + return strcasecmp(dst, src); }targets/osx/nanoCLR/targetPAL_Events.cpp (1)
87-110:Events_WaitForEventsreturns matched bits without clearing them.
Events_Getclears the matched bits froms_systemEvents, butEvents_WaitForEventsonly reads them. If callers expect wait-then-consume semantics, those events will fire repeatedly. If this is intentional (peek semantics, caller usesEvents_Getafterward), a brief comment would clarify the contract.
- Make CI smoke step fail on non-zero exit (pipefail) - Remove redundant host macro defines from target_platform.h - Align CLR_SETTINGS layout with canonical definition - Improve file read error handling and heap allocation reporting - Use monotonic clock for sys ticks and fix C linkage for Time_SetCompare - Ensure random double is in [0,1) and apply minor docs/const-correctness fixes Signed-off-by: Alexey Sokolov <asp2286@gmail.com>
- Align Events_SetBoolTimer behavior with Win32 assumption of a single active timer - Prevent earlier timers from completing after a newer one is scheduled (generation cancel) - Keep implementation minimal as a scaffold placeholder until completion queue wiring Signed-off-by: Alexey Sokolov <asp2286@gmail.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@targets/osx/Include/nanoHAL_Capabilites.h`:
- Around line 1-9: The filename "nanoHAL_Capabilites.h" is misspelled (missing
'i') and mismatches the include guard NANOHAL_CAPABILITIES_H; rename all four
files named nanoHAL_Capabilites.h to nanoHAL_Capabilities.h (locations:
src/HAL/Include, src/HAL, targets/osx/Include, targets/win32/Include) and update
the three includes that reference the old name in src/HAL/nanoHAL_Capabilites.c,
src/HAL/Include/nanoHAL_v2.h, and src/HAL/nanoHAL_Boot.c to `#include`
"nanoHAL_Capabilities.h" so the filename matches the existing guard and all
references.
🧹 Nitpick comments (6)
targets/osx/nanoCLR/targetPAL_Events.cpp (1)
87-95:notify_all()after clearing events is unnecessary.Waiters in
Events_WaitForEventsare predicated on events being set, not cleared. Notifying after a clear only causes spurious wakeups — the predicate will re-evaluate and the waiters will go back to sleep. Harmless, but wasteful.♻️ Suggested simplification
void Events_Clear(uint32_t events) { - { - std::lock_guard<std::mutex> lock(s_eventsMutex); - s_systemEvents &= ~events; - } - - s_eventsCondition.notify_all(); + std::lock_guard<std::mutex> lock(s_eventsMutex); + s_systemEvents &= ~events; }targets/osx/nanoCLR/targetRandom.cpp (1)
21-33:FillRandomBytesdiscards 24 bits of entropy per iteration.Each call to
randomDevice()produces 32 bits but only 8 are used. Fine for a scaffold, but when this moves beyond placeholder status, consider filling 4 bytes per call.♻️ Optional optimization for later
void nanoCLR_OSX_FillRandomBytes(uint8_t *buffer, unsigned int count) { if (buffer == nullptr) { return; } static std::random_device randomDevice; - for (unsigned int i = 0; i < count; i++) + unsigned int i = 0; + for (; i + 4 <= count; i += 4) { - buffer[i] = static_cast<uint8_t>(randomDevice()); + uint32_t val = randomDevice(); + std::memcpy(&buffer[i], &val, 4); + } + if (i < count) + { + uint32_t val = randomDevice(); + std::memcpy(&buffer[i], &val, count - i); } }targets/osx/Include/targetHAL_Power.h (1)
11-19:CPU_IsSoftRebootSupported()returnstruebutCPU_Reset()terminates the process.If a caller checks
CPU_IsSoftRebootSupported()and then callsCPU_Reset()expecting a soft restart, it will instead get a hard exit. This is fine for the current scaffold scope, but worth a TODO so it isn't forgotten when wiring in the runtime.targets/osx/nanoCLR/main.cpp (1)
27-43:main.cppduplicates and diverges fromParseOptions_OSX.cppargument parsing.
main.cpprolls its own argument loop whileParseOptions_OSX.cppprovides a dedicated parser (nanoCLR_OSX_ParseOptions) that also handles-loadand reports errors for unknown flags. Here, unknown arguments are silently ignored. Consider wiring the dedicated parser intomainto keep behaviour consistent and avoid maintaining two parallel parsing paths.targets/osx/nanoCLR/ParseOptions_OSX.cpp (1)
9-14:NanoClrHostOptionsandnanoCLR_OSX_ParseOptionslack a shared header.The struct and function are defined entirely in this
.cppfile with no corresponding header, so no other translation unit (e.g.,main.cpp) can reference them without redeclaring the types. Consider extracting the struct and function declaration into a header (e.g.,ParseOptions_OSX.h) so the parser can actually be consumed by the launcher.Also applies to: 17-20
targets/osx/nanoCLR/CLRStartup.cpp (1)
13-23: LocalCLR_SETTINGStypedef duplicates the canonical definition — track parity.This is a verbatim copy of the struct from
nanoCLR_Application.h. If fields are added or reordered upstream, silent layout mismatches will cause memory corruption at integration time. The TODO on lines 8-9 already acknowledges this, but consider adding a compile-time guard (e.g., astatic_assertonsizeof) once the shared header becomes available, to catch drift early.
Thanks for the review! I’ve added brief comments clarifying the intent and temporary scaffold nature of the macOS host implementation. The focus here is readability and parity with existing host targets rather than meeting a global coverage threshold. Happy to expand documentation further if needed. |
|
Tip For best results, initiate chat on the files or code changes.
The docstring coverage check is helpful for production code, but for a foundational scaffold like this where the focus is on buildable infrastructure and CI validation, inline comments and the README provide sufficient context. Feel free to ping me if you'd like a closer look at any specific area as you wire in the shared runtime in future PRs. Looking forward to seeing this evolve! 🚀 🧠 Learnings used |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
- Add BUILD_OSX gating in TargetsToBuild based on targets/osx changes - Add Build_OSX_nanoCLR job on macos-14 running CMake+Ninja smoke build and --version - Keep job path-filtered to avoid impacting unrelated PRs Signed-off-by: Alexey Sokolov <asp2286@gmail.com>
@Ellerbach Thanks for triggering the Azure pipeline. Looking ahead, would it make sense to add a path-filtered Happy to align with the project direction. |
Good question. I think we can find a compromise so that it does not run all the time but runs like some of the devices time to time. Reason is that it's a bit costly to run it all the time. And there are high chances if now one touches, it should be fine. So, as a gate but not all the time. Does that makes sense? |
Thanks for the clarification — that makes sense 👍 I've added a path-filtered If over time we see shared changes affecting it, we can consider widening the filter or adding a scheduled validation run. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@azure-pipelines.yml`:
- Around line 1047-1055: The Build_OSX_nanoCLR job's condition must be aligned
with other target jobs: add a guard to skip when SKIP_BUILD is true (or
StartReleaseCandidate implies skipping), honour BUILD_ALL=true, and allow a
manual trigger via the BUILD_OSX__ variable. Update the job's condition (the
existing condition referencing
dependencies.Check_Build_Options.outputs['TargetsToBuild.BUILD_OSX']) to also
check ne(variables['SKIP_BUILD'],'true') (or equivalent StartReleaseCandidate
guard), include an OR branch that accepts eq(variables['BUILD_ALL'],'true'), and
accept eq(variables['BUILD_OSX__'],'true') for manual builds so it matches the
logic used by Build_WIN32_nanoCLR and other targets. Ensure dependsOn remains or
is adjusted to include Check_Build_Options like the other jobs.
- Around line 346-358: The Check_Code_Style job's condition is missing the
BUILD_OSX flag so OSX-only changes skip linting; update the Check_Code_Style
condition to include
eq(dependencies.Check_Build_Options.outputs['TargetsToBuild.BUILD_OSX'], true)
alongside the other target checks, then add Check_Code_Style to the
Build_OSX_nanoCLR job's dependsOn and wrap Build_OSX_nanoCLR's existing
condition with succeeded('Check_Code_Style') so the OSX build waits for and
respects the style check result.
- Around line 1063-1073: Add an explicit validation after running
./build/osx/bin/nanoFramework.nanoCLR --version: capture its stdout (from the
tee'd build/osx/smoke.log) and grep/assert it contains an expected marker (e.g.,
"nanoCLR" or a semantic version regex) and exit non‑zero if the check fails so
the pipeline fails on unexpected output; additionally, avoid an unpinned
Homebrew install by installing a specific formula/version or switching to a
reproducible installer for ninja (replace the plain brew install ninja
invocation) — update the bash block that runs cmake/cmake --build and the final
--version check to include these assertions and the pinned ninja install
approach.
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@azure-pipelines.yml`: - Around line 346-358: The Check_Code_Style job's condition is missing the BUILD_OSX flag so OSX-only changes skip linting; update the Check_Code_Style condition to include eq(dependencies.Check_Build_Options.outputs['TargetsToBuild.BUILD_OSX'], true) alongside the other target checks, then add Check_Code_Style to the Build_OSX_nanoCLR job's dependsOn and wrap Build_OSX_nanoCLR's existing condition with succeeded('Check_Code_Style') so the OSX build waits for and respects the style check result. - Around line 1063-1073: Add an explicit validation after running ./build/osx/bin/nanoFramework.nanoCLR --version: capture its stdout (from the tee'd build/osx/smoke.log) and grep/assert it contains an expected marker (e.g., "nanoCLR" or a semantic version regex) and exit non‑zero if the check fails so the pipeline fails on unexpected output; additionally, avoid an unpinned Homebrew install by installing a specific formula/version or switching to a reproducible installer for ninja (replace the plain brew install ninja invocation) — update the bash block that runs cmake/cmake --build and the final --version check to include these assertions and the pinned ninja install approach.azure-pipelines.yml (2)
346-358:Check_Code_Stylecondition does not includeBUILD_OSX— OSX-only PRs will skip style checks.Every other target flag (
BUILD_CHIBIOS,BUILD_ESP32, …,BUILD_WIN32) gates style checking;BUILD_OSXis absent, so PRs touching onlytargets/osxwill never runcheck-code-style. This may be acceptable during the scaffold phase, but worth tracking as a follow-up so new C/C++ files undertargets/osxget the same linting gate.Proposed fix
or( eq(dependencies.Check_Build_Options.outputs['TargetsToBuild.BUILD_ALL'], true), eq(dependencies.Check_Build_Options.outputs['TargetsToBuild.BUILD_CHIBIOS'], true), eq(dependencies.Check_Build_Options.outputs['TargetsToBuild.BUILD_ESP32'], true), eq(dependencies.Check_Build_Options.outputs['TargetsToBuild.BUILD_FREERTOS'], true), eq(dependencies.Check_Build_Options.outputs['TargetsToBuild.BUILD_TI'], true), eq(dependencies.Check_Build_Options.outputs['TargetsToBuild.BUILD_AZURERTOS'], true), - eq(dependencies.Check_Build_Options.outputs['TargetsToBuild.BUILD_WIN32'], true) + eq(dependencies.Check_Build_Options.outputs['TargetsToBuild.BUILD_WIN32'], true), + eq(dependencies.Check_Build_Options.outputs['TargetsToBuild.BUILD_OSX'], true) )If added, also add
Check_Code_StyletoBuild_OSX_nanoCLR'sdependsOnand wrap its condition withsucceeded('Check_Code_Style')like the other jobs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@azure-pipelines.yml` around lines 346 - 358, The Check_Code_Style job's condition is missing the BUILD_OSX flag so OSX-only changes skip linting; update the Check_Code_Style condition to include eq(dependencies.Check_Build_Options.outputs['TargetsToBuild.BUILD_OSX'], true) alongside the other target checks, then add Check_Code_Style to the Build_OSX_nanoCLR job's dependsOn and wrap Build_OSX_nanoCLR's existing condition with succeeded('Check_Code_Style') so the OSX build waits for and respects the style check result.
1063-1073: Build step looks solid; consider validating the exit code explicitly.
set -euo pipefailis good — any non-zero exit will fail the step. One minor hardening note: if the--versionoutput is empty or unexpected, the step still passes. You could add a simple grep to assert the output contains the expected version string.Also,
brew install ninjais not pinned and could break if Homebrew updates Ninja to an incompatible version. This is low-risk for now but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@azure-pipelines.yml` around lines 1063 - 1073, Add an explicit validation after running ./build/osx/bin/nanoFramework.nanoCLR --version: capture its stdout (from the tee'd build/osx/smoke.log) and grep/assert it contains an expected marker (e.g., "nanoCLR" or a semantic version regex) and exit non‑zero if the check fails so the pipeline fails on unexpected output; additionally, avoid an unpinned Homebrew install by installing a specific formula/version or switching to a reproducible installer for ninja (replace the plain brew install ninja invocation) — update the bash block that runs cmake/cmake --build and the final --version check to include these assertions and the pinned ninja install approach.
- Make Build_OSX_nanoCLR respect SKIP_BUILD, BUILD_ALL, and manual triggers - Ensure Check_Code_Style runs for OSX-only changes and OSX job depends on it - Add basic smoke output validation Signed-off-by: Alexey Sokolov <asp2286@gmail.com>
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@azure-pipelines.yml`:
- Line 1076: The checkout step currently uses "checkout: self" without a shallow
clone; update that step by adding "fetchDepth: 1" under the "checkout: self"
entry (maintain the same YAML indentation) so the pipeline performs a shallow
clone like the other jobs; ensure no other checkout options are removed and that
the new "fetchDepth: 1" line is aligned with existing checkout properties.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@azure-pipelines.yml`: - Line 1076: The checkout step currently uses "checkout: self" without a shallow clone; update that step by adding "fetchDepth: 1" under the "checkout: self" entry (maintain the same YAML indentation) so the pipeline performs a shallow clone like the other jobs; ensure no other checkout options are removed and that the new "fetchDepth: 1" line is aligned with existing checkout properties.azure-pipelines.yml (1)
1076-1076: Consider addingfetchDepth: 1to the checkout step.Other jobs in this pipeline (e.g.,
Check_Code_Styleat line 370,Run_UnitTests_mscorlibat line 1327) usefetchDepth: 1to speed up checkout. Since this job only needs a shallow clone for the build, the same optimization applies here.Proposed fix
steps: - checkout: self + fetchDepth: 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@azure-pipelines.yml` at line 1076, The checkout step currently uses "checkout: self" without a shallow clone; update that step by adding "fetchDepth: 1" under the "checkout: self" entry (maintain the same YAML indentation) so the pipeline performs a shallow clone like the other jobs; ensure no other checkout options are removed and that the new "fetchDepth: 1" line is aligned with existing checkout properties.
Thanks, yes, let's do that! |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
@Ellerbach thanks again for triggering the Azure pipeline. I was able to reproduce the failure locally. I validated the fix on:
The pipeline now detects the agent architecture via |
|
/azp run |
|
Commenter does not have sufficient privileges for PR 3256 in repo nanoframework/nf-interpreter |
|
Hi @asp2286, Appreciate your work on putting up a MacOS target. As you probably know we have the nanoCLR distributed as nanoclr .NET tool. Have you tested this to run on MacOS? I'm asking because that's the target we've been using for quite some time to run a virtual device and that is used by the test framework too. There is an old WIN32 target of which the new nanoclr shares files with. The C# wrapper interacts with it through Interop. On a quick look at the PR, I got the impression that you're mixing old (like command parsing) and the new architecture. If the existing nanoclr tool does not run on MacOS, I would rather have "fixed" whatever is needed in the existing project(s) instead of creating another target. Don't want to sound like I'm dismissing your work and effort. Just looking at this from a broader perspective. That's one of the reasons we have in the contributor guideline a note asking developers to discuss with the core team breaking or big workloads before committing to an approach. No one likes to waste time and effort! 😉 |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Hi @josesimoes — thanks for the thoughtful feedback, totally fair questions. I did look into the existing
So at the moment My intent with If the preferred direction is: “make |
|
Just to clarify one architectural point: I don’t see In other words, the POSIX replacements and CMake plumbing can later be moved into the netcore backend during the actual porting effort, so we converge rather than diverge. |
|
From your comments it's clear that you've looked deep into the code base. A couple of thoughts:
To wrap up this: let's make targets/netcore backend cross-platform (CMake + portable shims) and ship per-RID native assets. |
…oader The nanoCLR host relied on kernel32 SetDllDirectory/FreeLibrary P/Invokes that only work on Windows. Replace them with a cross-platform resolver built on .NET's NativeLibrary API that probes standard paths on Windows, macOS, and Linux using consistent library naming without the lib prefix.
# Conflicts: # azure-pipelines.yml
…wrapper The C# layer (targets/netcore) owns all CLI argument parsing via CommandLineParser. The native host only needs to export P/Invoke entry points (nanoCLR_Run, nanoCLR_LoadAssembly, nanoCLR_GetVersion, etc.) — it must not duplicate parsing. - Delete targets/osx/nanoCLR/ParseOptions_OSX.cpp - Remove ParseOptions_OSX.cpp from targets/osx/nanoCLR/CMakeLists.txt - Remove NanoClrHostOptions struct and nanoCLR_OSX_ParseOptions declaration/call sites
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
josesimoes
left a comment
josesimoes
left a comment
There was a problem hiding this comment.
Looking good.
I need to test the part related with HHD interop DLL loading. I recall that one is very picky...
Apart from that, looking good.
There was a problem hiding this comment.
Pull request overview
Adds a new macOS host target scaffold under targets/osx and updates CI/build plumbing to compile and smoke-run it, alongside improving the .NET host’s native-library loading to be cross-platform friendly.
Changes:
- Introduces
targets/osxCMake/Ninja build with a minimal nanoCLR host “smoke” executable and placeholder PAL/HAL glue. - Adds CI coverage for macOS (GitHub Actions workflow + Azure Pipelines job/target-selection updates).
- Reworks .NET host native-library loading/unloading to use a
DllImportResolver(removingkernel32-only logic) and adjusts CLI startup path resolution.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| targets/osx/README.md | Documents macOS target scaffold, build instructions, and limitations. |
| targets/osx/CMakeLists.txt | Adds macOS-only CMake entrypoint and arch/smoke configuration. |
| targets/osx/nanoCLR/CMakeLists.txt | Defines macOS nanoCLR build target, includes, and compile defs. |
| targets/osx/nanoCLR/main.cpp | Smoke-mode entrypoint printing banner and scaffold status. |
| targets/osx/nanoCLR/CLRStartup.cpp | Stub CLR startup API surface for future runtime integration. |
| targets/osx/nanoCLR/Various.cpp | Placeholder host implementations for timing/printf/power/yield APIs. |
| targets/osx/nanoCLR/Target_HAL.cpp | Provides host debug print implementation. |
| targets/osx/nanoCLR/targetHAL_Time.cpp | Implements monotonic time and microsecond sleeps for macOS. |
| targets/osx/nanoCLR/targetPAL_Time.cpp | Placeholder compare-timer storage for future completion wiring. |
| targets/osx/nanoCLR/targetPAL_Events.cpp | Minimal event system with placeholder bool-timer support. |
| targets/osx/nanoCLR/targetRandom.cpp | Provides host randomness helpers. |
| targets/osx/nanoCLR/Memory.cpp | Allocates/fills placeholder heaps for host scaffold. |
| targets/osx/nanoCLR/platform_heap.cpp | Provides platform_malloc/free for host builds. |
| targets/osx/nanoCLR/Target_BlockStorage.cpp | Placeholder block storage hooks for host mode. |
| targets/osx/nanoCLR/FileStore_OSX.cpp | Minimal host file load/save helpers (scaffold). |
| targets/osx/nanoCLR/WatchDog.cpp | Stub watchdog functions for host scaffold. |
| targets/osx/nanoCLR/platform_selector.h | Defines macOS platform selector macro(s). |
| targets/osx/Include/targetHAL.h | Defines macOS target HAL macros/capabilities/time conversions. |
| targets/osx/Include/targetHAL_Time.h | Declares macOS HAL time functions. |
| targets/osx/Include/targetHAL_Power.h | Defines CPU reset/soft reboot support for host scaffold. |
| targets/osx/Include/TargetHAL_Spi.h | Defines SPI bus/device constants (placeholder). |
| targets/osx/Include/targetPAL_time.h | Adds macOS target PAL time header (matches win32 shape). |
| targets/osx/Include/target_platform.h | Adds macOS target platform header (placeholder). |
| targets/osx/Include/target_os.h | Declares target OS macros (no nanobooter). |
| targets/osx/Include/target_common.h | Declares target/platform info strings for macOS. |
| targets/osx/Include/target_board.h | Adds macOS target board header (placeholder). |
| targets/osx/Include/target_BlockStorage.h | Declares block storage count for macOS target. |
| targets/osx/Include/nanoHAL_Capabilites.h | Adds capabilities header required by shared includes (empty placeholder). |
| targets/osx/Include/nanoHAL_Boot.h | Adds boot header required by shared includes (empty placeholder). |
| .github/workflows/osx-nanoclr.yml | Adds GitHub Actions workflow to build + smoke-run on macos-14. |
| azure-pipelines.yml | Adds BUILD_OSX detection + macOS build/smoke job to Azure Pipelines. |
| targets/netcore/nanoFramework.nanoCLR.Host/Interop/NativeNanoClrLoader.cs | Adds cross-platform native library probing via DllImportResolver. |
| targets/netcore/nanoFramework.nanoCLR.Host/Interop/nanoCLR.cs | Switches to the new loader; removes kernel32 SetDllDirectory/FreeLibrary logic. |
| targets/netcore/nanoFramework.nanoCLR.CLI/Program.cs | Simplifies executing-path computation; removes kernel32 SetDllDirectory usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void PrintBanner() | ||
| { | ||
| std::cout << ".NET nanoFramework nanoCLR OSX v" << NANOCLR_OSX_VERSION_STRING << "\n"; | ||
| std::cout << "Copyright (c) .NET Foundation and Contributors\n\n"; | ||
| } | ||
| } // namespace | ||
|
|
||
| int main() | ||
| { | ||
| PrintBanner(); | ||
|
|
||
| #if NANO_OSX_ENABLE_SMOKE | ||
| // TODO: Replace with real CLR startup flow once shared runtime portability shims are in place. | ||
| std::cout << "Smoke mode is enabled. Runtime startup is not wired yet.\n"; | ||
| return 0; | ||
| #else | ||
| std::cerr << "Smoke mode is disabled and runtime startup is not available yet.\n"; | ||
| return 2; | ||
| #endif |
There was a problem hiding this comment.
main() ignores command-line arguments, but the README/CI invocation implies support for --help and --version. Consider switching to int main(int argc, char** argv) and implementing minimal parsing so --help/--version produce deterministic output (and optionally exit early) instead of always printing the smoke banner/message.
| void HAL_Windows_Debug_Print(const char *szText) | ||
| { | ||
| if (szText == nullptr) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| std::string text(szText); |
There was a problem hiding this comment.
HAL_Windows_Debug_Print is declared as void HAL_Windows_Debug_Print(char* szText) in the shared header (src/CLR/Include/nanoCLR_Win32.h). Defining it here as const char* will produce a different mangled symbol once shared runtime code is wired for macOS, leading to unresolved externals. Align the signature with the existing declaration (you can still treat the buffer as read-only internally).
| $"The macOS native library is not available. " + | ||
| $"Build it from targets/osx and place it alongside the tool, " + | ||
| $"or set the search directory explicitly via NativeNanoClrLoader."); |
There was a problem hiding this comment.
On macOS, the loader throws a DllNotFoundException suggesting the native library can be built from targets/osx, but this PR’s CMake target in targets/osx currently builds an executable (nanoFramework.nanoCLR), not a nanoFramework.nanoCLR.dylib. Either update the guidance (and/or the expected filename) to match the actual artifact produced today, or change the macOS build to produce the dylib that the host expects to load.
| $"The macOS native library is not available. " + | |
| $"Build it from targets/osx and place it alongside the tool, " + | |
| $"or set the search directory explicitly via NativeNanoClrLoader."); | |
| "The macOS native nanoFramework.nanoCLR library could not be found or loaded. " + | |
| "Ensure that the correct macOS native library is built and available on the native library search path, " + | |
| "or set the search directory explicitly via NativeNanoClrLoader."); |
| private static string NormalizeDirectory(string path) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(path)) | ||
| { | ||
| return string.Empty; | ||
| } | ||
|
|
||
| string fullPath = Path.GetFullPath(path); | ||
| string rootPath = Path.GetPathRoot(fullPath) ?? string.Empty; | ||
|
|
||
| if (string.Equals(fullPath, rootPath, StringComparison.Ordinal)) | ||
| { | ||
| return fullPath; | ||
| } | ||
|
|
||
| return fullPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); | ||
| } |
There was a problem hiding this comment.
NormalizeDirectory() calls Path.GetFullPath(path) without handling exceptions. If the caller passes an invalid/unresolvable path (e.g., bad characters, missing drive), this will throw during loader initialization and can mask the underlying native-load problem. Consider catching Exception types thrown by GetFullPath and returning an empty string or rethrowing a more actionable argument exception.
| void Events_SetBoolTimer(bool *timerCompleteFlag, uint32_t millisecondsFromNow) | ||
| { | ||
| // TODO: Placeholder timer implementation. Make this path thread-safe when runtime integration is wired. | ||
| if (timerCompleteFlag == nullptr) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| s_savedTimerCompleteFlag = timerCompleteFlag; | ||
| *timerCompleteFlag = false; | ||
| const uint64_t myGen = s_boolTimerGeneration.fetch_add(1, std::memory_order_acq_rel) + 1; | ||
|
|
||
| std::thread([millisecondsFromNow, myGen]() { | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(millisecondsFromNow)); | ||
|
|
||
| if (s_boolTimerGeneration.load(std::memory_order_acquire) != myGen) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (s_savedTimerCompleteFlag != nullptr) | ||
| { | ||
| // TODO: Scaffold placeholder until this is wired into a proper completion/event path. | ||
| *s_savedTimerCompleteFlag = true; | ||
| } | ||
| }).detach(); | ||
| } |
There was a problem hiding this comment.
Events_SetBoolTimer() spawns a detached std::thread on every call. If the runtime ends up resetting timers frequently, this can create many concurrent sleeping threads and unnecessary resource usage. Consider using a single background timer thread/dispatch queue, or a cancellable timer primitive, even in scaffold form.
josesimoes
left a comment
josesimoes
left a comment
There was a problem hiding this comment.
Looks good to me.
Want to address any of Copilot review points before merging?
Add macOS nanoCLR host target scaffold
Description
targets/osx.targets/win32structure.--help,--version) for build validation.macos-14.Motivation and Context
This change introduces a safe and incremental starting point for a native macOS host nanoCLR implementation.
It enables:
Resolves nanoframework/Home#1708
How Has This Been Tested?
--version.macos-14.Types of changes
Checklist
Summary by CodeRabbit
New Features
Chores