Skip to content

Metadata of native assemblies is now generated and exposed in virtual device #3023

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 27 commits into from
Sep 26, 2024

Conversation

frobijn
Copy link
Collaborator

@frobijn frobijn commented Sep 23, 2024

Description

  • A file native_assemblies.csv is generated when a firmware package is built. This is done by CMake that reads the data in the source files. The native_assemblies.csv file lists name, version and checksum for the included native assemblies.

  • New option --getnativeassemblies added to nanoclr.exe (and corresponding code to the native CLR host). This produces a list of name, version and checksum for the native assemblies included in the runtime embedded in nanoclr.exe or external file. Can still be used with "old" runtimes.

  • Rename --localinstance to --clrpath and made it common to reuse on all verbs.

It would be great if the latest version of each firmware package on cloudsmith would be re-created (with the same version number) so that the native_assemblies.csv files are part of the packages. I have no idea how to make that happen.

Motivation and Context

This is a PR related to the "controlled update" strategy as alternative for the current nanoFramework's "auto-update" strategy. It is required for the "controlled update" strategy to work, but there are no other PR dependent on this PR. All other software changes can be made and tested independent of this PR.

For a description of how the "controlled update" strategy would work, see the concept documentation in my fork.

For a description of all related changes required to make the "controlled update" strategy work, see a detailed description in another fork.

Long description

The changes in this repository have been made to get a list of names and versions of the native assemblies that are used in a runtime, without having to connect to a device via a serial port and retrieve the list via the wire protocol.

  • NanoCLR.exe: an option has been added: --getnativeassemblies, similar to (and can be combined with) --getversion. The result is a list of names, version and checksums sent to the standard output.

    It calls a method in the CLR host assembly to get information about the native assemblies (a .NET class). The data for that class is deserialized from data obtained via two additional methods in the native NanoCLR runtime that serializes the static constants in the runtime with native assembly information. If a tool requires the list of native assemblies, it can run the nanoclr.exe and parse its output.

  • NanoCLR.exe: added --clrinstancepath to use with --getnativeassemblies, so that the above keeps working if nanoclr.exe is updated but an older runtime is used. If the instance does not support the two additional methods in the native NanoCLR runtime, no details about the native assemblies is displayed, but instead a message (if verbosity is high enough).

  • NanoCLR.exe: changes have been tested by running the tool, for the "embedded" runtime, the same runtime via --clrinstancepath, and the current version of the runtime (from Cloudsmith) via --clrinstancepath.

  • CMake/Modules/FindNF_NativeAssemblies.cmake: create a native_assemblies.csv file in the build output directory of a firmware package that contains the names, version and checksums of the native assemblies selected to be part of the firmware.

    • The CMake code to extract version and checksum works well for the native assemblies that use the skeleton generated by the metadata processor. There are two native assemblies that define the checksum in a different way:

      • src\nanoFramework.Runtime.Events\nf_rt_events_native_nanoFramework_Runtime_Events_EventSink.cpp has a g_CLR_AssemblyNative_nanoFramework_Runtime_Events_EventSink_DriverProcs with a #define as checksum (DRIVER_INTERRUPT_METHODS_CHECKSUM). However, this is not the checksum of the native assembly corresponding to the nanoFramework.Runtime.Events .NET class library - that is located in src\nanoFramework.Runtime.Events\nf_rt_events_native.cpp. This exception can be ignored for our purpose.
      • src\CLR\CorLib\corlib_native.cpp: The checksum depends on NANOCLR_REFLECTION that corresponds to TARGET_NANOCLR_REFLECTION in CMake.As the corlib is already treated as a special (as the corlib is always included and treated differently in CMake), the parsing is also done differently.
    • File creation has been tested for a few firmware packages. As there does not seem to be a target with NF_FEATURE_SUPPORT_REFLECTION = OFF, that part of the CMake script could not be tested. The CMake regex has been tested by temporarily using it for the NF_FEATURE_SUPPORT_REFLECTION = ON case.

    • As .csv files are copied to the firmware .zip package, it is assumed the native_assemblies.csv will become part of the firmware package. This is not tested.

  • Added .editorconfig.

How Has This Been Tested?

See long description.

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)
  • Dev Containers (changes related with Dev Containers, has no impact on code or features)
  • Dependencies/declarations (update dependencies or assembly declarations and changes associated, 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).

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a new .editorconfig file to enforce consistent coding styles across the project.
    • Added properties to ClrInstanceOperationsOptions for retrieving native assembly details and specifying local instances.
    • New methods for obtaining native assembly counts and information have been added to enhance interop functionality.
    • A new class NativeAssemblyDetails has been introduced to encapsulate information about native assemblies.
    • Added functionality to create a CSV file containing native assembly versions.
  • Bug Fixes

    • Improved error handling for local CLR instance validation.
  • Documentation

    • Updated licensing comments to reflect MIT licensing across various files.
    • Added a spelling exclusion dictionary to improve spell-check accuracy.
  • Chores

    • Introduced a spelling exclusion dictionary to improve spell-check accuracy.

…s built. This is done by CMake that reads the data in the source files. The native_assemblies.csv file lists name, version and checksum for the included native assemblies.

- Extra options --getnativeassemblies and --clrinstancepath added to nanoclr.exe (and corresponding code to the native CLR host). This produces a list of name, version and checksum for the native assemblies included in the runtime embedded in nanoclr.exe or external file. Can still be used with "old" runtimes.

- .editorconfig added

It would be great if the latest version of each firmware package on cloudsmith would be re-created (with the same version number) so that the `native_assemblies.csv` files are part of the packages. I have no idea how to make that happen.

Signed-off-by: Frank Robijn <robijn@good-heavens.nl>
@nfbot nfbot changed the title Easy access to the metadata of the native assemblies in a firmware package / virtual device runtime. Easy access to the metadata of the native assemblies in a firmware package / virtual device runtime Sep 23, 2024
Copy link

coderabbitai bot commented Sep 23, 2024

Walkthrough

The pull request introduces a new .editorconfig file that establishes coding style and formatting rules for C# and other file types. It adds macros in CMake for managing versions of native assemblies and updates several classes to include new properties and methods for handling CLR instance operations and native assembly management. Licensing comments are standardized to the MIT license across multiple files, and a new spelling exclusion dictionary is introduced. Additionally, new functionality for retrieving native assembly information is implemented, while a core native interface file is removed, indicating a significant structural change.

Changes

