Skip to content

Conversation

@Adarsha1999
Copy link
Member

@Adarsha1999 Adarsha1999 commented Jul 9, 2025

@Adarsha1999 Adarsha1999 force-pushed the ceph-build-pull-request-refactor branch from e623748 to a898d32 Compare July 9, 2025 13:30
Comment on lines 20 to 21
elif command -v python2.7 > /dev/null; then
virtualenv -p python2.7 $path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know youre copying this from the other script now, but perhaps it's also a good time to drop python2 support entirely. WDYT?

Copy link
Member Author

@Adarsha1999 Adarsha1999 Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phlogistonjohn good catch , yes we can do that. Actually I am also modifying something like I am removing the function keyword to make it POSIX compliant.

@Adarsha1999 Adarsha1999 force-pushed the ceph-build-pull-request-refactor branch from a898d32 to 9e5db2d Compare July 9, 2025 16:34
@dmick
Copy link
Member

dmick commented Jul 9, 2025

So I assume we are creating a smaller reusable chunk of code in setup_deps.sh here.

  1. It looks ilke this is strictly about python dependencies, so I think the script name should reflect that.
  2. it would be nice to understand, either by description or, preferably, to be able to see a diff of this version of the code and the one that is currently in build_utils.sh to know exactly how it has changed. (your hint about function defs would seem to indicate that something has changed). The point is so that the reviewer can easily review what the actual delta is between the old code and the new reorganized code.
  3. can you assert that other jobs that use these shell functions will be able to use them compatibly from the new script setup_deps.sh as well, so that this is one step on a future path?

@Adarsha1999
Copy link
Member Author

Hi @dmick ,

This PR is part of the ongoing refactor of the build_utils.sh script. A few key points:

Script Name: I've named the new script setup_deps.sh since it focuses on setting up dependencies and can be reused across other jobs that require similar functionality. Open to suggestions if you have a better name in mind.

Function Scope: All functions included in this script are migrated from build_utils.sh. There are no new additions—only modifications such as dropping Python 2 support and ensuring POSIX compliance.

Future Use: The goal is to make this script reusable across multiple jobs. We're also working on logically splitting the functions—for example, separating setup-related logic from RHEL-specific build steps.

@dmick
Copy link
Member

dmick commented Jul 14, 2025

script name: well, yes, as I said, it seems to be strictly Python dependencies, so I would definitely include that in the name, something like, oh, say, setup_python_deps.sh

function movement: yes, but if there's no easy way to get diffs, there's no easy way to review it. Please provide text for review of the transported code.

goals: good, but what I meant to try to encourage is looking at future users to see that you're doing things in a way compatible with their use, so that hopefully what changes in future is only the callers, and not the callee here

@Adarsha1999 Adarsha1999 force-pushed the ceph-build-pull-request-refactor branch from 9e5db2d to db6060b Compare July 15, 2025 16:07
@Adarsha1999
Copy link
Member Author

