-
Notifications
You must be signed in to change notification settings - Fork 115
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
Only run dnf commands on platforms that have dnf #794
Conversation
Reviewer's Guide by SourceryThis pull request refactors the build script by adding command availability checks and isolating platform-specific DNF installation logic into dedicated functions. Conditional execution of DNF commands is now implemented to ensure that commands are only run on platforms that support them, enhancing the script's portability and maintainability. Sequence diagram for conditional DNF command executionsequenceDiagram
participant Main as main()
participant Check as available()
participant Install as dnf_install()
participant Intel as dnf_install_intel_gpu()
participant Clean as dnf_clean
Main->>Check: Check availability of "dnf"
alt dnf available
Check-->>Main: returns true
Main->>Install: Invoke dnf_install()
alt containerfile == "intel-gpu"
Install->>Intel: Call dnf_install_intel_gpu()
Intel-->>Install: Install Intel GPU packages and source setvars.sh
end
else dnf not available
Check-->>Main: returns false
Note over Main: Skip DNF installation commands
end
Main->>Main: clone_and_build_whisper_cpp()
Main->>Main: clone_and_build_llama_cpp()
Main->>Check: Check availability of "dnf" for cleaning
alt dnf available
Check-->>Main: returns true
Main->>Clean: Execute "dnf -y clean all"
Clean-->>Main: Clean performed
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @ericcurtin - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider extracting the logic for installing common packages into a separate function to reduce duplication.
- It might be cleaner to use a case statement instead of multiple
elif
conditions for the different containerfile types.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
277679b
to
8b50496
Compare
set_install_prefix | ||
local common_flags | ||
configure_common_flags | ||
common_flags+=("-DGGML_CCACHE=OFF" "-DCMAKE_INSTALL_PREFIX=$install_prefix") | ||
dnf_install | ||
if available dnf; then |
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.
How about available dnf && dnf_install
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.
Yup same thing
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 have a tendency to default to C-style
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.
We may want to continue with
if available dnf; then
elif available apt; then
fi
here.
8b50496
to
fbdfd21
Compare
Build failed on pull of: Tiny-Vicuna taking the opportunity to move to an even smaller model, to accelerate the builds. |
5c1d731
to
74b9cf5
Compare
Other refactorings, fixes Signed-off-by: Eric Curtin <[email protected]>
74b9cf5
to
d1c5bb0
Compare
LGTM |
Other refactorings, fixes
Summary by Sourcery
Refactor the build script to conditionally run
dnf
commands only on platforms wherednf
is available. Extract the installation of Intel GPU packages into a dedicated function.Enhancements:
Build:
dnf
availability before runningdnf
commands.