File(s) Change Summary
.editorconfig Introduces coding style and formatting rules for C#, including end-of-line characters, naming conventions, and preferences for various file types.
CMake/Modules/FindNF_NativeAssemblies.cmake Adds macros for extracting and appending version information for native assemblies and CorLib, modifies existing macros to include these new functionalities.
spelling_exclusion.dic Introduces a new file containing the word "nano" for spelling exclusions.
targets/netcore/nanoFramework.nanoCLR.CLI/ClrInstanceOperationsOptions.cs Adds property GetNativeAssemblies to manage native assembly retrieval and updates class inheritance.
targets/netcore/nanoFramework.nanoCLR.CLI/ClrInstanceOperationsProcessor.cs Enhances logic for handling local CLR instances, including validation and output of CLR and native assembly details.
targets/netcore/nanoFramework.nanoCLR.CLI/CommonOptions.cs Introduces a new class for command-line options, including properties for CLR instance path and verbosity level.
targets/netcore/nanoFramework.nanoCLR.CLI/ExecuteCommandLineOptions.cs Updates class to inherit from CommonOptions and removes unnecessary properties.
targets/netcore/nanoFramework.nanoCLR.CLI/ExecuteCommandProcessor.cs Renames variable to reflect new property for CLR instance path.
targets/netcore/nanoFramework.nanoCLR.CLI/ExitCode.cs Adds new enum values for error codes related to CLR instance updates and native assembly retrieval.
targets/netcore/nanoFramework.nanoCLR.Host/Interop/nanoCLR.cs Updates licensing comments and adds external method declarations for retrieving native assembly count and information.
targets/netcore/nanoFramework.nanoCLR.Host/NanoClrHostBuilder.cs Modifies licensing comments and adds a public method GetNativeAssemblies to retrieve native assembly details.
targets/netcore/nanoFramework.nanoCLR.Host/NativeAssemblyDetails.cs Introduces a new class to encapsulate native assembly information, including properties and methods for retrieval and deserialization.
targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp Introduces new functions for retrieving the count and information of native assemblies, enhancing management capabilities.
targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.h Updates header file with new function declarations for native assembly count and information retrieval.

Possibly related PRs

  • Fixing FileSystem for ESP32 #2985: This PR addresses improvements in the FileSystem configuration for ESP32, which may relate to the overall project structure and coding standards that the main PR aims to enforce through the new .editorconfig file.
  • Improvements in littlefs for virtual device #2986: This PR enhances the littlefs functionality, which could be relevant to the coding style and formatting rules introduced in the main PR, particularly if it involves file handling in C#.
  • Minor improvements in virtual device #2990: This PR focuses on minor improvements in the CLR DLL update process, which may align with the coding standards and practices established in the main PR.
  • Add private implementation of vsnprintf #3011: This PR introduces a private implementation of vsnprintf, which may relate to the coding style and formatting rules in the main PR, especially if it involves string formatting in C#.
  • Remove private implementation of printf #3020: This PR removes the private implementation of printf, reverting to a more efficient implementation, which could be relevant to the coding standards and practices that the main PR aims to enforce.

Suggested labels

Area: Common libs, Type: documentation, Type: dependencies


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 13

Outside diff range and nitpick comments (12)
targets/netcore/nanoFramework.nanoCLR.CLI/ClrInstanceOperationsOptions.cs (1)

45-51: LGTM: New property for specifying local instance path

The LocalInstance property has been correctly implemented as a string option. Its purpose aligns with the PR objectives of ensuring compatibility with older runtimes. The Option attribute is properly configured, and the Hidden = true parameter is appropriate for this advanced option.

Consider slightly rewording the help text for clarity:

- HelpText = "Path to the directory where a local version of nanoFramework.nanoCLR.dll is located.")]
+ HelpText = "Path to the directory containing a local version of nanoFramework.nanoCLR.dll.")]
targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.h (1)

71-71: New function declaration looks good, consider adding documentation

The nanoCLR_GetNativeAssemblyCount() function declaration is well-formed and consistent with the existing code style. It's purpose is clear from the name, but consider adding a brief comment to describe its functionality and any potential limitations (e.g., maximum count).

Consider adding a brief documentation comment:

/// @brief Get the count of native assemblies
/// @return The number of native assemblies (max 65535)
extern "C" NANOCLRNATIVE_API uint16_t nanoCLR_GetNativeAssemblyCount();
targets/netcore/nanoFramework.nanoCLR.Host/NanoClrHostBuilder.cs (1)

94-96: LGTM: New method for accessing native assemblies

The new GetNativeAssemblies() method provides a clean way to access native assembly details, aligning with the PR objectives. The implementation is concise and uses modern C# syntax.

Consider adding XML documentation for this new public method to improve API usability. For example:

/// <summary>
/// Retrieves a list of native assembly details.
/// </summary>
/// <returns>A list of NativeAssemblyDetails objects.</returns>
public List<NativeAssemblyDetails> GetNativeAssemblies()
    => NativeAssemblyDetails.Get();
targets/netcore/nanoFramework.nanoCLR.Host/Interop/nanoCLR.cs (2)

96-97: LGTM: New method for retrieving native assembly information. Consider adding a comment about buffer size.

The nanoCLR_GetNativeAssemblyInformation method is well-defined and consistent with the existing code style. It aligns with the PR objectives to retrieve detailed native assembly information.

Consider adding a comment to clarify the expected buffer size or any potential risks of buffer overflow. For example:

/// <summary>
/// Retrieves information about native assemblies.
/// </summary>
/// <param name="buffer">Buffer to store the assembly information. Ensure it's large enough to hold all data.</param>
/// <param name="size">Size of the provided buffer.</param>
/// <remarks>
/// Caller is responsible for providing a buffer large enough to hold all assembly information.
/// Use nanoCLR_GetNativeAssemblyCount to determine the number of assemblies and allocate accordingly.
/// </remarks>

93-97: Overall impact: Positive addition with minimal risk. Consider updating documentation.

The new methods nanoCLR_GetNativeAssemblyCount and nanoCLR_GetNativeAssemblyInformation are well-integrated into the existing codebase. They provide the functionality described in the PR objectives without modifying existing code, which minimizes the risk of unintended side effects.

Consider the following suggestions:

  1. Update any relevant documentation or usage examples in the project to reflect these new capabilities.
  2. If not already planned, consider adding unit tests for the new functionality to ensure it behaves as expected.
  3. Verify that any code that will use these new methods handles potential exceptions (e.g., DllNotFoundException) appropriately.
targets/netcore/nanoFramework.nanoCLR.Host/NativeAssemblyDetails.cs (1)

70-70: Ensure compatibility of using var syntax with target frameworks

The use of using var requires C# 8.0 or later. Ensure that all target environments support this syntax. If not, consider using traditional using statements for broader compatibility.

targets/netcore/nanoFramework.nanoCLR.CLI/ClrInstanceOperationsProcessor.cs (3)

31-32: Provide informative error messages in exceptions

When throwing CLIException, consider including an informative message to help users understand the issue. For instance, specify that --localinstance and --updateclr options cannot be combined.


38-38: Provide informative error messages when the local instance path is invalid

When the specified local instance path does not exist, include a clear error message in the CLIException to inform the user that the directory was not found.


84-87: Inform the user when native assembly information is not available

If nativeAssemblies is null, the message indicating that the CLR instance is too old is only displayed at verbosity levels above normal. Consider showing this message at normal verbosity to inform users why no native assembly information is displayed.

.editorconfig (2)

15-15: Consider using UTF-8 without BOM for character encoding

