fix: add support for non-Debian Linux package managers#401
fix: add support for non-Debian Linux package managers#401yurekami wants to merge 1 commit intounslothai:mainfrom
Conversation
Summary of ChangesHello @yurekami, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the software's compatibility across a wider range of Linux distributions. By integrating support for several new package managers and improving distribution detection, it aims to provide a more robust and user-friendly experience for users on various Linux environments, while also offering clearer guidance in case of installation issues. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves support for various Linux distributions by adding detection and package installation logic for Arch, Gentoo, Alpine, and openSUSE. The changes are well-structured, and the error messages are more informative. I've provided a few suggestions to improve maintainability and robustness by refactoring some duplicated code and making the distribution detection more consistent. Overall, this is a great enhancement.
unsloth_zoo/llama_cpp.py
Outdated
| pkg_mgr_names = { | ||
| "rpm": "yum/dnf", "arch": "pacman", "gentoo": "emerge", | ||
| "alpine": "apk", "suse": "zypper", "debian": "apt-get" | ||
| } | ||
| pkg_mgr_name = pkg_mgr_names.get(system_type, "package manager") |
There was a problem hiding this comment.
This pkg_mgr_names dictionary is duplicated in the do_we_need_sudo function (lines 244-248). To follow the DRY (Don't Repeat Yourself) principle and improve maintainability, you should define this dictionary as a module-level constant (e.g., PKG_MGR_NAMES) and use it in both places.
You can define the constant at the top of the file like so:
PKG_MGR_NAMES = {
"rpm": "yum/dnf", "arch": "pacman", "gentoo": "emerge",
"alpine": "apk", "suse": "zypper", "debian": "apt-get"
} pkg_mgr_name = PKG_MGR_NAMES.get(system_type, "package manager")
unsloth_zoo/llama_cpp.py
Outdated
| # Choose update command based on system type | ||
| if system_type == "rpm": | ||
| pkg_manager = "yum" if os.path.exists('/usr/bin/yum') else "dnf" | ||
| update_cmd = f"{pkg_manager} check-update" | ||
| else: | ||
| elif system_type == "arch": | ||
| update_cmd = "pacman -Sy" | ||
| elif system_type == "gentoo": | ||
| update_cmd = "emerge --sync" | ||
| elif system_type == "alpine": | ||
| update_cmd = "apk update" | ||
| elif system_type == "suse": | ||
| update_cmd = "zypper refresh" | ||
| elif system_type == "unknown": | ||
| # For unknown systems, skip the update check and assume sudo is needed | ||
| print("Unsloth: Unknown Linux distribution, skipping package manager check") | ||
| return True | ||
| else: # Default to debian/apt-get | ||
| update_cmd = "apt-get update -y" |
There was a problem hiding this comment.
For better maintainability and readability, you can refactor this long if/elif chain into a dictionary lookup. This makes it easier to add or modify support for package managers in the future.
# Choose update command based on system type
if system_type == "unknown":
# For unknown systems, skip the update check and assume sudo is needed
print("Unsloth: Unknown Linux distribution, skipping package manager check")
return True
update_commands = {
"arch": "pacman -Sy",
"gentoo": "emerge --sync",
"alpine": "apk update",
"suse": "zypper refresh",
"debian": "apt-get update -y",
}
if system_type == "rpm":
pkg_manager = "yum" if os.path.exists('/usr/bin/yum') else "dnf"
update_cmd = f"{pkg_manager} check-update"
else:
# Default to debian for any unhandled system type, which is the original behavior.
update_cmd = update_commands.get(system_type, update_commands["debian"])
unsloth_zoo/llama_cpp.py
Outdated
| pkg_mgr_names = { | ||
| "rpm": "yum/dnf", "arch": "pacman", "gentoo": "emerge", | ||
| "alpine": "apk", "suse": "zypper", "debian": "apt-get" | ||
| } | ||
| pkg_mgr_name = pkg_mgr_names.get(system_type, "package manager") |
There was a problem hiding this comment.
As mentioned in the comment for the install_package function, this dictionary is duplicated. Please replace it with a reference to the new module-level constant PKG_MGR_NAMES to adhere to the DRY principle.
pkg_mgr_name = PKG_MGR_NAMES.get(system_type, "package manager")| elif os.path.exists('/etc/SuSE-release') or os.path.exists('/etc/SUSE-brand'): | ||
| return 'suse' |
There was a problem hiding this comment.
For consistency with the detection logic for other distributions like Arch and Gentoo, you should also check for the existence of the zypper executable. This makes the detection more robust, especially in cases where the release files might be missing or in non-standard locations.
elif os.path.exists('/etc/SuSE-release') or os.path.exists('/etc/SUSE-brand') or os.path.exists('/usr/bin/zypper'):
return 'suse'![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
unsloth_zoo/llama_cpp.py
Outdated
| elif system_type == "arch": | ||
| install_cmd = f"{sudo_prefix}pacman -S --noconfirm {package}" |
There was a problem hiding this comment.
On Arch/Gentoo/Alpine/openSUSE, check_linux_type() now returns those system types, but check_libcurl_dev() still falls through to the Debian default and returns libcurl4-openssl-dev for any non‑debian/rpm system. With the new branches here, that Debian package name is passed directly to pacman/emerge/apk/zypper, which will report “package not found” and abort installation. This means the newly advertised distro support will still fail whenever libcurl headers are missing. You likely need distro-specific libcurl package mapping (e.g., curl/libcurl-devel/curl-dev) before invoking these package managers.
Useful? React with 👍 / 👎.
Extends package manager support beyond Debian/apt-get and RPM/yum to include: - Arch Linux (pacman) - Alpine Linux (apk) - openSUSE (zypper) - Gentoo (emerge) Changes: - Updated check_linux_type() to detect additional distributions - Added _get_pkg_manager_info() helper for update commands - Added _get_install_cmd() helper for install commands - Added _get_package_names_for_system() for distro-specific package names - Updated check_libcurl_dev() to check packages on all supported distros This allows users on non-Debian/RPM systems to use GGUF conversion without encountering "apt-get does not exist" errors. Fixes unslothai#251 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
yurekami
force-pushed
the
fix/multi-distro-package-managers
branch
from
8a18a5d to
2abc040
Compare
1 participant
Summary
check_linux_type()to detect these distributionsinstall_package()anddo_we_need_sudo()to use the appropriate package managerRelated Issue
Fixes #251
Test plan
🤖 Generated with Claude Code