Fix: make dev_setup.sh cross-platform (Windows & Linux/Mac) — follow-up to #770 #867
Conversation
- Detect python (python3/python/py -3) - Create venv only if missing - Activate via bin/activate or Scripts/activate - Upgrade pip - Install requirements.txt if present - Follow-up to OpenSPP#770
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
Summary of ChangesHello @devendra1973, 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 introduces a significantly improved Highlights
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 introduces a helpful cross-platform developer setup script. My review focuses on improving its robustness and usability. I've suggested replacing eval with a safer alternative using bash arrays to handle Python command variations. I've also recommended providing clear instructions for the user on how to activate the virtual environment in their shell, as the script's activation is not persistent. These changes will make the script more secure and user-friendly.
| if command -v python3 >/dev/null 2>&1; then | ||
| PY=python3 | ||
| elif command -v python >/dev/null 2>&1; then | ||
| PY=python | ||
| elif command -v py >/dev/null 2>&1; then | ||
| PY="py -3" | ||
| else | ||
| echo "❌ Python not found. Install Python 3 and try again." | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Create venv only if missing | ||
| if [ ! -d "$VENV_DIR" ]; then | ||
| eval $PY -m venv "$VENV_DIR" | ||
| fi |
There was a problem hiding this comment.
Using eval can be a security risk if the variable contains untrusted input. While it's safe in this specific context as you control the inputs, it's a best practice to avoid it. You can use a bash array to handle commands with arguments, which is safer and more robust.
This also simplifies the command execution when creating the virtual environment.
| if command -v python3 >/dev/null 2>&1; then | |
| PY=python3 | |
| elif command -v python >/dev/null 2>&1; then | |
| PY=python | |
| elif command -v py >/dev/null 2>&1; then | |
| PY="py -3" | |
| else | |
| echo "❌ Python not found. Install Python 3 and try again." | |
| exit 1 | |
| fi | |
| # Create venv only if missing | |
| if [ ! -d "$VENV_DIR" ]; then | |
| eval $PY -m venv "$VENV_DIR" | |
| fi | |
| if command -v python3 >/dev/null 2>&1; then | |
| PY_CMD=(python3) | |
| elif command -v python >/dev/null 2>&1; then | |
| PY_CMD=(python) | |
| elif command -v py >/dev/null 2>&1; then | |
| PY_CMD=(py -3) | |
| else | |
| echo "❌ Python not found. Install Python 3 and try again." | |
| exit 1 | |
| fi | |
| # Create venv only if missing | |
| if [ ! -d "$VENV_DIR" ]; then | |
| "${PY_CMD[@]}" -m venv "$VENV_DIR" | |
| fi |
| if [ -f "$VENV_DIR/bin/activate" ]; then | ||
| # shellcheck source=/dev/null | ||
| source "$VENV_DIR/bin/activate" | ||
| elif [ -f "$VENV_DIR/Scripts/activate" ]; then | ||
| # shellcheck source=/dev/null | ||
| source "$VENV_DIR/Scripts/activate" | ||
| else | ||
| echo "❌ Could not find activate script in $VENV_DIR" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Upgrade pip | ||
| python -m pip install --upgrade pip | ||
|
|
||
| # Install deps if present | ||
| if [ -f "requirements.txt" ]; then | ||
| pip install -r requirements.txt | ||
| else | ||
| echo "ℹ️ No requirements.txt — skipping" | ||
| fi | ||
|
|
||
| echo "✅ Environment Ready" |
There was a problem hiding this comment.
The source command within the script activates the virtual environment only for the duration of the script's execution. When the script finishes, the user's parent shell will not have the virtual environment activated, which might be unexpected. To improve usability, it's helpful to inform the user how to activate it manually in their shell.
This can be done by storing the path to the activation script and printing a helpful message at the end.
# Activate (Linux/Mac = bin; Windows = Scripts)
ACTIVATE_SCRIPT=""
if [ -f "$VENV_DIR/bin/activate" ]; then
ACTIVATE_SCRIPT="$VENV_DIR/bin/activate"
elif [ -f "$VENV_DIR/Scripts/activate" ]; then
ACTIVATE_SCRIPT="$VENV_DIR/Scripts/activate"
fi
if [ -z "$ACTIVATE_SCRIPT" ]; then
echo "❌ Could not find activate script in $VENV_DIR"
exit 1
fi
# shellcheck source=/dev/null
source "$ACTIVATE_SCRIPT"
# Upgrade pip
python -m pip install --upgrade pip
# Install deps if present
if [ -f "requirements.txt" ]; then
pip install -r requirements.txt
else
echo "ℹ️ No requirements.txt — skipping"
fi
echo "✅ Environment Ready"
echo "To use it, run:"
echo "source $ACTIVATE_SCRIPT"2 participants
This PR improves the developer setup script:
Tested on Windows (Git Bash).
Note
Introduces a cross-platform
scripts/dev_setup.shthat creates/activates a Python venv, upgrades pip, and installsrequirements.txtif present.scripts/dev_setup.sh:python3/python/py -3)..venvif missing and activates it (Linux/Macbin, WindowsScripts).pipand installs dependencies fromrequirements.txtwhen available.Written by Cursor Bugbot for commit 1e33a1f. This will update automatically on new commits. Configure here.