feat(tools): allow to specify python binary by ESP_PYTHON_CUSTOM environment… (IDFGH-16803) #17880
Conversation
|
|
👋 Hello garywill, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
github-actions
bot
changed the title
| $p_cmd -c "import sys; exit(1) if sys.version_info.major < int(\"$OLDEST_PYTHON_SUPPORTED_MAJOR\") else exit(0);" || continue | ||
| $p_cmd -c "import sys; exit(1) if sys.version_info.minor < int(\"$OLDEST_PYTHON_SUPPORTED_MINOR\") else exit(0);" || continue | ||
| "$p_cmd" -c "import sys; exit(1) if sys.version_info.major < int(\"$OLDEST_PYTHON_SUPPORTED_MAJOR\") else exit(0);" || continue | ||
| "$p_cmd" -c "import sys; exit(1) if sys.version_info.minor < int(\"$OLDEST_PYTHON_SUPPORTED_MINOR\") else exit(0);" || continue |
There was a problem hiding this comment.
Bug: Inconsistent Python Version Validation Logic
The version validation checks major and minor versions independently, creating inconsistent behavior for future Python versions. Python 4.0-4.9 would be rejected (minor < 10) while Python 4.10+ would be accepted, even though Python 4.x may have breaking changes. The logic should either accept all Python 4+ versions or only Python 3.10+, not create an arbitrary cutoff at 4.10.
| $ESP_PYTHON --version 2>/dev/null || { | ||
| echo "Python ${OLDEST_PYTHON_SUPPORTED_MAJOR}.${OLDEST_PYTHON_SUPPORTED_MINOR}+ is not installed! Please see the documentation for how to install it." | ||
| [[ -n "$ESP_PYTHON" ]] && "$ESP_PYTHON" --version 2>/dev/null || { | ||
| echo "Python ${OLDEST_PYTHON_SUPPORTED_MAJOR}.${OLDEST_PYTHON_SUPPORTED_MINOR}+ is not installed! Please see the documentation for how to install it." >&2 |
There was a problem hiding this comment.
Bug: Misleading Python Version Error Message
When a user specifies ESP_PYTHON pointing to a Python binary that doesn't meet version requirements, the error message "Python 3.10+ is not installed!" is misleading. Python 3.10+ might be installed on the system, but the user-specified binary doesn't meet requirements. The error should distinguish between these cases to avoid confusion.
tools/detect_python.fish
Outdated
| $p_cmd -c "import sys; exit(1) if sys.version_info.minor < int(\"$OLDEST_PYTHON_SUPPORTED_MINOR\") else exit(0);"; or continue | ||
|
|
||
| set ESP_PYTHON $p_cmd | ||
| set -x ESP_PYTHON $p_cmd |
There was a problem hiding this comment.
Bug: Inconsistent Validation for Future Python Versions
The version validation checks major and minor versions independently, creating inconsistent behavior for future Python versions. Python 4.0-4.9 would be rejected (minor < 10) while Python 4.10+ would be accepted, even though Python 4.x may have breaking changes. The logic should either accept all Python 4+ versions or only Python 3.10+, not create an arbitrary cutoff at 4.10.
| set -e PYTHON_CANDIDATES | ||
|
|
||
| test -n "$ESP_PYTHON"; or echo "Python $OLDEST_PYTHON_SUPPORTED_MAJOR.$OLDEST_PYTHON_SUPPORTED_MINOR+ is not installed! Please see the documentation for how to install it." | ||
| test -n "$ESP_PYTHON"; or echo "Python $OLDEST_PYTHON_SUPPORTED_MAJOR.$OLDEST_PYTHON_SUPPORTED_MINOR+ is not installed! Please see the documentation for how to install it." >&2 |
There was a problem hiding this comment.
Bug: Misleading Python Version Error Message
When a user specifies ESP_PYTHON pointing to a Python binary that doesn't meet version requirements, the error message "Python 3.10+ is not installed!" is misleading. Python 3.10+ might be installed on the system, but the user-specified binary doesn't meet requirements. The error should distinguish between these cases to avoid confusion.
|
Hello @garywill , thank you for your contribution. I generally have no objections, but there is one issue that might cause problems. With these changes, the $ shellcheck --shell=sh export.sh tools/detect_python.sh
In tools/detect_python.sh line 14:
if [[ -z "$ESP_PYTHON" ]]; then
^--------------------^ SC3010 (warning): In POSIX sh, [[ ]] is undefined.
In tools/detect_python.sh line 15:
PYTHON_CANDIDATES=(python3 python python3.10 python3.11 python3.12 python3.13)
^-- SC3030 (warning): In POSIX sh, arrays are undefined.
In tools/detect_python.sh line 18:
PYTHON_CANDIDATES=("$ESP_PYTHON")
^-------------^ SC3030 (warning): In POSIX sh, arrays are undefined.
In tools/detect_python.sh line 22:
for p_cmd in "${PYTHON_CANDIDATES[@]}"; do
^---------------------^ SC3054 (warning): In POSIX sh, array references are undefined.
In tools/detect_python.sh line 34:
[[ -n "$ESP_PYTHON" ]] && "$ESP_PYTHON" --version 2>/dev/null || {
^--------------------^ SC3010 (warning): In POSIX sh, [[ ]] is undefined.
^-- SC2015 (info): Note that A && B || C is not if-then-else. C may run when A is true.
For more information:
https://www.shellcheck.net/wiki/SC3010 -- In POSIX sh, [[ ]] is undefined.
https://www.shellcheck.net/wiki/SC3030 -- In POSIX sh, arrays are undefined.
https://www.shellcheck.net/wiki/SC3054 -- In POSIX sh, array references are...
Perhaps we can use OLDEST_PYTHON_SUPPORTED_MAJOR=3
OLDEST_PYTHON_SUPPORTED_MINOR=10
PYTHON_CANDIDATES="python3
python
python3.10
python3.11
python3.12
python3.13"
if [ -n "$ESP_PYTHON" ]; then
echo "Reading ESP_PYTHON from environment: \"$ESP_PYTHON\""
PYTHON_CANDIDATES="$ESP_PYTHON"
unset ESP_PYTHON
fi
while IFS= read -r p_cmd; do
echo "Checking \"$p_cmd\" ..."
"$p_cmd" --version >/dev/null 2>&1 || continue
"$p_cmd" -c "import sys; exit(1) if sys.version_info.major < int(\"$OLDEST_PYTHON_SUPPORTED_MAJOR\") else exit(0);" || continue
"$p_cmd" -c "import sys; exit(1) if sys.version_info.minor < int(\"$OLDEST_PYTHON_SUPPORTED_MINOR\") else exit(0);" || continue
ESP_PYTHON="$p_cmd"
break
done << EOF
$PYTHON_CANDIDATES
EOF
if [ -z "$ESP_PYTHON" ]; then
echo "Python ${OLDEST_PYTHON_SUPPORTED_MAJOR}.${OLDEST_PYTHON_SUPPORTED_MINOR}+ is not installed! Please see the documentation for how to install it." >&2
exit 1
fi
echo "\"$ESP_PYTHON\" has been detected"Or maybe a different approach to ensure the script remains POSIX compliant. Thank you! |
Because if we 'unset ESP_PYTHON' in during python binary check, it may break user's custom environment. So we should separate those two variables
Using 'exit' may cause user shell killed if user runs '. export.sh'. So use function to wrap the code.
garywill
changed the title
| return 1 | ||
| fi | ||
| echo "\"$ESP_PYTHON\" has been detected" | ||
| export ESP_PYTHON |
There was a problem hiding this comment.
Bug: Bash version doesn't display Python version
The bash version no longer displays the Python version after detection, while the fish version still does at line 32. The old bash code ran $ESP_PYTHON --version to show the version, but the new code only prints a detection message. This creates inconsistent output between the bash and fish scripts, where users see the Python version in fish but not in bash.
|
I make it POSIX compatitable, and did some changes:
|
Thank you, it's much appreciated!
I agree, but not because "it may break the user's custom environment". This is an internal variable, and it has always been explicitly set to Without disregarding the previously set $ ESP_PYTHON_CUSTOM=python3.11 . ./tools/detect_python.sh
Reading ESP_PYTHON_CUSTOM from environment: "python3.11"
Checking "python3.11" ...
"python3.11" has been detected
$ echo $ESP_PYTHON
python3.11
$ ESP_PYTHON_CUSTOM=NOPYTHON . ./tools/detect_python.sh
Reading ESP_PYTHON_CUSTOM from environment: "NOPYTHON"
"python3.11" has been detected
$ echo $ESP_PYTHON # previously python is used, instead of error(ENOENT)
python3.11
I also think this might not work as expected from within already activated(after
same with
The problem is with IDF_PYTHON_ENV_PATH which is set with I guess we can unset it if
Yes, this problem is there probably for a long time. I guess that it's a very rare condition(no python) that actually triggers this code path, so nobody noticed or complained. Agreed this should fixed. I've been playing with this slightly modified sh version # Ignore any previously set ESP_PYTHON value. It is the responsibility of this script to set it correctly.
unset ESP_PYTHON
OLDEST_PYTHON_SUPPORTED_MAJOR=3
OLDEST_PYTHON_SUPPORTED_MINOR=10
PYTHON_CANDIDATES="python3
python
python3.10
python3.11
python3.12
python3.13"
if [ -n "${ESP_PYTHON_CUSTOM:-}" ]; then
echo "Reading ESP_PYTHON_CUSTOM from environment: \"$ESP_PYTHON_CUSTOM\""
PYTHON_CANDIDATES="$ESP_PYTHON_CUSTOM"
# If a custom Python version is requested, ignore any previously set Python
# environment path.
unset IDF_PYTHON_ENV_PATH
fi
while IFS= read -r p_cmd; do
"$p_cmd" --version >/dev/null 2>&1 || continue
echo "Checking \"$p_cmd\" ..."
"$p_cmd" -c "import sys; exit(1) if sys.version_info.major < int(\"$OLDEST_PYTHON_SUPPORTED_MAJOR\") else exit(0);" || continue
"$p_cmd" -c "import sys; exit(1) if sys.version_info.minor < int(\"$OLDEST_PYTHON_SUPPORTED_MINOR\") else exit(0);" || continue
ESP_PYTHON="$p_cmd"
break
done << EOF
$PYTHON_CANDIDATES
EOF
unset PYTHON_CANDIDATES
if [ -z "${ESP_PYTHON:-}" ]; then
echo "Python ${OLDEST_PYTHON_SUPPORTED_MAJOR}.${OLDEST_PYTHON_SUPPORTED_MINOR}+ is not installed! Please see the documentation for how to install it." >&2
return 1
fi
echo "\"$ESP_PYTHON\" has been detected"
which seems to be working, but there might be other issues. This still might require some additional work. Note: I dropped the function, because IMHO it's not needed based on the dot and return description. I also dropped the In my experience, almost every time we touch these scripts, something breaks for someone :). Thank you very much! |
4 participants
Description
If environment variable
ESP_PYTHON_CUSTOMalready set,detect_python.shuses that python binary, otherwise, looks for python version same or greater than minimal required version on path.That allow user to choose a particular python version or python binary
Notice:
Related
Testing
I did some simple test with bash
Checklist
Before submitting a Pull Request, please ensure the following:
Note
Adds
ESP_PYTHON_CUSTOMsupport to select a specific Python and makes install/export scripts fail fast on Python detection errors; docs updated (EN/ZN).tools/detect_python.shandtools/detect_python.fishto honorESP_PYTHON_CUSTOM, select the first supported interpreter, exportESP_PYTHON, and return non-zero with stderr on failure.install.sh,install.fish,export.sh,export.fishto source detection with|| exit/return 1and proceed using"$ESP_PYTHON".ESP_PYTHON_CUSTOMusage indocs/en/*anddocs/zh_CN/*install/setup guides with example (export ESP_PYTHON_CUSTOM=/opt/bin/python3.13).Written by Cursor Bugbot for commit 9d3eee7. This will update automatically on new commits. Configure here.