Hello @dmick

  1. Updated the name of the script based on the feedback
  2. For review
    function create_venv_dir() {
    local venv_dir
    venv_dir=$(mktemp -td venv.XXXXXXXXXX)
    trap "rm -rf ${venv_dir}" EXIT
    echo "${venv_dir}"
    }
    function release_from_version() {
    local ver=$1
    case $ver in
    20.*)
    rel="tentacle"
    ;;
    19.*)
    rel="squid"
    ;;
    18.*)
    rel="reef"
    ;;
    17.*)
    rel="quincy"
    ;;
    16.*)
    rel="pacific"
    ;;
    15.*)
    rel="octopus"
    ;;
    14.*)
    rel="nautilus"
    ;;
    13.*)
    rel="mimic"
    ;;
    12.*)
    rel="luminous"
    ;;
    11.*)
    rel="kraken"
    ;;
    10.*)
    rel="jewel"
    ;;
    9.*)
    rel="infernalis"
    ;;
    0.94.*)
    rel="hammer"
    ;;
    0.87.*)
    rel="giant"
    ;;
    0.80.*)
    rel="firefly"
    ;;
    0.72.*)
    rel="emperor"
    ;;
    0.67.*)
    rel="dumpling"
    ;;
    *)
    rel="unknown"
    echo "ERROR: Unknown release for version '$ver'" > /dev/stderr
    echo $rel
    exit 1
    ;;
    esac
    echo $rel
    }
    branch_slash_filter() {
    # The build system relies on an HTTP binary store that uses branches/refs
    # as URL parts. A literal extra slash in the branch name is considered
    # illegal, so this function performs a check *and* prunes the common
    # `origin/branch-name` scenario (which is OK to have).
    RAW_BRANCH=$1
    branch_slashes=$(grep -o "/" <<< ${RAW_BRANCH} | wc -l)
    FILTERED_BRANCH=`echo ${RAW_BRANCH} | rev | cut -d '/' -f 1 | rev`
    # Prevent building branches that have slashes in their name
    if [ "$((branch_slashes))" -gt 1 ] ; then
    echo "Will refuse to build branch: ${RAW_BRANCH}"
    echo "Invalid branch name (contains slashes): ${FILTERED_BRANCH}"
    exit 1
    fi
    echo $FILTERED_BRANCH
    }
    pip_download() {
    local venv=$1
    shift
    local package=$1
    shift
    local options=$@
    if ! $venv/pip download $options --dest="$PIP_SDIST_INDEX" $package; then
    # pip <8.0.0 does not have "download" command
    $venv/pip install $options \
    --upgrade --exists-action=i --cache-dir="$PIP_SDIST_INDEX" \
    $package
    fi
    }
    create_virtualenv () {
    local path=$1
    if [ -d $path ]; then
    echo "Will reuse existing virtual env: $path"
    else
    if command -v python3 > /dev/null; then
    python3 -m venv $path
    elif command -v python2.7 > /dev/null; then
    virtualenv -p python2.7 $path
    else
    virtualenv -p python $path
    fi
    fi
    }
    install_python_packages_no_binary () {
    local venv_dir=$1
    shift
    local venv="$venv_dir/bin"
    # Use this function to create a virtualenv and install python packages
    # without compiling (or using wheels). Pass a list of package names. If
    # the virtualenv exists it will get re-used since this function can be used
    # along with install_python_packages
    #
    # Usage (with pip==24.0 [the default]):
    #
    # to_install=( "ansible" "chacractl>=0.0.21" )
    # install_python_packages_no_binary $TEMPVENV "to_install[@]"
    #
    # Usage (with pip<X.X.X [can also do ==X.X.X or !=X.X.X]):
    #
    # to_install=( "ansible" "chacractl>=0.0.21" )
    # install_python_packages_no_binary $TEMPVENV "to_install[@]" "pip<X.X.X"
    #
    # Usage (with latest pip):
    #
    # to_install=( "ansible" "chacractl>=0.0.21" )
    # install_python_packages_no_binary $TEMPVENV "to_install[@]" latest
    create_virtualenv $venv_dir
    # Define and ensure the PIP cache
    PIP_SDIST_INDEX="$HOME/.cache/pip"
    mkdir -p $PIP_SDIST_INDEX
    if [ "$2" == "latest" ]; then
    echo "Ensuring latest pip is installed"
    $venv/pip install -U pip
    elif [[ -n $2 && "$2" != "latest" ]]; then
    echo "Installing $2"
    $venv/pip install "$2"
    else
    # This is the default for most jobs.
    # See ff01d2c5 and fea10f52
    echo "Installing pip 24.0"
    $venv/pip install "pip==24.0"
    fi
    echo "Ensuring latest wheel is installed"
    $venv/pip install -U wheel
    echo "Updating setuptools"
    pip_download $VENV setuptools
    pkgs=("${!1}")
    for package in ${pkgs[@]}; do
    echo $package
    # download packages to the local pip cache
    pip_download $VENV $package --no-binary=:all:
    # install packages from the local pip cache, ignoring pypi
    $venv/pip install --no-binary=:all: --upgrade --exists-action=i --find-links="file://$PIP_SDIST_INDEX" --no-index $package
    done
    }
    install_python_packages () {
    local venv_dir=$1
    shift
    local venv="$venv_dir/bin"
    # Use this function to create a virtualenv and install
    # python packages. Pass a list of package names.
    #
    # Usage (with pip 24.0 [the default]):
    #
    # to_install=( "ansible" "chacractl>=0.0.21" )
    # install_python_packages $TEMPVENV "to_install[@]"
    #
    # Usage (with pip<X.X.X [can also do ==X.X.X or !=X.X.X]):
    #
    # to_install=( "ansible" "chacractl>=0.0.21" )
    # install_python_packages_no_binary $TEMPVENV "to_install[@]" "pip<X.X.X"
    #
    # Usage (with latest pip):
    #
    # to_install=( "ansible" "chacractl>=0.0.21" )
    # install_python_packages $TEMPVENV "to_install[@]" latest
    create_virtualenv $venv_dir
    # Define and ensure the PIP cache
    PIP_SDIST_INDEX="$HOME/.cache/pip"
    mkdir -p $PIP_SDIST_INDEX
    # Avoid UnicodeErrors when installing packages.
    # See https://github.com/ceph/ceph/pull/42811
    export LC_ALL=en_US.UTF-8
    if [ "$2" == "latest" ]; then
    echo "Ensuring latest pip is installed"
    $venv/pip install -U pip
    elif [[ -n $2 && "$2" != "latest" ]]; then
    echo "Installing $2"
    $venv/pip install "$2"
    else
    # This is the default for most jobs.
    # See ff01d2c5 and fea10f52
    echo "Installing pip 24.0"
    $venv/pip install "pip==24.0"
    fi
    echo "Ensuring latest wheel is installed"
    $venv/pip install -U wheel
    echo "Updating setuptools"
    pip_download $venv setuptools
    pkgs=("${!1}")
    for package in ${pkgs[@]}; do
    echo $package
    # download packages to the local pip cache
    pip_download $venv $package
    # install packages from the local pip cache, ignoring pypi
    $venv/pip install --upgrade --exists-action=i --find-links="file://$PIP_SDIST_INDEX" --no-index $package
    done
    # See https://tracker.ceph.com/issues/59652
    echo "Pinning urllib3 and requests"
    $venv/pip install "urllib3<2.0.0"
    $venv/pip install "requests<2.30.0"
    }

@Adarsha1999
Copy link
Member Author

retest

@Adarsha1999 Adarsha1999 force-pushed the ceph-build-pull-request-refactor branch from db6060b to bf32bcf Compare August 4, 2025 17:54
@djgalloway
Copy link
Contributor

Out of fear of creating a split brain scenario, I still don't think duplicating code is the right answer for this endeavor. e.g.,

  1. install_python_packages is copied to scripts/install_python_packages.sh
  2. Only one job is reconfigured to use this new separate script.
  3. install_python_packages() is changed in scripts/build_utils.sh but not install_python_packages.sh (or even vice versa)
  4. No one notices
  5. scripts/build_utils.sh is finally fully deprecated
  6. All the other jobs that were still using build_utils.sh break

@dmick
Copy link
Member

dmick commented Aug 18, 2025

I'm assuming this is only affecting one job because we're attempting to stage one job, and then will shortly follow with the others, but yes, there's a potential for change in only one branch of the duplicated code. It's not ideal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants