Consume Harmony via NuGet instead of submodule#2354
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughRemoves the ChangesHarmony submodule → NuGet + optional local source
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
💻 FW Lite
C# FwHeadless Unit Tests48 tests 48 ✅ 15s ⏱️ Results for commit 058dad3. ♻️ This comment has been updated with latest results. |
C# Unit Tests165 tests 165 ✅ 21s ⏱️ Results for commit 058dad3. ♻️ This comment has been updated with latest results. |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
# Conflicts: # backend/harmony
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/Harmony.props`:
- Line 7: The HarmonySourcePath property definition uses
System.IO.Path::GetFullPath with backslash path separators which fails on
Linux/macOS platforms where backslashes are treated as literal characters.
Replace the System.IO.Path::GetFullPath call with MSBuild::NormalizePath in both
the RepoRoot property (line 6) and the HarmonySourcePath property (line 7) to
ensure cross-platform path composition compatibility with .NET 8+. This will
properly normalize path separators on all platforms without hardcoding
platform-specific backslashes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ca966302-8f6a-4360-a75e-b6a20968c469
📒 Files selected for processing (32)
.claude/agents/harmony-sentinel.md.claude/hooks/session-start.sh.claude/skills/_shared/apply-rules.md.claude/skills/_shared/dispatch-matrix.md.github/workflows/codeql.yml.github/workflows/fw-lite.yaml.github/workflows/integration-test-gha.yaml.github/workflows/integration-test.yaml.github/workflows/lexbox-api.yaml.github/workflows/lexbox-fw-headless.yaml.gitignore.gitmodulesFwLiteOnly.slnfLexBox.slnLexBoxOnly.slnfREADME.mdTaskfile.ymlbackend/AGENTS.mdbackend/Directory.Build.propsbackend/Directory.Build.props.user.examplebackend/Directory.Packages.propsbackend/FwHeadless/FwHeadless.csprojbackend/FwHeadless/dev.Dockerfilebackend/FwLite/LcmCrdt/LcmCrdt.csprojbackend/Harmony.App.References.propsbackend/Harmony.Core.References.propsbackend/Harmony.Linq2db.References.propsbackend/Harmony.propsbackend/LexBoxApi/LexBoxApi.csprojbackend/LexBoxApi/dev.Dockerfilebackend/LexData/LexData.csprojbackend/harmony
💤 Files with no reviewable changes (9)
- .gitmodules
- .github/workflows/lexbox-fw-headless.yaml
- .github/workflows/lexbox-api.yaml
- .github/workflows/codeql.yml
- backend/harmony
- .github/workflows/integration-test.yaml
- .github/workflows/integration-test-gha.yaml
- Taskfile.yml
- LexBox.sln
|
|
||
| <PropertyGroup> | ||
| <RepoRoot>$([System.IO.Path]::GetFullPath('$(MSBuildThisFileDirectory)..'))</RepoRoot> | ||
| <HarmonySourcePath Condition="'$(HarmonySourcePath)' == ''">$([System.IO.Path]::GetFullPath('$(RepoRoot)\..\harmony'))</HarmonySourcePath> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
On .NET/MSBuild running on Linux/macOS, does System.IO.Path.GetFullPath treat backslash "\" as a directory separator, and is MSBuild::NormalizePath recommended for cross-platform path construction in .props files?
💡 Result:
On.NET (starting with.NET 8), System.IO.Path.GetFullPath no longer treats the backslash character ("") as a directory separator on Linux and macOS [1]. Because backslashes are valid characters in Unix filenames, the.NET runtime now preserves them literally rather than converting them to forward slashes [1]. Previously, older versions of.NET performed this automatic conversion [1]. Regarding MSBuild property functions for cross-platform path construction, the use of [MSBuild]::NormalizePath or [MSBuild]::NormalizeDirectory is recommended for handling path normalization and ensuring consistent directory separators [2][3]. However, users should be aware that these functions may behave differently across operating systems [4]. For example, [MSBuild]::NormalizePath may not always canonicalize backslashes into forward slashes on non-Windows platforms [4]. To ensure cross-platform compatibility in.props files: 1. Prefer forward slashes ("/") as directory separators, as they are widely supported across platforms [1][3]. 2. Use MSBuild property functions like [MSBuild]::NormalizePath to combine and normalize paths, but verify that the output meets your project's specific requirements [2]. 3. For cross-platform logic, utilize helpers like [MSBuild]::IsOSUnixLike to conditionally adjust path construction if necessary [3]. 4. Avoid hardcoding backslashes in.props or.targets files, as these can lead to issues on Unix-based systems where they are interpreted as literal file name characters rather than separators [1][5].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/8.0/file-path-backslash
- 2: https://github.com/dotnet/skills/blob/main/plugins/dotnet-msbuild/skills/property-patterns/SKILL.md
- 3: https://stackoverflow.com/questions/75134157/vary-the-build-output-path-depending-on-the-current-os
- 4: MSBuild::NormalizePath does not correctly canocalize paths containing back-slashes on xplat dotnet/msbuild#5114
- 5: Normalize paths to remove \ on UNIX when fetching them from props in BuildCheck dotnet/msbuild#10238
🏁 Script executed:
# First, let's examine the Harmony.props file to understand the context
cat -n backend/Harmony.propsRepository: sillsdev/languageforge-lexbox
Length of output: 984
🏁 Script executed:
# Check for any .csproj, .vbproj or other project files that might indicate .NET version
fd -e csproj -e vbproj -e fsproj | head -5Repository: sillsdev/languageforge-lexbox
Length of output: 325
🏁 Script executed:
# Let's check if there are any other references to path handling in related files
rg "HarmonySourcePath|UseHarmonySource" --type xml -A 2 -B 2Repository: sillsdev/languageforge-lexbox
Length of output: 55
🏁 Script executed:
# Check what .NET version is targeted - look at a few key project files
head -20 backend/FwHeadless/FwHeadless.csprojRepository: sillsdev/languageforge-lexbox
Length of output: 1181
🏁 Script executed:
# Also check the root directory for any central package or version management
find . -maxdepth 2 -name "Directory.Build.props" -o -name "global.json" -o -name ".net-version" | head -5Repository: sillsdev/languageforge-lexbox
Length of output: 119
🏁 Script executed:
# Check global.json for .NET version
cat global.jsonRepository: sillsdev/languageforge-lexbox
Length of output: 152
🏁 Script executed:
# Check Directory.Build.props for target framework info
cat backend/Directory.Build.props | head -50Repository: sillsdev/languageforge-lexbox
Length of output: 2630
Use OS-neutral path composition for RepoRoot and HarmonySourcePath.
System.IO.Path::GetFullPath with backslash separators (\..\) will not work on Linux/macOS when targeting .NET 8+. The .NET runtime now treats backslashes as literal filename characters rather than path separators on Unix platforms. This causes HarmonySourcePath to resolve incorrectly when UseHarmonySource=true, leading to "project not found" errors.
Both line 6 (RepoRoot) and line 7 (HarmonySourcePath) are affected and should use MSBuild::NormalizePath for cross-platform compatibility.
Proposed fix
- <RepoRoot>$([System.IO.Path]::GetFullPath('$(MSBuildThisFileDirectory)..'))</RepoRoot>
- <HarmonySourcePath Condition="'$(HarmonySourcePath)' == ''">$([System.IO.Path]::GetFullPath('$(RepoRoot)\..\harmony'))</HarmonySourcePath>
+ <RepoRoot>$([MSBuild]::NormalizePath('$(MSBuildThisFileDirectory)', '..'))</RepoRoot>
+ <HarmonySourcePath Condition="'$(HarmonySourcePath)' == ''">$([MSBuild]::NormalizePath('$(RepoRoot)', '..', 'harmony'))</HarmonySourcePath>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/Harmony.props` at line 7, The HarmonySourcePath property definition
uses System.IO.Path::GetFullPath with backslash path separators which fails on
Linux/macOS platforms where backslashes are treated as literal characters.
Replace the System.IO.Path::GetFullPath call with MSBuild::NormalizePath in both
the RepoRoot property (line 6) and the HarmonySourcePath property (line 7) to
ensure cross-platform path composition compatibility with .NET 8+. This will
properly normalize path separators on all platforms without hardcoding
platform-specific backslashes.
2 participants
Summary
backend/harmonygit submodule; LexBox now depends onSIL.Harmony,SIL.Harmony.Core, andSIL.Harmony.Linq2dbfrom NuGet (pinned inDirectory.Packages.props).UseHarmonySource=true, default sibling path../harmony).task setup.Test plan
dotnet build backend/LexBoxApi/LexBoxApi.csprojdotnet build backend/FwLite/LcmCrdt/LcmCrdt.csprojdotnet build backend/FwHeadless/FwHeadless.csprojUseHarmonySource=truewith a local../harmonyclone