Make create-env-files.sh respect min python version supported - #1446
Make create-env-files.sh respect min python version supported#1446mhucka wants to merge 1 commit into
create-env-files.sh respect min python version supported#1446Conversation
This makes `dev_tools/requirements/create-env-files.sh` get the minimum version of Python the project supports from pyproject.toml, and use that as a constraint passed to `uv`.
There was a problem hiding this comment.
Code Review
This pull request updates the dev_tools/requirements/create-env-files.sh script to dynamically determine the minimum Python version from pyproject.toml and introduces a new -p / --min-python option to override this default. Feedback on these changes highlights three key areas for improvement: handling scenarios where the script is run outside of a git repository to avoid cryptic failures, making the inline Python parser more robust against missing keys in pyproject.toml to prevent unhandled exceptions, and refactoring the argument parsing loop so that it does not prematurely terminate when encountering unrecognized options.
| script_dir=$(CDPATH="" cd -- "$(dirname -- "${0}")" && pwd -P) | ||
| repo_dir=$(git -C "${script_dir}" rev-parse --show-toplevel 2>/dev/null) | ||
| cd "${repo_dir}" |
There was a problem hiding this comment.
If the script is run outside of a git repository (for example, from a source archive/tarball) or if git is not installed, repo_dir will be empty. Because set -o errexit is enabled, the subsequent cd "${repo_dir}" will fail with a cryptic error and terminate the script immediately. We should check if repo_dir is empty and provide a clear, helpful error message.
| script_dir=$(CDPATH="" cd -- "$(dirname -- "${0}")" && pwd -P) | |
| repo_dir=$(git -C "${script_dir}" rev-parse --show-toplevel 2>/dev/null) | |
| cd "${repo_dir}" | |
| script_dir=$(CDPATH="" cd -- "$(dirname -- "${0}")" && pwd -P) | |
| repo_dir=$(git -C "${script_dir}" rev-parse --show-toplevel 2>/dev/null) | |
| if [[ -z "${repo_dir}" ]]; then | |
| echo "Error: Failed to determine repository root. Are you in a git repository?" >&2 | |
| exit 1 | |
| fi | |
| cd "${repo_dir}" |
| default_min_python=$(python3 -c 'import re | ||
| s = open("pyproject.toml").read() | ||
| m = re.search(r"requires-python.*?>=?(\d+\.\d+)", s) | ||
| if not m: | ||
| m = re.search(r"target-version.*?py(\d)(\d+)", s) | ||
| print(m.group(1) if "requires-python" in m.re.pattern else m.expand(r"\1.\2")) | ||
| ' 2>/dev/null || echo "3.10") |
There was a problem hiding this comment.
If pyproject.toml exists but contains neither requires-python nor target-version, the python script will raise an unhandled AttributeError when trying to access m.re or m.expand on a None object. While the shell script catches this with || echo "3.10", relying on silent python crashes is fragile and makes debugging harder. We should handle the None case and potential exceptions gracefully within the Python snippet.
| default_min_python=$(python3 -c 'import re | |
| s = open("pyproject.toml").read() | |
| m = re.search(r"requires-python.*?>=?(\d+\.\d+)", s) | |
| if not m: | |
| m = re.search(r"target-version.*?py(\d)(\d+)", s) | |
| print(m.group(1) if "requires-python" in m.re.pattern else m.expand(r"\1.\2")) | |
| ' 2>/dev/null || echo "3.10") | |
| default_min_python=$(python3 -c 'import re | |
| try: | |
| s = open("pyproject.toml").read() | |
| m = re.search(r"requires-python.*?>=?(\d+\.\d+)", s) | |
| if m: | |
| print(m.group(1)) | |
| else: | |
| m = re.search(r"target-version.*?py(\d)(\d+)", s) | |
| print(m.expand(r"\1.\2") if m else "3.10") | |
| except Exception: | |
| print("3.10") | |
| ' 2>/dev/null) |
There was a problem hiding this comment.
Slop and Bloat Gemini, Yuck!
| while [[ $# -gt 0 ]]; do | ||
| case "${1}" in | ||
| -h|--help) echo "${usage}"; exit 0 ;; | ||
| -p|--min-python|--python-version) | ||
| if [[ $# -lt 2 || -z "${2:-}" ]]; then | ||
| echo "Error: option '${1}' requires an argument" >&2 | ||
| echo "${usage}" >&2 | ||
| exit 1 | ||
| fi | ||
| min_python=("--python-version" "${2}") | ||
| shift 2 | ||
| ;; | ||
| *) break ;; | ||
| esac | ||
| done |
There was a problem hiding this comment.
The current argument parsing loop terminates on the first unrecognized option (using *) break ;;). If a user passes any other options (such as --upgrade or --no-cache) before -p or --min-python, the script will stop parsing arguments, leaving -p unparsed and passing it directly to uv pip compile, which will fail because uv does not recognize -p.
We can fix this by collecting unrecognized options into an array and resetting the positional parameters ($@) at the end of the loop.
| while [[ $# -gt 0 ]]; do | |
| case "${1}" in | |
| -h|--help) echo "${usage}"; exit 0 ;; | |
| -p|--min-python|--python-version) | |
| if [[ $# -lt 2 || -z "${2:-}" ]]; then | |
| echo "Error: option '${1}' requires an argument" >&2 | |
| echo "${usage}" >&2 | |
| exit 1 | |
| fi | |
| min_python=("--python-version" "${2}") | |
| shift 2 | |
| ;; | |
| *) break ;; | |
| esac | |
| done | |
| uv_options=() | |
| while [[ $# -gt 0 ]]; do | |
| case "${1}" in | |
| -h|--help) echo "${usage}"; exit 0 ;; | |
| -p|--min-python|--python-version) | |
| if [[ $# -lt 2 || -z "${2:-}" ]]; then | |
| echo "Error: option '${1}' requires an argument" >&2 | |
| echo "${usage}" >&2 | |
| exit 1 | |
| fi | |
| min_python=("--python-version" "${2}") | |
| shift 2 | |
| ;; | |
| *) | |
| uv_options+=("${1}") | |
| shift | |
| ;; | |
| esac | |
| done | |
| set -- "${uv_options[@]}" |
pavoljuhas
left a comment
There was a problem hiding this comment.
Please set python-version via pyproject.toml instead. Most of the changes in this PR can be then reverted.
| declare -r usage="Usage: ${0} [-h] [UV_OPTIONS] | ||
| # Go to the top of the local TFQ git tree. Do it early in case this fails. | ||
| script_dir=$(CDPATH="" cd -- "$(dirname -- "${0}")" && pwd -P) | ||
| repo_dir=$(git -C "${script_dir}" rev-parse --show-toplevel 2>/dev/null) |
There was a problem hiding this comment.
We should keep the stderr message so user gets a clue what is going on if the script exits here on git-rev-parse failure.
| repo_dir=$(git -C "${script_dir}" rev-parse --show-toplevel 2>/dev/null) | |
| repo_dir=$(git -C "${script_dir}" rev-parse --show-toplevel) |
| repo_dir=$(git -C "${script_dir}" rev-parse --show-toplevel 2>/dev/null) | ||
| cd "${repo_dir}" | ||
|
|
||
| default_min_python=$(python3 -c 'import re |
There was a problem hiding this comment.
This is way too complicated and as is it extracts the black formatter setting which may be unrelated to the actual supported version. A better way would be to grep setup.py for python_requires, but I feel even that would be more fragile and more laborious to maintain in a long term than to just set
default_min_python=3.10
and bump it up when the minimum required version goes up.
Add: not needed at all per #1446 (comment)
There was a problem hiding this comment.
Setup.py is going away in another PR, which is why this doesn't look in setup.py
|
|
||
| Options: | ||
| -h Show this help message and exit | ||
| -h Show this help message and exit |
There was a problem hiding this comment.
| -h Show this help message and exit | |
| -h, --help Show this help message and exit |
| while [[ $# -gt 0 ]]; do | ||
| case "${1}" in | ||
| -h|--help) echo "${usage}"; exit 0 ;; | ||
| -p|--min-python|--python-version) |
There was a problem hiding this comment.
This is shadowing the uv-pip -p, --python option which would be ignored if specified first, but processed if placed as a later argument. In addition, user would not be able to pass --python-version=X.Y.Z because uv does not accept that option multiple times.
I recommend to add the python-version setting to pyproject.toml instead and drop the custom -p, --min-python option here altogether:
diff --git a/pyproject.toml b/pyproject.toml
index 72ba61d..b8de85d 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -73,4 +73,5 @@ follow_untyped_imports = true
[tool.uv.pip]
universal = true
generate-hashes = true
custom-compile-command = "dev_tools/requirements/create-env-files.sh"
+python-version = "3.10"
| # ~~~~ Generate basic requirements files ~~~~ | ||
|
|
||
| uv pip compile "$@" \ | ||
| uv pip compile "${min_python[@]}" "$@" \ |
There was a problem hiding this comment.
This and below can be reverted after setting python-version in pyproject.toml.
Good point. There's another PR that will move setup.py settings to pyproject.toml. I'll close this one and then check if any changes are even needed after that, because the python version set in pyproject.toml may be all that's needed. |
This makes
dev_tools/requirements/create-env-files.shget the minimum version of Python the project supports from pyproject.toml, and use that as a constraint passed touv.