-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
add M5 CoreInk project #304
Conversation
Draft mode as missing for:
|
SonarCloud Quality Gate failed.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start, few comments to help pointing out what also need to be adjusted to create the nuget.
///////////////////////////////////////////////////////////////// | ||
// This attribute is mandatory when building Interop libraries // | ||
// update this whenever the native assembly signature changes // | ||
[assembly: AssemblyNativeVersion("0.0.0.0")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need a specific native version I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guilty of copy/paste... 😅
I will cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still need to clean up this part ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that your goal to create this specific README is to use it for the nuget. In that case, looks good. Otherwise, add the key elements into the main readme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think about including it in the nuget. It was more to display a current state of code , and not render the main one too full of data (we can create a README per core also with the same kind of information)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you also need a lock package
And you'll also need a specific nuspec in the root folder (see the other ones as example)
And most likely adjust the azure pipeline to add the build and package for this one!
@xas any update on this? |
@networkfusion Yeah sorry, I was waiting for the merge of the E-Ink driver and then lot of work happened 😞 I will try to update the PR this week |
WalkthroughThe pull request introduces a new template step for packaging the NuGet package Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this 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
Files ignored due to path filters (9)
README.md
is excluded by!**/*.md
and included by nonenanoFramework.CoreInk.nuspec
is excluded by none and included by nonenanoFramework.CoreInk/M5CoreInk.cs
is excluded by none and included by nonenanoFramework.CoreInk/Properties/AssemblyInfo.cs
is excluded by none and included by nonenanoFramework.CoreInk/README.md
is excluded by!**/*.md
and included by nonenanoFramework.CoreInk/key.snk
is excluded by none and included by nonenanoFramework.CoreInk/nanoFramework.CoreInk.nfproj
is excluded by none and included by nonenanoFramework.CoreInk/packages.config
is excluded by none and included by nonenanoFramework.M5Stack.sln
is excluded by none and included by none
Files selected for processing (2)
- azure-pipelines.yml (1 hunks)
- nanoFramework.CoreInk/packages.lock.json (1 hunks)
Additional context used
yamllint
azure-pipelines.yml
[error] 96-96: trailing spaces
(trailing-spaces)
Additional comments not posted (2)
azure-pipelines.yml (1)
93-95
: LGTM!The new template invocation for packaging the
nanoFramework.CoreInk
NuGet package is consistent with the existing template invocations and expands the pipeline's capabilities without altering any existing functionality.nanoFramework.CoreInk/packages.lock.json (1)
1-121
: LGTM!The addition of the
packages.lock.json
file is a good practice to ensure consistent and reproducible builds across different environments. The file follows the standard structure and includes the necessary dependencies for the CoreInk project targeting the .NET nanoFramework platform.
azure-pipelines.yml
Outdated
- template: azure-pipelines-templates/class-lib-package.yml@templates | ||
parameters: | ||
nugetPackageName: 'nanoFramework.CoreInk' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove trailing spaces.
The static analysis tool has flagged a trailing spaces issue on this line. Please remove the trailing spaces to maintain consistent formatting and adhere to best practices.
Apply this diff to fix the trailing spaces issue:
-
+
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Tools
yamllint
[error] 96-96: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks a lot for moving it forward. Just a little thing and you'll be good to go!
///////////////////////////////////////////////////////////////// | ||
// This attribute is mandatory when building Interop libraries // | ||
// update this whenever the native assembly signature changes // | ||
[assembly: AssemblyNativeVersion("0.0.0.0")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still need to clean up this part ;-)
@xas hey hey! 😃 |
* add nuget properties * add key.snk file * add packages.lock * add azure pipelines line
There was a problem hiding this 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
🧹 Outside diff range and nitpick comments (3)
nanoFramework.CoreInk/packages.lock.json (3)
5-22
: Core dependencies are well-defined and pinned to specific versions.The core nanoFramework libraries (CoreLibrary, Graphics.Core, Hardware.Esp32) are correctly included with pinned version ranges. This ensures reproducibility and stability of the project.
Consider adding a comment in the file explaining the rationale behind pinning to specific versions, which can be helpful for future maintainers.
23-51
: IoT device dependencies are appropriate for the CoreInk project.The inclusion of IoT device libraries (Button, Buzzer, ePaper, Rtc) is consistent with the project's objectives for the CoreInk product. Version pinning is maintained, ensuring consistency with core dependencies.
Consider adding a brief comment or documentation explaining the purpose of each IoT device library in the context of the CoreInk project. This can help new contributors understand the project's hardware requirements and capabilities.
52-127
: Remaining dependencies and build tools are well-chosen and appropriate.The inclusion of system device libraries, math support, threading, and UnitsNet libraries for Frequency and Temperature is appropriate for an IoT project. The use of Nerdbank.GitVersioning for version management is a good practice.
Consider grouping related dependencies together (e.g., all System.Device.* libraries) for better readability. Also, you might want to add a comment explaining the purpose of the UnitsNet libraries in the context of the CoreInk project.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (9)
README.md
is excluded by!**/*.md
and included by nonenanoFramework.CoreInk.nuspec
is excluded by none and included by nonenanoFramework.CoreInk/M5CoreInk.cs
is excluded by none and included by nonenanoFramework.CoreInk/Properties/AssemblyInfo.cs
is excluded by none and included by nonenanoFramework.CoreInk/README.md
is excluded by!**/*.md
and included by nonenanoFramework.CoreInk/key.snk
is excluded by none and included by nonenanoFramework.CoreInk/nanoFramework.CoreInk.nfproj
is excluded by none and included by nonenanoFramework.CoreInk/packages.config
is excluded by none and included by nonenanoFramework.M5Stack.sln
is excluded by none and included by none
📒 Files selected for processing (2)
- azure-pipelines.yml (1 hunks)
- nanoFramework.CoreInk/packages.lock.json (1 hunks)
🧰 Additional context used
🪛 yamllint
azure-pipelines.yml
[error] 96-96: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
azure-pipelines.yml (2)
93-95
: LGTM: New package step added correctly.The addition of the new template step for packaging 'nanoFramework.CoreInk' is correct and consistent with the existing structure of the pipeline. It's appropriately placed before the publishing step.
96-96
:⚠️ Potential issueRemove trailing spaces.
There are trailing spaces on this line. Please remove them to maintain consistent formatting and adhere to best practices.
Apply this diff to fix the trailing spaces issue:
- +Likely invalid or redundant comment.
🧰 Tools
🪛 yamllint
[error] 96-96: trailing spaces
(trailing-spaces)
nanoFramework.CoreInk/packages.lock.json (1)
1-4
: LGTM: File structure and version are appropriate.The file structure and version number (1) are correct for a new package lock file in a .NET nanoFramework project. This follows best practices for dependency management.
}, | ||
"nanoFramework.Iot.Device.Buzzer": { | ||
"type": "Direct", | ||
"requested": "[1.2.656, 1.2.656]", | ||
"resolved": "1.2.656", | ||
"contentHash": "iLNlBzHiPgr0hhss+/pMAGWIxK8mM+pLOwaki0iXE0CRIakloTDySfpwjJlV0FAf9+dlkhxzm3oiT5+ZAWoa3g==" | ||
}, | ||
"nanoFramework.Iot.Device.Common.NumberHelper": { | ||
"type": "Direct", | ||
"requested": "[1.2.628, 1.2.628]", | ||
"resolved": "1.2.628", | ||
"contentHash": "n1Na6YBFFqNv32AU+rfmAYFObeqEtf5q5uTxjCT1x+ThWDbi8eespjp/4OvXoU7C+HldXz8/uS4UZ8lswXfOfg==" | ||
}, | ||
"nanoFramework.Iot.Device.ePaper": { | ||
"type": "Direct", | ||
"requested": "[1.0.487, 1.0.487]", | ||
"resolved": "1.0.487", | ||
"contentHash": "x20Pt6HFJFk8j8czvz2BilY2E5GE6Oa5h6ZFGNvCVPICnDFA8e0urneiG0LBZ3VuKUyXECAxbThGj2RS6tuVLQ==" | ||
}, | ||
"nanoFramework.Iot.Device.Rtc": { | ||
"type": "Direct", | ||
"requested": "[1.2.656, 1.2.656]", | ||
"resolved": "1.2.656", | ||
"contentHash": "W/sCNUrO1GBrrisMVfIiXQwdlLBeTdbAilyxwxNprVVXsbPHR0qw6oyo/pvJNh4LxIlC3MuPfoeky9Ac5miztw==" | ||
}, | ||
"nanoFramework.Runtime.Events": { | ||
"type": "Direct", | ||
"requested": "[1.11.18, 1.11.18]", | ||
"resolved": "1.11.18", | ||
"contentHash": "t0XpUkdyBBBv/0S4oGx3yUJG1iPYWc38odvZW8mVoioSxZOJrRkRHpNfwYxTxtP4LIEyyesOPEH42d05FHfHzA==" | ||
}, | ||
"nanoFramework.System.Device.Adc": { | ||
"type": "Direct", | ||
"requested": "[1.1.11, 1.1.11]", | ||
"resolved": "1.1.11", | ||
"contentHash": "BlFm2MR2Os6ab++EnW6RiZ4860F1ge48mSBbqsqVXnP5xvkfE03Hop4TyqTHWQbLFKcubCc5CMkH9xxuABR6UQ==" | ||
}, | ||
"nanoFramework.System.Device.Gpio": { | ||
"type": "Direct", | ||
"requested": "[1.1.41, 1.1.41]", | ||
"resolved": "1.1.41", | ||
"contentHash": "5QnpdfvjxOvka2S5IHSdKudWmkH+CDQ3TFFuXOGuNlgZJFsAx0/k5zuwgJYkxIyGbL8kdcjBWLyDNdihjA1pUg==" | ||
}, | ||
"nanoFramework.System.Device.I2c": { | ||
"type": "Direct", | ||
"requested": "[1.1.16, 1.1.16]", | ||
"resolved": "1.1.16", | ||
"contentHash": "33YPnlZVjFwx0mOhdKTOggx+TvbjTD3WZ6rkSB33ytF9fHoqfvS1AYr4+ScUHW4z2yE7vxVWBol5mI7iEg4Aiw==" | ||
}, | ||
"nanoFramework.System.Device.Model": { | ||
"type": "Direct", | ||
"requested": "[1.2.628, 1.2.628]", | ||
"resolved": "1.2.628", | ||
"contentHash": "39q4OScnVtMHlGdz9ZleiwllJtE+G6OZE0Xd/qn3gEvrR4XmLCwYZ+JdVxtDU7bK5JLrJA984NKeq+l5ZW0X4A==" | ||
}, | ||
"nanoFramework.System.Device.Pwm": { | ||
"type": "Direct", | ||
"requested": "[1.1.10, 1.1.10]", | ||
"resolved": "1.1.10", | ||
"contentHash": "vk/Dr8No2ec+eBwwufxDK0Rm6BRnOoevjaqEXbvpFz2BK3UPiC4OhUWH3Rwel34rywg1mXozAAY9ZwvN2gnxlA==" | ||
}, | ||
"nanoFramework.System.Device.Spi": { | ||
"type": "Direct", | ||
"requested": "[1.3.52, 1.3.52]", | ||
"resolved": "1.3.52", | ||
"contentHash": "chtkrJp424LMitA6Fw/QzzhIrYL9PdEaln+A7o5QR99VijDoOILdMvgeeVBnIpkicUH7aY9Vj+3F2TlIGQH/+g==" | ||
}, | ||
"nanoFramework.System.Math": { | ||
"type": "Direct", | ||
"requested": "[1.5.43, 1.5.43]", | ||
"resolved": "1.5.43", | ||
"contentHash": "JEOEGHoIpknJFwPjjz77sT5mej2PiT7JTv59jabzFf+d8XYy8Z4SH+NdX00Xc/yDS8LIPuWb7+C245XGUUx99A==" | ||
}, | ||
"nanoFramework.System.Threading": { | ||
"type": "Direct", | ||
"requested": "[1.1.32, 1.1.32]", | ||
"resolved": "1.1.32", | ||
"contentHash": "6o7Y4gH15FLuo2FWGLecABiCD57V5QMf5g/hEneV64VmhoXI8Bk7r6BDBPTfAePs738xbc1ECpA5dJmbSmtilg==" | ||
}, | ||
"Nerdbank.GitVersioning": { | ||
"type": "Direct", | ||
"requested": "[3.6.143, 3.6.143]", | ||
"resolved": "3.6.143", | ||
"contentHash": "N24MtdLq4PmdJ2woTQd9515q0I1jeO/DlimYCB/GoRd510Fc9dk9H6YaN8MPPrVF8RHrIalCUfWbWvcoarYeoQ==" | ||
}, | ||
"UnitsNet.nanoFramework.Frequency": { | ||
"type": "Direct", | ||
"requested": "[5.59.0, 5.59.0]", | ||
"resolved": "5.59.0", | ||
"contentHash": "zqlqhLHCUL8B8i5N2s//k93cAJ4RLcQi46EaBBlqGJFHVvOequz+zO8HfnUHYQFBELNeLxkJNZWx/qczRdCAng==" | ||
}, | ||
"UnitsNet.nanoFramework.Temperature": { | ||
"type": "Direct", | ||
"requested": "[5.59.0, 5.59.0]", | ||
"resolved": "5.59.0", | ||
"contentHash": "LDXMzn1y0OSSvXA+A+4iPI+P9co5QmlP/4AQdqxlNOvIZHVAN7j98HPUVSXzK8OfeUpF8lhwn41LqHDeeFNZtQ==" | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Discrepancies Found in Dependency References
The packages.lock.json
file lists several dependencies; however, no .csproj
files were found in the project. This indicates that the dependencies are not referenced in any project files, which may lead to build or runtime issues.
🔗 Analysis chain
Overall, the packages.lock.json file is well-structured and comprehensive.
The file includes all necessary dependencies for the CoreInk project, with consistent version pinning across all packages. This approach ensures reproducibility and stability. The included libraries align well with the project objectives mentioned in the PR description.
To ensure all dependencies are correctly referenced in the project, please run the following verification script:
This script will help ensure that all dependencies listed in the packages.lock.json file are correctly referenced in the project's .csproj files, maintaining consistency across the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all dependencies in packages.lock.json are referenced in .csproj files
# Find all .csproj files in the project
csproj_files=$(fd -e csproj)
# Extract package references from .csproj files
package_refs=$(cat $csproj_files | rg -o 'PackageReference Include="([^"]+)"' -r '$1' | sort -u)
# Extract dependencies from packages.lock.json
lock_deps=$(cat nanoFramework.CoreInk/packages.lock.json | jq -r '.dependencies[".NETnanoFramework,Version=v1.0"] | keys[]' | sort)
# Compare package references with lock file dependencies
diff <(echo "$package_refs") <(echo "$lock_deps")
# Check if there are any differences
if [ $? -eq 0 ]; then
echo "All dependencies in packages.lock.json are correctly referenced in .csproj files."
else
echo "Discrepancies found between packages.lock.json and .csproj files. Please review the differences above."
fi
Length of output: 1878
yeah, sorry, I wish I had more time. Plus updating packages/nuspec require some attention 😭 |
@xas glad to see that you're able to finish this. 👍🏻 |
Note: we can ignore the line duplication. It's mainly "normal" regarding how we've been setting up the projects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minimal comments and suggestions I'll merge to speed up the process :-)
|
Description
Motivation and Context
How Has This Been Tested?
see README file for implemented properties and testing result
Types of changes
Checklist:
Summary by CodeRabbit
New Features
nanoFramework.CoreInk
package in the pipeline.packages.lock.json
file fornanoFramework.CoreInk
, detailing essential project dependencies, including various nanoFramework libraries.Bug Fixes