Dependency installer for macOS, Debian/Ubuntu, and RedHat/CentOS #1017
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![qodo-merge-pro[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
scripts/install_dependencies.sh
Outdated
| # Verification | ||
| echo "Verifying installations..." | ||
| cmake --version || echo "cmake not found" | ||
| gcc --version || echo "gcc not found" | ||
| python3 --version || echo "python3 not found" | ||
| mpirun --version || echo "mpirun not found" | ||
| ldconfig -p | grep -q libfftw3 || echo "FFTW library not found" |
There was a problem hiding this comment.
✅ Suggestion: Replace the command || echo constructs in the verification steps with if ! command; then echo; fi blocks. This ensures the custom error messages are displayed correctly when set -e is active. [possible issue, importance: 7]
| # Verification | |
| echo "Verifying installations..." | |
| cmake --version || echo "cmake not found" | |
| gcc --version || echo "gcc not found" | |
| python3 --version || echo "python3 not found" | |
| mpirun --version || echo "mpirun not found" | |
| ldconfig -p | grep -q libfftw3 || echo "FFTW library not found" | |
| # Verification | |
| echo "Verifying installations..." | |
| if ! cmake --version; then echo "cmake not found"; fi | |
| if ! gcc --version; then echo "gcc not found"; fi | |
| if ! python3 --version; then echo "python3 not found"; fi | |
| if ! mpirun --version; then echo "mpirun not found"; fi | |
| if ! ldconfig -p | grep -q libfftw3; then echo "FFTW library not found"; fi |
scripts/install_dependencies.sh
Outdated
| # Install missing packages | ||
| for exe in "${!packages[@]}"; do | ||
| formula="${packages[$exe]}" | ||
| if ! command -v "$exe" >/dev/null 2>&1; then | ||
| echo "Installing $formula..." | ||
| brew install --verbose "$formula" | ||
| else | ||
| echo "$exe already installed" | ||
| fi | ||
| done |
There was a problem hiding this comment.
✅ Suggestion: Modify the macOS package check to use brew list instead of command -v. This correctly detects installed Homebrew libraries like boost and protobuf, preventing unnecessary reinstallations. [possible issue, importance: 8]
| # Install missing packages | |
| for exe in "${!packages[@]}"; do | |
| formula="${packages[$exe]}" | |
| if ! command -v "$exe" >/dev/null 2>&1; then | |
| echo "Installing $formula..." | |
| brew install --verbose "$formula" | |
| else | |
| echo "$exe already installed" | |
| fi | |
| done | |
| # Install missing packages | |
| for exe in "${!packages[@]}"; do | |
| formula="${packages[$exe]}" | |
| if ! brew list --formula | grep -q "^${formula%%@*}$"; then | |
| echo "Installing $formula..." | |
| brew install --verbose "$formula" | |
| else | |
| echo "$formula already installed" | |
| fi | |
| done |
|
Can you test any of the others via VMs? Otherwise, it's really hard to know if this 'works' or not. Also, try to make a PR change that involves the source code! |
|
This PR introduces a script but doesn't point the user to it, nor use it in CI, anywhere. Marking as draft as it's clearly incomplete. |
- scripts/install_dependencies.sh: installs required dependencies on CI - .github/workflows/test.yml: updated workflow to call the install_dependencies.sh script - CMakeLists.txt: added dependency check/integration for CI
|
Update as of 10/19:
Files included:
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
There was a problem hiding this comment.
High-level Suggestion
The dependency_check executable should be removed from the main CMakeLists.txt because it is only used for CI. Instead, the dependency verification should be handled directly within the install_dependencies.sh script. [High-level, importance: 8]
Solution Walkthrough:
Before:
# In CMakeLists.txt
# ... (main project configuration)
# Small dependency-check executable (used by CI to verify deps like Boost)
find_package(Boost REQUIRED)
add_executable(dependency_check examples/dependency_check/dependency_check.cpp)
target_include_directories(dependency_check PRIVATE ${Boost_INCLUDE_DIRS})
# No check inside install_dependencies.sh
After:
# In CMakeLists.txt
# ... (main project configuration)
# The dependency_check executable and related find_package(Boost) are removed.
# In scripts/install_dependencies.sh
# ... (after installing dependencies)
echo "Verifying Boost dependency by compiling a test file..."
cat <<EOF > /tmp/boost_check.cpp
#include <boost/version.hpp>
#include <iostream>
int main() { std::cout << "Boost version: " << BOOST_LIB_VERSION << std::endl; return 0; }
EOF
g++ /tmp/boost_check.cpp -o /tmp/boost_check $(pkg-config --cflags --libs boost || echo "-I${Boost_INCLUDE_DIRS}")
/tmp/boost_check
scripts/install_dependencies.sh
Outdated
| if [[ "${CI_INTEL:-false}" == "true" ]]; then | ||
| echo "Intel OneAPI environment detected. Installing Intel compilers and MPI..." | ||
| wget https://apt.repos.intel.com/intel-gpg-keys/GPG-PUB-KEY-INTEL-SW-PRODUCTS.PUB | ||
| $SUDO apt-key add GPG-PUB-KEY-INTEL-SW-PRODUCTS.PUB |
There was a problem hiding this comment.
✅ Suggestion: Replace the deprecated apt-key command with the modern, secure method of managing APT keys. Store the GPG key in /etc/apt/keyrings and reference it from a dedicated sources list file. [security, importance: 8]
| $SUDO apt-key add GPG-PUB-KEY-INTEL-SW-PRODUCTS.PUB | |
| $SUDO install -m 0644 GPG-PUB-KEY-INTEL-SW-PRODUCTS.PUB /etc/apt/keyrings/intel-archive-keyring.gpg | |
| echo "deb [signed-by=/etc/apt/keyrings/intel-archive-keyring.gpg] https://apt.repos.intel.com/oneapi all main" | $SUDO tee /etc/apt/sources.list.d/oneapi.list > /dev/null |
scripts/install_dependencies.sh
Outdated
| source /opt/intel/oneapi/setvars.sh | ||
| if [[ -n "${GITHUB_ENV:-}" ]]; then | ||
| echo "source /opt/intel/oneapi/setvars.sh" >> "$GITHUB_ENV" | ||
| fi | ||
| printenv >> "$GITHUB_ENV" |
There was a problem hiding this comment.
✅ Suggestion: Correct the method for persisting environment variables from setvars.sh in the GitHub Actions environment. Instead of dumping all variables with printenv, capture and append only the variables modified by the script to $GITHUB_ENV. [possible issue, importance: 10]
| source /opt/intel/oneapi/setvars.sh | |
| if [[ -n "${GITHUB_ENV:-}" ]]; then | |
| echo "source /opt/intel/oneapi/setvars.sh" >> "$GITHUB_ENV" | |
| fi | |
| printenv >> "$GITHUB_ENV" | |
| # Create a temporary file to store the environment before sourcing | |
| env > /tmp/env_before | |
| # Source the Intel script to set variables for the current shell | |
| source /opt/intel/oneapi/setvars.sh | |
| # Compare the environment before and after, and append only the new/changed variables to GITHUB_ENV | |
| if [[ -n "${GITHUB_ENV:-}" ]]; then | |
| diff --unchanged-line-format="" --old-line-format="" --new-line-format="%L" <(sort /tmp/env_before) <(env | sort) >> "$GITHUB_ENV" | |
| fi | |
| rm /tmp/env_before |
scripts/install_dependencies.sh
Outdated
| pkgs="tar wget make cmake3 gcc gcc-c++ python3 openmpi fftw protobuf boost-devel" | ||
| $SUDO yum install -y $pkgs |
There was a problem hiding this comment.
✅ Suggestion: Add missing development packages (-devel) to the yum installation list for RedHat/CentOS. The current list lacks headers and libraries for openmpi, fftw, protobuf, and python3, which are required for compilation. [possible issue, importance: 9]
| pkgs="tar wget make cmake3 gcc gcc-c++ python3 openmpi fftw protobuf boost-devel" | |
| $SUDO yum install -y $pkgs | |
| pkgs="tar wget make cmake3 gcc gcc-c++ python3-devel openmpi openmpi-devel fftw fftw-devel protobuf protobuf-devel boost-devel" | |
| $SUDO yum install -y $pkgs |
|
very cool! keep going... seems to be almost there |
|
Update as of 10/24: macOS:
Debian/Ubuntu:
RHEL/CentOS:
Boost verification: Compiles a tiny program with safe fallbacks if Boost include paths are unknown. |
|
This seems to be going the wrong direction, and now you've replaced the CMakeLists with something entirely different. Not sure what's going on. |
Aadharsh-Rajkumar
force-pushed
the
ci-build-script
branch
from
f6084cb to
aa497c8
Compare
…and added suggested fixes for dependency installer
|
Update as of 10/27: I undid the past commit and have implemented all suggestions, most importantly Boost compile checks and secure APT key handling. I have tested both on MacOS and Ubuntu through WSL. Specific Key Changes:
|
|
I think this might be the wrong project for you, thanks for your interest but I'm closing this PR. |
2 participants
User description
Description
A solution for consolidating dependency installation commands for different operating systems (for macOS, Debian/Ubuntu, RedHat/CentOS). Allows minimal repetition in command installations in multiple CI workflow files. CI logs on which commands are being run.
Fixes #660
Type of change
Scope
How Has This Been Tested?
By running these two commands:
I was only able to test on a macOS system so for other OS like ubuntu and RedHat tests still need to be done.
Checklist
docs/)examples/that demonstrate my new feature performing as expected.They run to completion and demonstrate "interesting physics"
./mfc.sh formatbefore committing my codeI left unchecked boxes for things that I did not find relevant for this issue which was a shell script and not a Fortran or C++ file.
PR Type
Enhancement
Description
Consolidate OS-specific dependency installation into unified script
Replace inline CI workflow setup with centralized install_dependencies.sh
Support macOS, Debian/Ubuntu, and RedHat/CentOS with conditional logic
Add dependency verification executable and enforce LF line endings
Diagram Walkthrough
File Walkthrough
install_dependencies.sh
Cross-platform dependency installation scriptscripts/install_dependencies.sh
with yum
environments
test.yml
Refactor CI workflow to use unified installer.github/workflows/test.yml
install_dependencies.sh invocation
CMakeLists.txt
Add dependency verification executable targetCMakeLists.txt
.gitattributes
Enforce consistent LF line endings across file types.gitattributes