-
Notifications
You must be signed in to change notification settings - Fork 10.6k
fix(tools): correct python version check in op.sh #37064
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
base: master
Are you sure you want to change the base?
fix(tools): correct python version check in op.sh #37064
Conversation
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.
Pull request overview
This PR improves the Python version check in op.sh by replacing a fragile string comparison approach with a robust version evaluation using Python's packaging.specifiers module. The change allows the script to properly handle three-digit version numbers (e.g., 3.12.3) and complex version specifiers with various comparison operators.
Changes:
- Simplified extraction of the required Python version from
pyproject.toml - Updated the installed Python version detection to capture full version strings including patch numbers
- Replaced manual version comparison logic with
packaging.specifiers.SpecifierSetfor accurate version validation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return 1 | ||
| fi | ||
|
|
||
| if python3 -c "from packaging.specifiers import SpecifierSet; import sys; exit(0 if '$INSTALLED_PYTHON_VERSION' in SpecifierSet('$REQUIRED_PYTHON_VERSION') else 1)" 2>/dev/null; then |
Copilot
AI
Feb 2, 2026
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.
The 2>/dev/null suppresses all errors from the Python command, including import errors if the packaging module is missing. This could lead to confusing behavior where the check silently fails. Consider capturing stderr and providing a helpful error message if packaging is not installed, or document the dependency requirement more explicitly in the script's error output.
| if python3 -c "from packaging.specifiers import SpecifierSet; import sys; exit(0 if '$INSTALLED_PYTHON_VERSION' in SpecifierSet('$REQUIRED_PYTHON_VERSION') else 1)" 2>/dev/null; then | |
| # Ensure the required 'packaging' module is available before using it for version checks. | |
| if ! python3 -c "import packaging.specifiers" >/dev/null 2>&1; then | |
| echo -e " ↳ [${RED}✗${NC}] The Python 'packaging' module is required but not installed in your environment." | |
| echo " Install it with: python3 -m pip install packaging" | |
| loge "ERROR_PYTHON_PACKAGING_MISSING" | |
| return 1 | |
| fi | |
| if python3 -c "from packaging.specifiers import SpecifierSet; import sys; exit(0 if '$INSTALLED_PYTHON_VERSION' in SpecifierSet('$REQUIRED_PYTHON_VERSION') else 1)"; 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.
We could add an extra check if they don't mind the extra code
Fixes #37063
Makes the Python version check more robust by switching from a simple comparison check for version strings like
3.11and3.13to evaluating the versions using thepackaging.specifiersPython package.This now allow for 3 digits (e.g.
3.12.3) and also will work with any combination of comparison operators (e.g.>,<=).As far as I can tell, the "packaging" module is usually included in Ubuntu, but if not, it would need to be added to the Ubuntu script dependencies as "python3-packaging".