Transition to multi-package repository #126
Conversation
Signed-off-by: Frank Robijn <[email protected]>
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe pull request updates several configuration files and introduces new dependency lock files. The changelog configuration now lists two NuGet package links instead of one. GitHub Actions workflows are modified by adding a solution parameter and updating formatting. The Azure Pipelines configuration is restructured with new jobs for building libraries, updating dependents, and reporting build failures based on version tag conditions. Additionally, new Changes
Sequence Diagram(s)sequenceDiagram
participant VCS as Version Control
participant BP as Build Pipeline
participant BL as Build_Library Job
participant UD as Update_Dependents Job
participant RB as Report_Build_Failure Job
VCS->>BP: Trigger build (commit or tag push)
alt Branch without version tag
BP->>BL: Run Build_Library job
else Version tag detected
BP->>UD: Run Update_Dependents job
end
BP->>RB: Evaluate job outcomes
RB->>BP: Report build failure if any job fails
Possibly related PRs
Suggested labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
azure-pipelines.yml (6)
20-23: Tag Trigger Configuration & Trailing Whitespace
The newtagsblock (lines 20–23) correctly enables trigger builds from version tags; however, YAMLlint has flagged trailing spaces (e.g., on line 21). Please remove any trailing whitespace to ensure clean formatting.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 21-21: trailing spaces
(trailing-spaces)
43-51: Build_Library Job: Indentation Adjustment
TheBuild_Libraryjob definition (starting at line 43) is not indented as expected under thejobs:key. It should be indented (typically 2 spaces) for proper YAML hierarchy. Please adjust the indentation.
Proposed diff:-jobs: -############################## - - job: Build_Library +jobs: + ############################## + - job: Build_Library🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 43-43: wrong indentation: expected 2 but found 0
(indentation)
55-63: Variable Definition: Remove Trailing Spaces
In the Build_Library job variables block (lines 55–63), YAMLlint has flagged trailing spaces (e.g., on line 58). Please remove any extraneous whitespace to maintain consistency and avoid potential parsing issues.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 58-58: trailing spaces
(trailing-spaces)
65-72: Build Step Indentation in Build_Library Job
The comment and steps for building the library (lines 65–72) show an indentation issue (line 67 is reported as “expected 4 but found 2”). Please adjust the indentation within the steps block so that they are consistently indented relative to their parent job step.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 67-67: wrong indentation: expected 4 but found 2
(indentation)
74-86: Cleanup Trailing Spaces in Commented-Out Sections
Several commented-out steps for packaging additional libraries (lines 74, 78, 82, and 86) contain trailing spaces. Although these lines are commented, cleaning them up improves readability and consistency.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 74-74: trailing spaces
(trailing-spaces)
[error] 78-78: trailing spaces
(trailing-spaces)
[error] 82-82: trailing spaces
(trailing-spaces)
[error] 86-86: trailing spaces
(trailing-spaces)
145-170: Report_Build_Failure Job: Formatting Issues
Within theReport_Build_Failurejob block (lines 145–170), YAMLlint has flagged multiple formatting issues:
- Trailing spaces on lines 146, 160, and 165.
- Indentation issues on lines 148 (expected 4 spaces but found 2) and 161.
Please fix these formatting issues to ensure the YAML passes linting and maintains a consistent style.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 146-146: trailing spaces
(trailing-spaces)
[warning] 148-148: wrong indentation: expected 4 but found 2
(indentation)
[error] 160-160: trailing spaces
(trailing-spaces)
[warning] 161-161: wrong indentation: expected 4 but found 2
(indentation)
[error] 165-165: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (45)
.editorconfigis excluded by none and included by noneREADME.mdis excluded by!**/*.mdand included by noneStub-generation.mdis excluded by!**/*.mdand included by nonenanoFramework.Device.Can.Core.nuspecis excluded by none and included by nonenanoFramework.Device.Can.Core/CanController.csis excluded by none and included by nonenanoFramework.Device.Can.Core/CanControllerEventListener.csis excluded by none and included by nonenanoFramework.Device.Can.Core/CanControllerManager.csis excluded by none and included by nonenanoFramework.Device.Can.Core/CanEvent.csis excluded by none and included by nonenanoFramework.Device.Can.Core/CanMessage.csis excluded by none and included by nonenanoFramework.Device.Can.Core/CanMessageEvent.csis excluded by none and included by nonenanoFramework.Device.Can.Core/CanMessageFrameType.csis excluded by none and included by nonenanoFramework.Device.Can.Core/CanMessageIdType.csis excluded by none and included by nonenanoFramework.Device.Can.Core/CanMessageReceivedEventArgs.csis excluded by none and included by nonenanoFramework.Device.Can.Core/CanSettings.csis excluded by none and included by nonenanoFramework.Device.Can.Core/Properties/AssemblyInfo.csis excluded by none and included by nonenanoFramework.Device.Can.Core/key.snkis excluded by none and included by nonenanoFramework.Device.Can.Core/nanoFramework.Device.Can.Core.nfprojis excluded by none and included by nonenanoFramework.Device.Can.Core/packages.configis excluded by none and included by nonenanoFramework.Device.Can.Esp32.nuspecis excluded by none and included by nonenanoFramework.Device.Can.Esp32/Properties/AssemblyInfo.csis excluded by none and included by nonenanoFramework.Device.Can.Esp32/key.snkis excluded by none and included by nonenanoFramework.Device.Can.Esp32/nanoFramework.Device.Can.Esp32.nfprojis excluded by none and included by nonenanoFramework.Device.Can.Esp32/packages.configis excluded by none and included by nonenanoFramework.Device.Can.Mcp2515.nuspecis excluded by none and included by nonenanoFramework.Device.Can.Mcp2515/Properties/AssemblyInfo.csis excluded by none and included by nonenanoFramework.Device.Can.Mcp2515/key.snkis excluded by none and included by nonenanoFramework.Device.Can.Mcp2515/nanoFramework.Device.Can.Mcp2515.nfprojis excluded by none and included by nonenanoFramework.Device.Can.Mcp2515/packages.configis excluded by none and included by nonenanoFramework.Device.Can.Stm32.nuspecis excluded by none and included by nonenanoFramework.Device.Can.Stm32/CanController.csis excluded by none and included by nonenanoFramework.Device.Can.Stm32/CanControllerEventListener.csis excluded by none and included by nonenanoFramework.Device.Can.Stm32/CanControllerManager.csis excluded by none and included by nonenanoFramework.Device.Can.Stm32/CanEvent.csis excluded by none and included by nonenanoFramework.Device.Can.Stm32/CanMessage.csis excluded by none and included by nonenanoFramework.Device.Can.Stm32/CanMessageEvent.csis excluded by none and included by nonenanoFramework.Device.Can.Stm32/CanMessageFrameType.csis excluded by none and included by nonenanoFramework.Device.Can.Stm32/CanMessageIdType.csis excluded by none and included by nonenanoFramework.Device.Can.Stm32/CanMessageReceivedEventArgs.csis excluded by none and included by nonenanoFramework.Device.Can.Stm32/CanSettings.csis excluded by none and included by nonenanoFramework.Device.Can.Stm32/Properties/AssemblyInfo.csis excluded by none and included by nonenanoFramework.Device.Can.Stm32/key.snkis excluded by none and included by nonenanoFramework.Device.Can.Stm32/nanoFramework.Device.Can.Stm32.nfprojis excluded by none and included by nonenanoFramework.Device.Can.Stm32/packages.configis excluded by none and included by nonenanoFramework.Device.Can.slnis excluded by none and included by nonespelling_exclusion.dicis excluded by none and included by none
📒 Files selected for processing (7)
.github/.changelog-config.json(1 hunks).github/workflows/pr-checks.yml(1 hunks).github/workflows/update-dependencies.yml(2 hunks)azure-pipelines.yml(2 hunks)nanoFramework.Device.Can.Esp32/packages.lock.json(1 hunks)nanoFramework.Device.Can.Mcp2515/packages.lock.json(1 hunks)nanoFramework.Device.Can.Stm32/packages.lock.json(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- .github/workflows/update-dependencies.yml
- .github/.changelog-config.json
- nanoFramework.Device.Can.Stm32/packages.lock.json
- nanoFramework.Device.Can.Esp32/packages.lock.json
- nanoFramework.Device.Can.Mcp2515/packages.lock.json
🧰 Additional context used
🪛 YAMLlint (1.35.1)
azure-pipelines.yml
[error] 21-21: trailing spaces
(trailing-spaces)
[warning] 43-43: wrong indentation: expected 2 but found 0
(indentation)
[error] 58-58: trailing spaces
(trailing-spaces)
[warning] 67-67: wrong indentation: expected 4 but found 2
(indentation)
[error] 74-74: trailing spaces
(trailing-spaces)
[error] 78-78: trailing spaces
(trailing-spaces)
[error] 82-82: trailing spaces
(trailing-spaces)
[error] 86-86: trailing spaces
(trailing-spaces)
[error] 139-139: trailing spaces
(trailing-spaces)
[warning] 140-140: wrong indentation: expected 4 but found 2
(indentation)
[error] 146-146: trailing spaces
(trailing-spaces)
[warning] 148-148: wrong indentation: expected 4 but found 2
(indentation)
[error] 160-160: trailing spaces
(trailing-spaces)
[warning] 161-161: wrong indentation: expected 4 but found 2
(indentation)
[error] 165-165: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: nanoFramework.Device.Can (Build_Library)
- GitHub Check: nanoFramework / NuGets using latest version
- GitHub Check: nanoFramework.Device.Can (Report_Build_Failure)
- GitHub Check: nanoFramework.Device.Can (Build_Library)
🔇 Additional comments (3)
.github/workflows/pr-checks.yml (2)
13-14: Consistency Update: Added Solution ParameterThe addition of the
solution: 'nanoFramework.Device.Can.sln'parameter in thecheck_package_lockjob aligns it with the existing configuration for thecheck_nuget_latestjob. This change ensures both jobs reference the same solution file, which is essential for consistency as the repository transitions to a multi-package setup. Please verify that the solution filename matches the actual file in the repository.
15-20: Review Check: Matching Solution Reference in Both JobsThe
check_nuget_latestjob already specifies the same solution file, which confirms cohesive usage across the workflow. Maintaining the consistency of the solution reference helps avoid potential misconfigurations in the build pipeline.azure-pipelines.yml (1)
118-130: Update_Dependents Job: Condition Verification
The conditions for theUpdate_Dependentsjob (lines 118–130) are logically grouped. Please ensure that the variables used (such asStartReleaseCandidateandUPDATE_DEPENDENTS) are set appropriately elsewhere in the pipeline.
|
@josesimoes There is a bug in VersionCop that causes the build to fail. Bug fix available in other PR. |
|
@frobijn noticed that. Just fixed it. |
|
Why not using a shared project for common code? That would make things easier to maintain. And it's possible to have code conditions as well. All the M5Stack repo is built like this. This avoid duplication of code. |
|
@Ellerbach There is a lot of duplicated code because I have not yet made the split between Core and Stm32 code. I'm going to do that in a subsequent PR. The purpose of this PR is: update the repository to produce three NuGet packages that depend on a fourth, check that the pipeline will run properly, and make sure the core team is happy about the new repository structure. I've copied the current nanoFramework.Device.Can code to both Core and Stm32 projects. I thought it would make it easier to review the next PR with code split. It is then more obvious in which library the current code ends up, and also what code has to be changed to make the split work. BTW personally I try to avoid shared projects, especially since I've read this answer by one of the Roslyn developers. Although shared code is understood when consumed by regular projects, Roslyn and thus Visual Studio get really confused when there is a problem with the regular projects that makes the shared code incomprehensible. If one of the regular projects is fine but another not, Visual Studio doesn't seem to understand that and some IDE features do not work properly (e.g. formatting/cleanup on save, F12). You can still share code by linking code from one project to another - that works very well, as the linked code is considered to be part of the regular project and all IDE features know what to do. Although the linked code file looks the same in the IDE as a file from a shared project, Roslyn/Visual Studio has a better understanding of what it is and the IDE works better. The difference may go away in future versions of Visual Studio, but for now I find shared projects much harder to work with. |
|
So I guess the question is: is this the correct way the repository should be organized? And: if I prepare the next PR with code changes, should I send that as a separate PR or include these changes as well? How to proceed from here? |
|
I would prefer to directly have things clean. But ok to have subsequent PR. |
|
It's not my experience that shared projects work well. Mileage may vary... If the setup is now OK, then I'll start on the code changes. |
|
This is a WIP, duplicate code reported by sonarcloud will be addressed in future PRs. |
|
Description
Do not merge this PR!
It is the first step towards a multi-package repository. The PR is to make sure we're on the right track.
(Attempt 2 to trigger the pipeline again)
Transition of this Github repository into a repository for multiple NuGet packages/class libraries that collectively are the nanoFramework.Device.Can implementation in the near future.
Most work is done to achieve the correct build infrastructure:
The library code in this PR is not yet up to snuff:
The nanoFramework.Device.Can.Core project contains a complete copy of the current
nanoFramework.Device.Canlibrary. TheAssemblyNativeVersionattribute is removed for normal builds. In a subsequent PR the Stm32-specific code will be removed and optionally replaced by abstract base classes or interfaces.The nanoFramework.Device.Can.Stm32 project contains a complete copy of the current
nanoFramework.Device.Canlibrary. The version inAssemblyNativeVersionis changed to an arbitrary value. In a subsequent PR only the Stm32-specific code will be retained as an implementation of the classes and interfaces from the nanoFramework.Device.Can.Core library.The nanoFramework.Device.Can.Esp32 and nanoFramework.Device.Can.Mcp2515 projects are empty except for the assembly properties.
The subsequent PR that contains the correct versions of nanoFramework.Device.Can.Core and nanoFramework.Device.Can.Stm32 will also require a PR for the
nf-interpreterrepository as the stubs for the native components change. After the PR for both repositories are accepted and merged, the current nanoFramework.Device.Can NuGet package can be marked as deprecated.Motivation and Context
The conclusion of the discussion on the related PR.
How Has This Been Tested?
I have no idea how to test the Azure pipeline.
Types of changes
Checklist:
Summary by CodeRabbit