Conversation
|
Do we need this? @kiran-thumma still a draft? |
There was a problem hiding this comment.
Pull request overview
This PR generalizes the wheel build pipeline to support an arbitrary, caller-provided list of Python versions (e.g., adding Python 3.13 support) by threading a python_versions/PYTHON_VERSIONS knob through CI workflows and the wheel build script.
Changes:
- Add
python_versionsas a workflow input in CI and forward it into the wheel build workflow/container. - Update the wheel build workflow to install the requested Python versions and copy
fly-optfrom the first version’s build directory. - Refactor
scripts/build_wheels.shto build wheels in a loop overPYTHON_VERSIONSinstead of hardcoded 3.10/3.12 paths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
scripts/build_wheels.sh |
Replaces hardcoded per-version variables with a PYTHON_VERSIONS-driven loop for building wheels. |
.github/workflows/ci.yaml |
Adds python_versions (and release_type) workflow-dispatch inputs and forwards them to the wheel-build workflow. |
.github/workflows/build-whl.yaml |
Installs requested Python versions in-container, passes PYTHON_VERSIONS into the build, and copies fly-opt dynamically. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| PY310_BIN="${PY310_BIN:-python3.10}" | ||
| PY312_BIN="${PY312_BIN:-python3.12}" | ||
| PYTHON_VERSIONS="${PYTHON_VERSIONS:-3.10 3.12}" | ||
|
|
There was a problem hiding this comment.
Now that PYTHON_VERSIONS is user-configurable, it can be set to an empty/whitespace-only value; in that case the build loop won't build any wheels and the script will later fail when listing dist/*.whl (with a less clear error). Consider validating early that PYTHON_VERSIONS contains at least one version token and exiting with a clear message if it doesn't.
| # Validate that PYTHON_VERSIONS contains at least one version token. | |
| read -r -a _fly_python_versions <<< "${PYTHON_VERSIONS}" | |
| if [[ ${#_fly_python_versions[@]} -eq 0 ]]; then | |
| echo "Error: PYTHON_VERSIONS is empty; set it to at least one Python version (e.g. '3.10')." >&2 | |
| exit 1 | |
| fi |
coderfeli
left a comment
There was a problem hiding this comment.
Security: Shell injection via ${{ inputs.python_versions }} (2 locations)
Two places in build-whl.yaml interpolate ${{ inputs.python_versions }} directly into the runner shell / docker exec, which allows command injection if the workflow_dispatch input contains shell metacharacters (;, $(), newlines, single quotes).
Location 1 — Runner shell (Install build dependencies step)
for ver in ${{ inputs.python_versions }}; do
docker exec flydsl_build bash -c "command -v python${ver} || apt-get install -y ..."
doneLocation 2 — Docker exec (Build wheels step)
docker exec flydsl_build bash -c "... export PYTHON_VERSIONS='${{ inputs.python_versions }}' ..."The single-quote wrapping breaks if the input itself contains a single quote.
Suggested fix
Pass the input via env: instead of inline interpolation:
- name: Install build dependencies
env:
PYTHON_VERSIONS: ${{ inputs.python_versions }}
run: |
docker exec flydsl_build bash -c "add-apt-repository -y ppa:deadsnakes/ppa && apt-get update"
for ver in ${PYTHON_VERSIONS}; do
docker exec flydsl_build bash -c "command -v python${ver} || apt-get install -y python${ver} python${ver}-dev python${ver}-venv"
done
- name: Build wheels
env:
PYTHON_VERSIONS: ${{ inputs.python_versions }}
run: |
docker exec -e PYTHON_VERSIONS="${PYTHON_VERSIONS}" flydsl_build bash -c \
"export MLIR_PATH=/llvm-project/mlir_install && export ALLOW_ANY_GLIBC=1 && export FLYDSL_RELEASE_TYPE=${{ inputs.release_type }} && cd /flydsl && bash scripts/build_wheels.sh"This way inputs.python_versions is set as an environment variable by the Actions runner (which handles quoting correctly), and never directly embedded in shell code.
Motivation
The FlyDSL wheel build pipeline was hardcoded to only build for Python 3.10 and 3.12. This PR generalizes the build system to accept an arbitrary list of Python versions, enabling support for Python 3.13 (and any future versions) without requiring further code changes. This unblocks users and downstream consumers who have adopted Python 3.13.
Technical Details
3 files changed across CI workflows and the build script:
.github/workflows/build-whl.yamlpython_versionsworkflow input (default:"3.10 3.12") so callers can specify which Python versions to build wheels for.python3.12install step with a loop that installs each requested Python version from the deadsnakes PPA (skipping versions already present in the Docker image).PYTHON_VERSIONSenv var into the Docker build container.fly-opt, instead of hardcodingbuild_py312..github/workflows/ci.yamlpython_versionsinput (default:"3.10 3.12") and forwards it to thebuild-whlworkflow.scripts/build_wheels.shPY310_BIN/PY312_BIN,VENV_310/VENV_312, andFLY_BUILD_DIR_310/FLY_BUILD_DIR_312variables with a singlePYTHON_VERSIONSenvironment variable.ensure_python_bins()to loop overPYTHON_VERSIONSdynamically.main()build loop to derivepy_tag(e.g.,cp313),pybin,venvpath, andbuild_dirfrom each version string.PYTHON_VERSIONSknob.To build with Python 3.13, callers simply pass
python_versions: "3.10 3.12 3.13"to the workflow.Test Plan
ci.yamlworkflow withpython_versions: "3.10 3.12"(default) and verify wheels are built for both versions.ci.yamlworkflow withpython_versions: "3.10 3.12 3.13"and verify acp313wheel is produced.fly-optbinary is correctly copied from the first version's build directory.Test Result
Pending
Submission Checklist