Specifying charset = utf-8-bom may introduce issues with tools that do not expect a Byte Order Mark (BOM). It's generally recommended to use utf-8 without BOM to improve compatibility across different environments.

Apply this diff to adjust the encoding:

-charset = utf-8-bom
+charset = utf-8

64-64: Clarify the comment to prevent confusion

The comment on line 64 may be misleading because the word "this" can be mistaken for the C# this keyword. To enhance clarity, consider rephrasing the comment.

Apply this diff to improve the comment:

-# avoid this. unless absolutely necessary
+# Avoid using `this.` qualification unless absolutely necessary
CMake/Modules/FindNF_NativeAssemblies.cmake (1)

486-488: Ensure Proper Line Endings in Generated CSV File

The string(REPLACE ";" "\r\n" ...) command replaces semicolons with Windows-style line endings. If the build is performed on a Unix-based system, this could cause issues with tools expecting Unix-style line endings.

Consider using \n instead of \r\n for consistency across different environments, or configure the line endings based on the operating system.

Apply this diff to adjust the line endings:

-string(REPLACE ";" "\r\n" NF_NativeAssemblies_CSV "${NF_NativeAssemblies_VERSIONS}")
+string(REPLACE ";" "\n" NF_NativeAssemblies_CSV "${NF_NativeAssemblies_VERSIONS}")
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 888f098 and 50263be.

Files selected for processing (10)
  • .editorconfig (1 hunks)
  • CMake/Modules/FindNF_NativeAssemblies.cmake (6 hunks)
  • spelling_exclusion.dic (1 hunks)
  • targets/netcore/nanoFramework.nanoCLR.CLI/ClrInstanceOperationsOptions.cs (3 hunks)
  • targets/netcore/nanoFramework.nanoCLR.CLI/ClrInstanceOperationsProcessor.cs (3 hunks)
  • targets/netcore/nanoFramework.nanoCLR.Host/Interop/nanoCLR.cs (2 hunks)
  • targets/netcore/nanoFramework.nanoCLR.Host/NanoClrHostBuilder.cs (2 hunks)
  • targets/netcore/nanoFramework.nanoCLR.Host/NativeAssemblyDetails.cs (1 hunks)
  • targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp (2 hunks)
  • targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.h (3 hunks)
Files skipped from review due to trivial changes (1)
  • spelling_exclusion.dic
Additional comments not posted (14)
targets/netcore/nanoFramework.nanoCLR.CLI/ClrInstanceOperationsOptions.cs (3)

1-2: LGTM: Updated copyright notice

The copyright notice has been appropriately updated to reflect the MIT license. This change aligns with standard open-source practices and ensures proper attribution.


19-23: LGTM: New property for retrieving native assemblies information

The GetNativeAssemblies property has been correctly implemented as a boolean option. Its purpose aligns with the PR objectives of allowing users to obtain a list of native assemblies directly from the runtime. The Option attribute is properly configured with clear help text.


Line range hint 1-51: Summary: Changes align well with PR objectives

The modifications to ClrInstanceOperationsOptions.cs successfully implement the new features described in the PR objectives. The addition of GetNativeAssemblies and LocalInstance properties enhances the CLI's capabilities, allowing users to retrieve native assembly information and specify local runtime instances.

These changes contribute to the "controlled update" strategy mentioned in the PR summary by providing more flexibility and information to users. The implementation is clean, well-documented, and consistent with the existing code style.

targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.h (2)

Line range hint 1-69: Existing code structure looks good

The file is well-organized with proper include guards, API export macros, and clear function declarations. The existing code follows good C++ practices and provides a comprehensive API for the nanoCLR.


Line range hint 1-72: Overall, the changes look good and align with PR objectives

The additions to nanoCLR_native.h provide the necessary API extensions for accessing native assembly metadata, as described in the PR objectives. The new function declarations are consistent with the existing code style and integrate well with the current API.

Consider implementing the suggested improvements for documentation and error handling to enhance the API's usability and robustness.

targets/netcore/nanoFramework.nanoCLR.Host/NanoClrHostBuilder.cs (3)

1-2: LGTM: Updated license header

The license header has been appropriately updated to use the MIT license, which is clear and concise. This change aligns well with open-source best practices.


9-12: LGTM: Reordered using directives

The using directives have been slightly reordered. This change doesn't affect functionality and may improve code organization and readability.


Line range hint 1-140: Overall assessment: Enhancements align with PR objectives

The changes to NanoClrHostBuilder.cs successfully implement the new functionality for accessing native assembly details. The code modifications are well-structured, use modern C# features, and align with the PR objectives of providing easy access to metadata of native assemblies in a firmware package.

The new GetNativeAssemblies() method enhances the nanoCLRHostBuilder class by allowing users to retrieve native assembly details directly. This addition supports the "controlled update" strategy mentioned in the PR objectives.

The code changes maintain good coding practices and the overall structure of the file. The minor improvements to the license header and using directives contribute to better code organization.

targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp (2)

186-189: LGTM: Simple and effective implementation.

The function nanoCLR_GetNativeAssemblyCount provides a clean way to access the count of native assemblies. The use of uint16_t is appropriate for the expected range of assembly counts.


185-217: Overall, the changes align well with the PR objectives.

The new functions nanoCLR_GetNativeAssemblyCount and nanoCLR_GetNativeAssemblyInformation provide the necessary functionality to access metadata of native assemblies in the firmware package. This aligns with the PR's goal of facilitating a "controlled update" strategy for nanoFramework.

While the implementations are generally sound, consider the suggested improvements for nanoCLR_GetNativeAssemblyInformation to enhance safety and efficiency.

These additions will enable the runtime to provide information about native assemblies, supporting the new --getnativeassemblies option mentioned in the PR objectives.

targets/netcore/nanoFramework.nanoCLR.Host/Interop/nanoCLR.cs (1)

93-94: LGTM: New method for retrieving native assembly count.

The nanoCLR_GetNativeAssemblyCount method is well-defined and consistent with the existing code style. The use of ushort as the return type is appropriate for counting assemblies.

targets/netcore/nanoFramework.nanoCLR.Host/NativeAssemblyDetails.cs (1)

94-94: Verify the encoding used for assembly names

In the constructor, GetZeroTerminatedString is called with true for fUTF8, assuming the assembly names are UTF-8 encoded. Confirm that the runtime indeed provides names in UTF-8 encoding to prevent potential misinterpretation of the assembly names.

.editorconfig (1)

179-179: Confirm the license header template

The file_header_template is set to the .NET Foundation's license. Please verify that this reflects the correct licensing for the nanoFramework project. If a different license applies, update the template accordingly to ensure proper attribution.

CMake/Modules/FindNF_NativeAssemblies.cmake (1)

230-232: Verify Existence of Core Library Source Files

When adding versions for always-included assemblies, ensure that the specified source files exist to prevent file read errors.

Run the following script to confirm the source files are present:

Verification successful

Core Library Source Files Exist

Both corlib_native.cpp and nf_rt_native.cpp were found in the repository at their expected locations.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that core library source files exist.

