Patch ubuntu - #8
Conversation
This solves issue of dotnet not being found when installed with apt-get on ubuntu
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughFindDotNetHost now computes multiple .NET runtime identifiers, including Linux distribution-specific identifiers. It discovers additional host-pack roots from Changes.NET host discovery
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmake/FindDotNetHost.cmake (2)
29-30: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueHandle optional single quotes in
/etc/os-releasevalues.The current regular expressions correctly strip optional double quotes. However, according to the
os-releasespecification, values can also be enclosed in single quotes. Updating the regex to handle both quote types improves parsing robustness.💡 Proposed refactor
- string(REGEX REPLACE "^ID=\"?([^\"]*)\"?$" "\\1" _os_id "${_os_release_id}") - string(REGEX REPLACE "^VERSION_ID=\"?([^\"]*)\"?$" "\\1" _os_version "${_os_release_version}") + string(REGEX REPLACE "^ID=[\"']?([^\"']*)[\"']?$" "\\1" _os_id "${_os_release_id}") + string(REGEX REPLACE "^VERSION_ID=[\"']?([^\"']*)[\"']?$" "\\1" _os_version "${_os_release_version}")🤖 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 `@cmake/FindDotNetHost.cmake` around lines 29 - 30, Update the _os_id and _os_version extraction expressions in FindDotNetHost.cmake to strip optional matching single or double quotes, while preserving support for unquoted values and existing double-quoted inputs.
31-33: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd ARM64 support for distro-specific RIDs.
Ubuntu is widely used on ARM64 architectures (e.g., AWS Graviton, Raspberry Pi). Extending the distro-specific RID logic to support ARM64 ensures that
.NEThost packs installed viaapt-geton these platforms are also correctly discovered.💡 Proposed refactor
- if(_os_id AND _os_version AND CMAKE_SYSTEM_PROCESSOR MATCHES "^(x86_64|amd64)$") - list(APPEND _dotnet_host_rids "${_os_id}.${_os_version}-x64") - endif() + if(_os_id AND _os_version) + if(CMAKE_SYSTEM_PROCESSOR MATCHES "^(x86_64|amd64)$") + list(APPEND _dotnet_host_rids "${_os_id}.${_os_version}-x64") + elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "^(arm64|aarch64)$") + list(APPEND _dotnet_host_rids "${_os_id}.${_os_version}-arm64") + endif() + endif()🤖 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 `@cmake/FindDotNetHost.cmake` around lines 31 - 33, Extend the distro-specific RID logic in FindDotNetHost.cmake to support ARM64 processors in addition to x86_64/amd64. Update the processor check and RID construction so ARM64 platforms produce the correct distro RID suffix expected by installed .NET host packs, while preserving the existing x64 behavior.
🤖 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 `@cmake/FindDotNetHost.cmake`:
- Around line 64-75: Remove the REALPATH-derived snap directory from the
_dotnet_exe discovery path, and add the standard snap installation location
(typically /snap/dotnet/current) to the static candidates in the foreach list.
Preserve the existing apt and manual-install candidates and ensure snap
discovery no longer depends on resolving the /snap/bin/dotnet wrapper.
---
Nitpick comments:
In `@cmake/FindDotNetHost.cmake`:
- Around line 29-30: Update the _os_id and _os_version extraction expressions in
FindDotNetHost.cmake to strip optional matching single or double quotes, while
preserving support for unquoted values and existing double-quoted inputs.
- Around line 31-33: Extend the distro-specific RID logic in
FindDotNetHost.cmake to support ARM64 processors in addition to x86_64/amd64.
Update the processor check and RID construction so ARM64 platforms produce the
correct distro RID suffix expected by installed .NET host packs, while
preserving the existing x64 behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 783cebcd-deff-4d83-a91c-d1c8423f7918
📒 Files selected for processing (1)
cmake/FindDotNetHost.cmake
| # Ask the dotnet muxer itself where it lives, if one is on PATH. | ||
| # This picks up apt (/usr/lib/dotnet), snap, and manual installs alike. | ||
| find_program(_dotnet_exe NAMES dotnet) | ||
| if(_dotnet_exe) | ||
| get_filename_component(_dotnet_exe_resolved "${_dotnet_exe}" REALPATH) | ||
| get_filename_component(_dotnet_exe_dir "${_dotnet_exe_resolved}" DIRECTORY) | ||
| list(APPEND _dotnet_host_roots "${_dotnet_exe_dir}") | ||
| endif() | ||
|
|
||
| foreach(candidate IN ITEMS | ||
| "/usr/share/dotnet" | ||
| "/usr/lib/dotnet" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Incorrect REALPATH resolution for snap installs.
The comment indicates that this block picks up snap installations, but running REALPATH on a snap wrapper (e.g., /snap/bin/dotnet) resolves to the snap system binary /usr/bin/snap. Consequently, _dotnet_exe_dir becomes /usr/bin, which does not contain the .NET packs directory, and discovery for snap installs will silently fail.
To properly support snap installations, explicitly add the standard snap path to the static candidates list.
🐛 Proposed fix
# Ask the dotnet muxer itself where it lives, if one is on PATH.
-# This picks up apt (/usr/lib/dotnet), snap, and manual installs alike.
+# This picks up apt (/usr/lib/dotnet) and manual installs alike.
find_program(_dotnet_exe NAMES dotnet)
if(_dotnet_exe)
get_filename_component(_dotnet_exe_resolved "${_dotnet_exe}" REALPATH)
get_filename_component(_dotnet_exe_dir "${_dotnet_exe_resolved}" DIRECTORY)
list(APPEND _dotnet_host_roots "${_dotnet_exe_dir}")
endif()
foreach(candidate IN ITEMS
"/usr/share/dotnet"
"/usr/lib/dotnet"
+ "/snap/dotnet-sdk/current"📝 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.
| # Ask the dotnet muxer itself where it lives, if one is on PATH. | |
| # This picks up apt (/usr/lib/dotnet), snap, and manual installs alike. | |
| find_program(_dotnet_exe NAMES dotnet) | |
| if(_dotnet_exe) | |
| get_filename_component(_dotnet_exe_resolved "${_dotnet_exe}" REALPATH) | |
| get_filename_component(_dotnet_exe_dir "${_dotnet_exe_resolved}" DIRECTORY) | |
| list(APPEND _dotnet_host_roots "${_dotnet_exe_dir}") | |
| endif() | |
| foreach(candidate IN ITEMS | |
| "/usr/share/dotnet" | |
| "/usr/lib/dotnet" | |
| # Ask the dotnet muxer itself where it lives, if one is on PATH. | |
| # This picks up apt (/usr/lib/dotnet) and manual installs alike. | |
| find_program(_dotnet_exe NAMES dotnet) | |
| if(_dotnet_exe) | |
| get_filename_component(_dotnet_exe_resolved "${_dotnet_exe}" REALPATH) | |
| get_filename_component(_dotnet_exe_dir "${_dotnet_exe_resolved}" DIRECTORY) | |
| list(APPEND _dotnet_host_roots "${_dotnet_exe_dir}") | |
| endif() | |
| foreach(candidate IN ITEMS | |
| "/usr/share/dotnet" | |
| "/usr/lib/dotnet" | |
| "/snap/dotnet-sdk/current" |
🤖 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 `@cmake/FindDotNetHost.cmake` around lines 64 - 75, Remove the REALPATH-derived
snap directory from the _dotnet_exe discovery path, and add the standard snap
installation location (typically /snap/dotnet/current) to the static candidates
in the foreach list. Preserve the existing apt and manual-install candidates and
ensure snap discovery no longer depends on resolving the /snap/bin/dotnet
wrapper.
remove snap from comment since it does not do this
|
@timosachsenberg @jpfeuffer can you please review this when you get the chance? It works on my end but I am not an expert in cmake. |
|
haha same. but looks good to me. @jpfeuffer ? |
2 participants
These changes should allow cmake to locate the packs that are ubuntu specific (labelled ubuntu instead of linux). This occurs when dotnet is installed via the
apt-getcommand.This has only been tested on ubuntu so hopefully does not break anything else.
Changes have been tested locally on ubuntu 24.04 with dotnet installed via apt-get however are mainly written by AI.
This should remove the need for OpenMS/OpenMS#9746
Summary by CodeRabbit
Summary by CodeRabbit
dotnetinstallation and standard system locations.