[GitHub] Add python 3.7 to libclang python test #77219
Conversation
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-github-workflow Author: Craig Hesling (linux4life798) ChangesThis enables the libclang python binding test to check It is important to check this for issue #76664, since Additionally, frustration around ever increasing Adding this additional build step will increase the Issue #76664. Full diff: https://github.com/llvm/llvm-project/pull/77219.diff 2 Files Affected:
diff --git a/.github/workflows/libclang-python-tests.yml b/.github/workflows/libclang-python-tests.yml
index 73edb6cf3bad26..e12acbc0f6ce8c 100644
--- a/.github/workflows/libclang-python-tests.yml
+++ b/.github/workflows/libclang-python-tests.yml
@@ -30,6 +30,10 @@ jobs:
check-clang-python:
# Build libclang and then run the libclang Python binding's unit tests.
name: Build and run Python unit tests
+ strategy:
+ fail-fast: false
+ matrix:
+ python-version: ["3.7", "3.11"]
uses: ./.github/workflows/llvm-project-tests.yml
with:
build_target: check-clang-python
@@ -37,3 +41,4 @@ jobs:
# There is an issue running on "windows-2019".
# See https://github.com/llvm/llvm-project/issues/76601#issuecomment-1873049082.
os_list: '["ubuntu-latest"]'
+ python_version: ${{ matrix.python-version }}
diff --git a/.github/workflows/llvm-project-tests.yml b/.github/workflows/llvm-project-tests.yml
index 594831ee6b5f52..a6caa94f2d158a 100644
--- a/.github/workflows/llvm-project-tests.yml
+++ b/.github/workflows/llvm-project-tests.yml
@@ -15,6 +15,10 @@ on:
os_list:
required: false
default: '["ubuntu-latest", "windows-2019", "macOS-12"]'
+ python_version:
+ required: false
+ type: string
+ default: '3.11'
workflow_call:
inputs:
build_target:
@@ -36,6 +40,11 @@ on:
# https://developercommunity.visualstudio.com/t/Prev-Issue---with-__assume-isnan-/1597317
default: '["ubuntu-latest", "windows-2019", "macOS-12"]'
+ python_version:
+ required: false
+ type: string
+ default: '3.11'
+
concurrency:
# Skip intermediate builds: always.
# Cancel intermediate builds: only if it is a pull request build.
@@ -65,7 +74,7 @@ jobs:
- name: Setup Python
uses: actions/setup-python@v4
with:
- python-version: '3.11'
+ python-version: ${{ inputs.python_version }}
- name: Install Ninja
uses: llvm/actions/install-ninja@main
# actions/checkout deletes any existing files in the new git directory,
|
linux4life798
force-pushed
the
add-python-3.7-testing
branch
from
ac9b92c to
b34f420
Compare
There was a problem hiding this comment.
I'm fine landing this with support for just testing 3.7 for now. Hopefully the python version bump discussion goes somewhere. Probably want to wait and see what @tru has to say though.
This will be used for testing the libclang Python binding against different versions of Python. Issue llvm#76664. Issue llvm#76601.
This enables the libclang python binding test to check the oldest version of Python supported in addition to the normal python version. It is important to check this for issue llvm#76664, since many new mainstream python type annotation features and best practices are not compatible with older versions of python. Additionally, frustration around ever increasing platform dependencies and versions has been raised. This will help ensure that python maintains reasonable backwards compatibility. Adding this additional build step will increase the run time, but this should always be minimal, since the additional libclang compilation should see 100% cache hit rate. Issue llvm#76664. Fixes llvm#76601.
linux4life798
force-pushed
the
add-python-3.7-testing
branch
from
b34f420 to
59a427a
Compare
|
I dropped the note in |
|
Do you need someone else to merge this? |
|
Thank you all for the review! Unfortunately, I don't have commit rights. Could someone merge this PR? |
|
The tests added have passed previously in the PR CI and/or locally on a fork, right? |
|
Yes, the previous run for this PR was https://github.com/llvm/llvm-project/actions/runs/7522828209/job/20475369720. I believe security policy disallows it from running the newly added python 3.7 variant, but the full run can be seen in my fork https://github.com/linux4life798/llvm-project/actions/runs/7522828512. The only delta from those runs was removing the README.txt change. |
|
This is not working correctly. One of the jobs always gets cancelled, because the concurrency_group defined in llvm-project-tests.yml is the same for both jobs since it is keyed on the input projects. See https://github.com/llvm/llvm-project/actions/runs/7845680285 |
Yep, I saw that. I think the simplest fix is to push the python test matrix into the llvm-project-tests.yml. |
Fixes PR llvm#77219. Issue llvm#76664. Issue llvm#76601.
) Previously, #77219 added a `python_version` parameter for the Github Actions CI Ninja-based build tests. This is necessary to run component tests on different Python versions, as is currently done by the only user of this parameter, the [Libclang Python bindings test](https://github.com/llvm/llvm-project/blob/main/.github/workflows/libclang-python-tests.yml). The parameter is missing from the concurrency group of the workflow, meaning that starting the workflow with two different Python versions immediately cancels one of them, as pointed out by #77219 (comment). This change fixes that problem by making the Python version part of the concurrency group key, and removes the superfluous concurrency group from the calling workflow.
This enables the libclang python binding test to check
the oldest version of Python supported in addition
to the normal python version.
It is important to check this for issue #76664, since
many new mainstream python type annotation features
and best practices are not compatible with older
versions of python.
Additionally, frustration around ever increasing
platform dependencies and versions has been raised.
This will help ensure that python maintains reasonable
backwards compatibility.
Adding this additional build step will increase the
run time, but this should always be minimal, since
the additional libclang compilation should see 100%
cache hit rate.
Issue #76664.
Fixes #76601.