# Expected: Both files should exist.

files=(
    "${CMAKE_SOURCE_DIR}/src/CLR/CorLib/corlib_native.cpp"
    "${CMAKE_SOURCE_DIR}/src/nanoFramework.Runtime.Native/nf_rt_native.cpp"
)

for file in "${files[@]}"; do
    if [ ! -f "$file" ]; then
        echo "Error: $file not found."
    else
        echo "Found: $file"
    fi
done

Length of output: 571


Script:

#!/bin/bash
# Description: Search for the missing core library source files in the repository.

fd "corlib_native.cpp" .
fd "nf_rt_native.cpp" .

Length of output: 135

Frank Robijn and others added 3 commits September 23, 2024 15:50
Automated fixes for code style.
…d90-142b-4847-aa77-bf3e8370fb56

Code style fixes for nanoframework/nf-interpreter PR#3023
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 50263be and 6fc83d8.

Files selected for processing (6)
  • CMake/Modules/FindNF_NativeAssemblies.cmake (6 hunks)
  • targets/netcore/nanoFramework.nanoCLR.CLI/ClrInstanceOperationsProcessor.cs (3 hunks)
  • targets/netcore/nanoFramework.nanoCLR.Host/Interop/nanoCLR.cs (2 hunks)
  • targets/netcore/nanoFramework.nanoCLR.Host/NanoClrHostBuilder.cs (2 hunks)
  • targets/netcore/nanoFramework.nanoCLR.Host/NativeAssemblyDetails.cs (1 hunks)
  • targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp (2 hunks)
Files skipped from review as they are similar to previous changes (5)
  • targets/netcore/nanoFramework.nanoCLR.CLI/ClrInstanceOperationsProcessor.cs
  • targets/netcore/nanoFramework.nanoCLR.Host/Interop/nanoCLR.cs
  • targets/netcore/nanoFramework.nanoCLR.Host/NanoClrHostBuilder.cs
  • targets/netcore/nanoFramework.nanoCLR.Host/NativeAssemblyDetails.cs
  • targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp
Additional comments not posted (1)
CMake/Modules/FindNF_NativeAssemblies.cmake (1)

135-140: LGTM: Correctly integrated AddNativeAssemblyVersion

The addition of the AddNativeAssemblyVersion call in the PerformSettingsForApiEntry macro is correct and well-integrated. This change ensures that the version information for each API is properly collected.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range and nitpick comments (1)
CMake/Modules/FindNF_NativeAssemblies.cmake (1)

486-491: LGTM: CSV file creation with native assembly versions

The implementation for creating the CSV file with native assembly versions is well done. It correctly formats the content and includes appropriate error handling for the file write operation.

A minor suggestion for improvement:
Consider adding a status message when the file is successfully written, to provide positive feedback in addition to the warning for failure.

You could add a success message like this:

if(WRITE_RESULT EQUAL 0)
    message(STATUS "Successfully wrote native_assemblies.csv file")
else()
    message(WARNING "Failed to write native_assemblies.csv file")
endif()
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 69513f9 and 4807232.

Files selected for processing (1)
  • CMake/Modules/FindNF_NativeAssemblies.cmake (6 hunks)
Additional context used
Learnings (1)
CMake/Modules/FindNF_NativeAssemblies.cmake (3)
Learnt from: frobijn
PR: nanoframework/nf-interpreter#3023
File: CMake/Modules/FindNF_NativeAssemblies.cmake:95-106
Timestamp: 2024-09-23T17:56:37.570Z
Learning: The `AddCorLibAssemblyVersion` macro in `CMake/Modules/FindNF_NativeAssemblies.cmake` does not require additional error handling for regex failures because the associated *.cpp files contain warnings against modification. Any changes to these files should be accompanied by corresponding updates to the CMake file.
Learnt from: frobijn
PR: nanoframework/nf-interpreter#3023
File: CMake/Modules/FindNF_NativeAssemblies.cmake:88-109
Timestamp: 2024-09-23T17:59:04.080Z
Learning: In `CMake/Modules/FindNF_NativeAssemblies.cmake`, when similar macros like `AddNativeAssemblyVersion` and `AddCorLibAssemblyVersion` have different logic, prefer keeping them separate to maintain readability rather than refactoring into a shared function.
Learnt from: frobijn
PR: nanoframework/nf-interpreter#3023
File: CMake/Modules/FindNF_NativeAssemblies.cmake:74-79
Timestamp: 2024-09-23T17:56:30.234Z
Learning: In the `AddNativeAssemblyVersion` macro, error handling for missing version information may not be necessary because the version format is generated and errors are unlikely. Additionally, native components not related to managed components may not need their native assembly metadata added to the list.
Additional comments not posted (4)
CMake/Modules/FindNF_NativeAssemblies.cmake (4)

67-83: LGTM: AddNativeAssemblyVersion macro implementation

The AddNativeAssemblyVersion macro is well-implemented. It correctly extracts the version information from the native assembly source files using a complex but accurate regex pattern. The extracted information is then appropriately appended to the NF_NativeAssemblies_VERSIONS list.


88-109: LGTM: AddCorLibAssemblyVersion macro implementation

The AddCorLibAssemblyVersion macro is well-implemented. It correctly handles the special case of the CorLib assembly, including:

  • Using a more complex regex pattern to capture two checksums.
  • Implementing conditional logic based on the NF_FEATURE_SUPPORT_REFLECTION flag.

The implementation aligns with the project's requirements and previous learnings regarding error handling.


139-140: LGTM: Integration of AddNativeAssemblyVersion in PerformSettingsForApiEntry

The modification to the PerformSettingsForApiEntry macro correctly integrates the new AddNativeAssemblyVersion macro. The parameters passed are accurate and consistent with the AddNativeAssemblyVersion definition, ensuring that the version information for each API is properly added to the list.


175-176: LGTM: Integration of AddNativeAssemblyVersion in PerformSettingsForInteropEntry

The modification to the PerformSettingsForInteropEntry macro correctly integrates the new AddNativeAssemblyVersion macro. The parameters passed are accurate and consistent with the AddNativeAssemblyVersion definition, ensuring that the version information for each Interop assembly is properly added to the list.

Copy link
Member

@Ellerbach Ellerbach left a comment

Choose a reason for hiding this comment

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

Thanks, couple of comments, mainly on the code style. I'll let José comment more about the rest. The implementation still looks good to me and aligned with the rest.

@josesimoes
Copy link
Member

Fixed the description sections to follow PR template.

@frobijn
Copy link
Collaborator Author

frobijn commented Sep 24, 2024

Fixed the description sections to follow PR template.

What was the problem? I stared at it for some time but didn't see what the issue was.

Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

I'm not a big fan of the new option to list the assemblies... 🤔
This is a deviation from all the other CLRs (on which this data is already possible to request) which makes it necessary to process it differently when connected to the virtual device and when connected to hardware.

Seems like a very very niche usage scenario that complicates the usage of, just because it deviates...

@@ -38,6 +42,14 @@ public class ClrInstanceOperationsOptions
HelpText = "Specify a version of nanoCRL to install.")]
public string TargetVersion { get; set; }

[Option(
Copy link
Member

Choose a reason for hiding this comment

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

This is a duplication of ExecuteCommandLineOptions.LocalInstance`, isn't it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes and no. It serves the same purpose. But the way the options are organised is per "verb", so per "instance" and "run", so a new option has to be introduced.

My choice was to name it differently than the LocalInstance option for "run", as the LocalInstance is quite tricky. The LocalInstance option is described as the specification of a file that should be used as runtime. But if you try that as a simple user and pass "\nanoclr-1.2.3.dll", that won't work. Nanoclr uses the directory of the path and always uses the file name "nanoFramework.nanoCLR.dll". There are good reasons for that if you look at the code, but as a user (at least for me) it was a surprise. Because of backward compatibility the existing LocalInstance should not be changed, but I decided to give the new option a better name, let the user pass the directory and mention in the description that the file name is fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Understood on the potential confusion about the path + name. That can be easily fixed, of course.
Now about the verbs, those are organized in that manner on purpose.

The CLI at the end of the day acts as a loader for a CLR instance. That's why you find the localinstance option under the run verb. Because one is requesting the tool to RUN that CLR instance (in lieu of the default one).

You're proposing to move this to the INSTANCE verb. That is not logical because, at that point of the execution its already running THAT instance that was previously requested. All this works perfectly garbled and mixed anyway considering that the CLI options are parsed and only after that the thing will actually do something.
Still, I would prefer to keep the consistency and have the options where they belong.

All for improving the option help text, documentation and code that processes it to simplify.

No strong opinion on allowing a DLL name different than nanoFramework.nanoCLR.dll... if that seems important... this was more a like an extra validation and using the default name seemed like a good approach to make the user life easier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're proposing to move this to the INSTANCE verb

No, not to move. Both verbs need this option:

  • If you have a runtime (dll) and want to know the native assemblies, there is no other way than to use "nanoclr.exe instance" that already provides information about the runtime and has the option to update the runtime. Either that or you create a separate tool for that.

  • If you want to use the runtime to run assemblies, you'd use the "nanoclr.exe run". This is also used by the test framework.

The way the CLI program is organized, there is no way to have common options. So I think the choices are: use the same name for both verbs (to stress that it is both concern the runtime DLL) or allow different names and use that to make one option a bit more clear.

No strong opinion on allowing a DLL name different than nanoFramework.nanoCLR.dll..

Maybe I don't understand how the various components work together, but I thought that the runtime DLL is the same one that is referred to by the various DLLImport declarations in the managed nanoFramework.nanoCLR.Host assembly. AFAIK if the DLL is a native DLL, the name used in the DLLImport must match the name of the DLL.

BTW I don't want to use a different file name, but the option suggests that it is possible. Put me on the wrong track.

Copy link
Member

Choose a reason for hiding this comment

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

Absolutely correct on that last part (I overlooked that) it must have that name because of the DLL import declarations as you've pointed out.

@frobijn
Copy link
Collaborator Author

frobijn commented Sep 24, 2024

I'm not a big fan of the new option to list the assemblies... 🤔

Actually it is worse than that. There are three ways a runtime is packaged:

  • for all real hardware: as a zip file, published to cloudsmith. This is the nicest way, as it was easy to add a .csv file to the package. Once you have the package, you have almost all information. Only the name of the platform is nowhere to be found in the package.

  • for the virtual device: as part of nanoclr.exe. The only way to get information on the runtime is by executing nanoclr.exe and reading the standard output. The alternative (producing a .csv file) is actually worse, because then the code that is running nanoclr.exe must find a directory to store the file, rather than reading and parsing stdout.

  • for the virtual device: as a dll (WIN_DLL_nanoCLR), published to cloudsmith. If the dll is downloaded, you cannot use a name like WIN_DLL_nanoCLR-1.12.0.74.dll so you must store it as, e.g., "WIN_DLL_nanoCLR-1.12.0.74/nanoFramework.nanoCLR.dll", otherwise you cannot use the DLL in nanoclr.exe. And if you want to know the native assemblies, you have to run "nanoclr.exe instance --clrinstancepath WIN_DLL_nanoCLR-1.12.0.74 --getnativeassemblies".

The last version is hidden in an auto-update scenario, as it is only used by "nanoclr instance --update". But if you want to freeze the framework version, I think you need to make a backup of that as well.

On the upside: all this information is required for tooling that runs at build-time. Once that tooling is part of the framework, I don't think that there are (m)any users that have a use case for retrieving the native assemblies. Then a new command line option that probably no human uses doesn't seem to be a bad solution.

Frank Robijn and others added 10 commits September 24, 2024 18:41
…s built. This is done by CMake that reads the data in the source files. The native_assemblies.csv file lists name, version and checksum for the included native assemblies.

- Extra options --getnativeassemblies and --clrinstancepath added to nanoclr.exe (and corresponding code to the native CLR host). This produces a list of name, version and checksum for the native assemblies included in the runtime embedded in nanoclr.exe or external file. Can still be used with "old" runtimes.

- .editorconfig added

It would be great if the latest version of each firmware package on cloudsmith would be re-created (with the same version number) so that the `native_assemblies.csv` files are part of the packages. I have no idea how to make that happen.

Signed-off-by: Frank Robijn <robijn@good-heavens.nl>
- Rename and adjust LocalInstance option to improve description.
- Add code to make sure DLL path is correct.
- Extract method to keep main method cleaner.
- Output is shown in a tidy 3 column table style to make it more visually attractive.
- Add exit code with warning about the fact that native assembly list can't be retrieved.
@josesimoes
Copy link
Member

@frobijn I've made some improvements on the code and pushed to the PR.

  • Refactor verb options to remove duplicate ones and improved the description to address your valid concern about clarity,
  • Add new exit code to warn about mutually exclusive options (can't use the same as missing DLL as that can lead to confusion).
  • Tidy up output of native assemblies list. Since we code for humans, not machines, better have it nicely presented 😉
    Here's the output:
image

Please review!

@josesimoes josesimoes changed the title Easy access to the metadata of the native assemblies in a firmware package / virtual device runtime Metadata of native assemblies is now generated and exposed in virtual device Sep 25, 2024
@josesimoes josesimoes added the Platform: Virtual Device & WIN32 Everything related specifically with WIN32 and .NET tool builds label Sep 25, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (10)
targets/netcore/nanoFramework.nanoCLR.CLI/CommonOptions.cs (3)

8-15: Consider making the property setter private.

The PathToCLRInstance property is well-defined with appropriate Option attribute settings. However, to enhance encapsulation, consider making the setter private:

-public string PathToCLRInstance { get; set; }
+public string PathToCLRInstance { get; private set; }

This change would prevent external code from modifying the property after it's set by the command-line parser, ensuring better control over the property's state.


17-31: Consider using an enum for verbosity levels.

The Verbosity property is well-defined with appropriate Option attribute settings and helpful comments. To improve type safety and validation, consider using an enum for verbosity levels:

public enum VerbosityLevel
{
    Quiet,
    Minimal,
    Normal,
    Detailed,
    Diagnostic
}

[Option(
    'v',
    "verbosity",
    Required = false,
    Default = VerbosityLevel.Normal,
    HelpText = "Sets the verbosity level of the command. Allowed values are Quiet, Minimal, Normal, Detailed, and Diagnostic. Not supported in every command; see specific command page to determine if this option is available.")]
public VerbosityLevel Verbosity { get; private set; }

This change would provide stronger typing and built-in validation for the verbosity levels. You may need to implement a custom type converter for the CommandLine parser to handle the enum values.


8-33: Consider making the class sealed.

The CommonOptions class is well-structured and focused on its purpose of encapsulating common CLI options. To prevent unintended inheritance and potentially improve performance, consider making the class sealed:

-public class CommonOptions
+public sealed class CommonOptions

This change ensures that the class cannot be inherited, which aligns with its purpose as a container for CLI options and may allow for compiler optimizations.

Overall, the implementation is clean, consistent, and follows good practices for command-line option handling.

targets/netcore/nanoFramework.nanoCLR.CLI/ExecuteCommandLineOptions.cs (1)

Line range hint 17-115: Approve remaining options with a minor suggestion.

The command-line options are comprehensive and well-documented, covering various aspects of nanoCLR functionality. They align well with the PR objectives of enhancing the CLI capabilities.

For consistency, consider adding a short name (single letter) option for the waitfordebugger, loopafterexit, forcegc, and compactionaftergc options. This would make them consistent with other options that have both short and long forms.

Example:

 [Option(
-    "waitfordebugger",
+    'w',
+    "waitfordebugger",
     Required = false,
     Default = false,
     HelpText = "Option to wait for a debugger connection after nanoCLR is started.")]
 public bool WaitForDebugger { get; set; }
targets/netcore/nanoFramework.nanoCLR.CLI/ExecuteCommandProcessor.cs (2)

28-28: Approve the change with a minor suggestion.

The change from options.LocalInstance to options.PathToCLRInstance is an improvement, as it more accurately describes the purpose of the variable. This aligns well with the new --clrinstancepath option mentioned in the PR objectives.

Consider using camelCase for the variable name to follow C# naming conventions:

-if (options.PathToCLRInstance != null)
+if (options.pathToClrInstance != null)

28-36: Summary of changes and areas needing attention

The modifications in this file implement the new --clrinstancepath option mentioned in the PR objectives. The changes look good overall, but there are a few points that require clarification:

  1. The shift from treating the path as a file to a directory (changing from File.Exists to Directory.Exists).
  2. The implications of passing only the directory name to nanoCLRHost.CreateBuilder.
  3. Potential backwards compatibility concerns with these changes.

Please address the questions raised in the previous comments to ensure that these changes are fully understood and don't introduce any unintended side effects.

Consider adding comments in the code to explain the rationale behind these changes, especially the switch to using a directory path instead of a file path. This will help future maintainers understand the design decisions made here.

targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp (1)

185-225: Overall assessment: Valuable addition with room for improvement

The introduced functions, nanoCLR_GetNativeAssemblyCount and nanoCLR_GetNativeAssemblyInformation, align well with the PR objectives of providing easy access to metadata of native assemblies. These additions contribute significantly to the "controlled update" strategy mentioned in the PR description.

While nanoCLR_GetNativeAssemblyCount is straightforward and well-implemented, nanoCLR_GetNativeAssemblyInformation could benefit from the suggested improvements to enhance its robustness, efficiency, and safety.

These changes lay a solid foundation for tooling that operates at build-time, as mentioned in the PR comments. However, consider the concerns raised about the efficiency of this approach compared to reading from standard output, especially for virtual device scenarios. It might be worth exploring alternative methods or optimizations to address these concerns in future iterations.

targets/netcore/nanoFramework.nanoCLR.CLI/ClrInstanceOperationsProcessor.cs (3)

25-47: LGTM! Consider enhancing error messages.

The new conditional structure for handling local CLR instances is well-implemented. It properly checks for conflicting options and directory existence before creating the nanoCLRHostBuilder.

Consider enhancing the error messages in the CLIException throws:

- throw new CLIException(ExitCode.E9010);
+ throw new CLIException(ExitCode.E9010, "Cannot combine PathToCLRInstance and UpdateCLR options.");

- throw new CLIException(ExitCode.E9009);
+ throw new CLIException(ExitCode.E9009, $"Directory not found: {options.PathToCLRInstance}");

This would provide more context to the user about the specific error encountered.


57-86: LGTM! Consider extracting method for better readability.

The new conditional structure for handling CLR version and native assemblies output is well-implemented. It properly handles different verbosity levels and formats the output clearly.

Consider extracting the logic for outputting CLR version and native assemblies into a separate method for better readability:

private static void OutputClrAndAssemblyInfo(ClrInstanceOperationsOptions options, nanoCLRHostBuilder hostBuilder)
{
    if (Program.VerbosityLevel > VerbosityLevel.Normal)
    {
        hostBuilder.OutputNanoClrDllInfo();
    }

    if (options.GetCLRVersion)
    {
        Console.WriteLine($"nanoCLR version: {hostBuilder.GetCLRVersion()}");
    }

    if (options.GetNativeAssemblies)
    {
        OutputNativeAssembliesInfo(hostBuilder, options.GetCLRVersion);
    }
}

private static void OutputNativeAssembliesInfo(nanoCLRHostBuilder hostBuilder, bool clrVersionOutputted)
{
    List<NativeAssemblyDetails>? nativeAssemblies = hostBuilder.GetNativeAssemblies();

    if (nativeAssemblies is not null)
    {
        if (clrVersionOutputted)
        {
            Console.WriteLine();
        }

        OutputNativeAssembliesList(nativeAssemblies);
    }
    else if (Program.VerbosityLevel > VerbosityLevel.Normal)
    {
        Console.WriteLine("Unable to retrieve native assemblies information.");
    }
}

Then, you can replace the existing code with:

else if (options.GetCLRVersion || options.GetNativeAssemblies)
{
    OutputClrAndAssemblyInfo(options, hostBuilder);
    return (int)ExitCode.OK;
}

This refactoring improves code readability and maintainability by separating concerns.


94-106: LGTM! Consider minor performance optimization.

The OutputNativeAssembliesList method is well-implemented, providing a neatly formatted output of native assembly details. The use of LINQ for calculating padding is efficient and results in a clean, readable output.

For a minor performance optimization, consider calculating the maximum lengths in a single pass:

private static void OutputNativeAssembliesList(List<NativeAssemblyDetails> nativeAssemblies)
{
    Console.WriteLine("Native assemblies:");

    int maxAssemblyNameLength = 0;
    int maxAssemblyVersionLength = 0;

    foreach (var assembly in nativeAssemblies)
    {
        maxAssemblyNameLength = Math.Max(maxAssemblyNameLength, assembly.Name.Length);
        maxAssemblyVersionLength = Math.Max(maxAssemblyVersionLength, assembly.Version.ToString().Length);
    }

    foreach (NativeAssemblyDetails assembly in nativeAssemblies)
    {
        Console.WriteLine($"  {assembly.Name.PadRight(maxAssemblyNameLength)} v{assembly.Version.ToString().PadRight(maxAssemblyVersionLength)} 0x{assembly.CheckSum:X8}");
    }
}

This approach avoids multiple iterations over the list and might be more efficient for larger lists of assemblies.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 2284649 and 132e84d.

📒 Files selected for processing (14)
  • .editorconfig (1 hunks)
  • CMake/Modules/FindNF_NativeAssemblies.cmake (6 hunks)
  • spelling_exclusion.dic (1 hunks)
  • targets/netcore/nanoFramework.nanoCLR.CLI/ClrInstanceOperationsOptions.cs (2 hunks)
  • targets/netcore/nanoFramework.nanoCLR.CLI/ClrInstanceOperationsProcessor.cs (3 hunks)
  • targets/netcore/nanoFramework.nanoCLR.CLI/CommonOptions.cs (1 hunks)
  • targets/netcore/nanoFramework.nanoCLR.CLI/ExecuteCommandLineOptions.cs (1 hunks)
  • targets/netcore/nanoFramework.nanoCLR.CLI/ExecuteCommandProcessor.cs (2 hunks)
  • targets/netcore/nanoFramework.nanoCLR.CLI/ExitCode.cs (1 hunks)
  • targets/netcore/nanoFramework.nanoCLR.Host/Interop/nanoCLR.cs (3 hunks)
  • targets/netcore/nanoFramework.nanoCLR.Host/NanoClrHostBuilder.cs (2 hunks)
  • targets/netcore/nanoFramework.nanoCLR.Host/NativeAssemblyDetails.cs (1 hunks)
  • targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp (2 hunks)
  • targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.h (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
  • .editorconfig
  • CMake/Modules/FindNF_NativeAssemblies.cmake
  • spelling_exclusion.dic
  • targets/netcore/nanoFramework.nanoCLR.CLI/ClrInstanceOperationsOptions.cs
  • targets/netcore/nanoFramework.nanoCLR.Host/Interop/nanoCLR.cs
  • targets/netcore/nanoFramework.nanoCLR.Host/NanoClrHostBuilder.cs
  • targets/netcore/nanoFramework.nanoCLR.Host/NativeAssemblyDetails.cs
  • targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.h
🔇 Additional comments not posted (10)
targets/netcore/nanoFramework.nanoCLR.CLI/CommonOptions.cs (1)

1-7: LGTM: Proper license and namespace declaration.

The file includes the appropriate license information and correctly declares the namespace. The import statement for the CommandLine namespace is also present, which is necessary for the Option attributes used in the class.

targets/netcore/nanoFramework.nanoCLR.CLI/ExecuteCommandLineOptions.cs (1)

Line range hint 1-115: Overall assessment: Positive changes that enhance CLI functionality.

The modifications to ExecuteCommandLineOptions.cs align well with the PR objectives. The inheritance change improves code organization, and the remaining options provide comprehensive control over the nanoCLR CLI. These changes contribute to the goal of facilitating a "controlled update" strategy for the nanoFramework.

targets/netcore/nanoFramework.nanoCLR.CLI/ExitCode.cs (3)

109-114: LGTM: New enum value E9010 is well-defined and consistent.

The new enum value E9010 is correctly implemented with a clear summary comment and display name. It aligns with the PR objectives by addressing the scenario where a CLR instance update is attempted when a specific version is requested.


116-120: LGTM: New enum value E9011 is well-defined and consistent.

The new enum value E9011 is correctly implemented with a clear summary comment and display name. It aligns with the PR objectives by addressing the scenario where the list of native assemblies cannot be retrieved.


109-120: Overall changes to ExitCode enum are consistent and well-implemented.

The two new enum values, E9010 and E9011, have been added correctly to the ExitCode enum. They follow the existing naming conventions and numbering pattern. The additions are consistent with the PR objectives and provide clear error codes for the new features related to CLR instance updates and native assembly list retrieval.

targets/netcore/nanoFramework.nanoCLR.CLI/ExecuteCommandProcessor.cs (2)

31-31: Clarify the change from file to directory check.

The change from File.Exists to Directory.Exists suggests that PathToCLRInstance now represents a directory path rather than a file path. This is a significant change that may affect how the CLR instance is loaded.

Could you please clarify:

  1. Why was this change made?
  2. How does this affect the loading of the CLR instance?
  3. Are there any backwards compatibility concerns?

To verify the impact of this change, please run the following script:

✅ Verification successful

Change from File.Exists to Directory.Exists Verified

The update to use Directory.Exists for PathToCLRInstance is consistent across the codebase. No instances of File.Exists related to PathToCLRInstance were found, ensuring that it is correctly treated as a directory path.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other occurrences of File.Exists or Directory.Exists related to CLR instance loading

# Test: Search for File.Exists and Directory.Exists usage
echo "Searching for File.Exists usage:"
rg --type csharp "File\.Exists.*Instance" -A 5
echo "Searching for Directory.Exists usage:"
rg --type csharp "Directory\.Exists.*Instance" -A 5

Length of output: 1663


36-36: Approve the change with a request for additional information.

The modification to use Path.GetDirectoryName(options.PathToCLRInstance) is consistent with the earlier changes and ensures that only the directory path is passed to nanoCLRHost.CreateBuilder.

To ensure this change doesn't introduce any unintended side effects:

  1. Could you explain how nanoCLRHost.CreateBuilder uses this directory path?
  2. Are there any scenarios where passing only the directory name might cause issues?

Please run the following script to verify the usage of CreateBuilder:

targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp (1)

186-189: LGTM: Simple and straightforward implementation.

The function nanoCLR_GetNativeAssemblyCount correctly returns the count of native assemblies using the global variable g_CLR_InteropAssembliesCount. The implementation is concise and appropriate for this task.

targets/netcore/nanoFramework.nanoCLR.CLI/ClrInstanceOperationsProcessor.cs (2)

7-7: LGTM! Necessary addition for LINQ operations.

The addition of using System.Linq; is appropriate and necessary for the LINQ operations used in the OutputNativeAssembliesList method.


11-12: LGTM! Verify Newtonsoft.Json usage.

The addition of using nanoFramework.nanoCLR.Host; and using Newtonsoft.Json; statements is appropriate for the new functionality in this file.

Let's verify the usage of Newtonsoft.Json in this file:

This will help ensure that Newtonsoft.Json is being used as expected in the file.

✅ Verification successful

Verified Newtonsoft.Json usage.

The usage of JsonConvert.DeserializeObject<List<CloudsmithPackageInfo>>(responseBody); in ClrInstanceOperationsProcessor.cs correctly implements JSON deserialization as intended.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify Newtonsoft.Json usage in ClrInstanceOperationsProcessor.cs

# Test: Search for Newtonsoft.Json usage
rg --type csharp 'JsonConvert' targets/netcore/nanoFramework.nanoCLR.CLI/ClrInstanceOperationsProcessor.cs

Length of output: 214

- Required to offer a smooth migration path to developers and tools currently using this option.
- Marked as Obsolete.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
targets/netcore/nanoFramework.nanoCLR.CLI/CommonOptions.cs (2)

18-31: LGTM: Well-handled deprecation of LocalInstance

The LocalInstance property is correctly marked as obsolete and hidden, maintaining backward compatibility while encouraging the use of the new PathToCLRInstance property. The setter logic ensures that values are still captured correctly.

Consider updating the obsolete message to include the new option name:

-[Obsolete("This option is deprecated and will be removed in a future version. Use --clrpath instead.")]
+[Obsolete("This option is deprecated and will be removed in a future version. Use --clrpath instead of --localinstance.")]

34-48: LGTM: Well-implemented Verbosity property

The Verbosity property is well-implemented with clear documentation, appropriate CommandLine attributes, and comprehensive help text. The use of both short and long option names enhances usability.

For consistency with the other properties, consider moving the XML documentation to the property itself:

-        /// <summary>
-        /// Allowed values:
-        /// q[uiet]
-        /// m[inimal]
-        /// n[ormal]
-        /// d[etailed]
-        /// diag[nostic]
-        /// </summary>
         [Option(
             'v',
             "verbosity",
             Required = false,
             Default = "n",
             HelpText = "Sets the verbosity level of the command. Allowed values are q[uiet], m[inimal], n[ormal], d[etailed], and diag[nostic]. Not supported in every command; see specific command page to determine if this option is available.")]
+        /// <summary>
+        /// Sets the verbosity level of the command.
+        /// Allowed values:
+        /// q[uiet], m[inimal], n[ormal], d[etailed], diag[nostic]
+        /// </summary>
         public string Verbosity { get; set; }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 132e84d and 0c5844e.

📒 Files selected for processing (1)
  • targets/netcore/nanoFramework.nanoCLR.CLI/CommonOptions.cs (1 hunks)
🔇 Additional comments not posted (2)
targets/netcore/nanoFramework.nanoCLR.CLI/CommonOptions.cs (2)

11-16: LGTM: Well-defined property for CLR path

The PathToCLRInstance property is well-implemented with clear naming and appropriate CommandLine attributes. The help text effectively explains its purpose.


1-50: LGTM: Well-structured CommonOptions class

The CommonOptions class is well-implemented, providing a clean and effective way to handle common command-line options for the nanoFramework CLI. It makes good use of the CommandLine library for robust option parsing and effectively manages the deprecation of an old option while introducing a new one.

The class structure and property implementations follow good practices, enhancing usability and maintainability of the CLI interface.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 0c5844e and 63e9e5b.

📒 Files selected for processing (3)
  • targets/netcore/nanoFramework.nanoCLR.Host/NativeAssemblyDetails.cs (1 hunks)
  • targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp (2 hunks)
  • targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.h (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • targets/netcore/nanoFramework.nanoCLR.Host/NativeAssemblyDetails.cs
  • targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.h
🧰 Additional context used
📓 Learnings (1)
targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp (1)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-09-25T11:28:38.536Z
Learning: When working with `nanoCLR_GetNativeAssemblyInformation`, fixed-size assembly names are required, so code that deals with variable-length names cannot be used.
🔇 Additional comments not posted (1)
targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp (1)

186-189: LGTM: Simple and correct implementation.

The nanoCLR_GetNativeAssemblyCount function is implemented correctly. It returns the value of g_CLR_InteropAssembliesCount as a uint16_t, which should be sufficient for the number of native assemblies.

frobijn and others added 4 commits September 26, 2024 12:52
Co-authored-by: Laurent Ellerbach <laurelle@microsoft.com>
Automated fixes for code style.
@frobijn
Copy link
Collaborator Author

frobijn commented Sep 26, 2024

Nice presentation! I've added a sort by name to make it even more readable.

Checked the replacement of the LocalInstance by PathToClrInstance option (few changes) and tested it. Works well.

…0ad-0310-4956-bd59-f669b6f7d3a9

Code style fixes for nanoframework/nf-interpreter PR#3023
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp (1)

191-231: LGTM: Well-implemented function with proper checks.

The nanoCLR_GetNativeAssemblyInformation function is well-implemented with appropriate null checks and buffer size validation. It efficiently copies native assembly information into the provided buffer.

A minor suggestion for improvement:

Consider using a constant for the fixed assembly name size (128 bytes) to improve maintainability. For example:

const size_t ASSEMBLY_NAME_SIZE = 128;

// Update the requiredSize calculation
const size_t requiredSize = g_CLR_InteropAssembliesCount * (5 * sizeof(CLR_UINT16) + ASSEMBLY_NAME_SIZE);

// ... and later in the code ...
hal_strcpy_s((char *)data, ASSEMBLY_NAME_SIZE, g_CLR_InteropAssembliesNativeData[i]->m_szAssemblyName);
data += ASSEMBLY_NAME_SIZE;

This change would make it easier to adjust the size in the future if needed.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 467b9c2 and a09eec1.

📒 Files selected for processing (1)
  • targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp (2 hunks)
🔇 Additional comments (1)
targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp (1)

186-189: LGTM: Simple and effective implementation.

The nanoCLR_GetNativeAssemblyCount function is well-implemented. It directly returns the g_CLR_InteropAssembliesCount, which is appropriate for retrieving the count of native assemblies. The use of uint16_t as the return type is suitable, assuming the number of native assemblies will never exceed 65535.

Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

LGTM! Nice improvement. 👍🏻

@josesimoes josesimoes merged commit 25db559 into nanoframework:main Sep 26, 2024
29 checks passed
@nfbot
Copy link
Member

nfbot commented Sep 26, 2024

@frobijn thank you again for your contribution! 🙏😄

.NET nanoFramework is all about community involvement, and no contribution is too small.
We would like to invite you to join the project's Contributors list.

Please edit it and add an entry with your GitHub username in the appropriate location (names are sorted alphabetically):

  <tr>
    <td><img src="https://github.com/frobijn.png?size=50" height="50" width="50" ></td>
    <td><a href="https://github.com/frobijn">Frank Robijn</a></td>
  </tr>

(Feel free to adjust your name if it's not correct)

@josesimoes
Copy link
Member

@frobijn to answer your question above: there is no easy way to recreate the published fw pacakges. That would require a lot of manual work. From now on that will be there.

@frobijn frobijn deleted the Versioning branch September 26, 2024 19:58
@coderabbitai coderabbitai bot mentioned this pull request Oct 10, 2024
13 tasks
@coderabbitai coderabbitai bot mentioned this pull request Apr 10, 2025
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Config-and-Build Platform: Virtual Device & WIN32 Everything related specifically with WIN32 and .NET tool builds Type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants