Skip to content

GH-144552: Clean up tail-call.yml CI#144553

Open
savannahostrowski wants to merge 17 commits intopython:mainfrom
savannahostrowski:refactor-tailcall-yml
Open

GH-144552: Clean up tail-call.yml CI#144553
savannahostrowski wants to merge 17 commits intopython:mainfrom
savannahostrowski:refactor-tailcall-yml

Conversation

@savannahostrowski
Copy link
Member

@savannahostrowski savannahostrowski commented Feb 6, 2026

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

@hugovk
Copy link
Member

hugovk commented Feb 6, 2026

Is this clang-20 also the LLVM number? (in three places)

-CC=clang-20 ...
+CC=clang-${{ matrix.llvm }} ...

@hugovk
Copy link
Member

hugovk commented Feb 6, 2026

Is this clang-20 also the LLVM number? (in three places)

-CC=clang-20 ...
+CC=clang-${{ matrix.llvm }} ...

Included in savannahostrowski#15, which also combines the Linux steps.

@hugovk
Copy link
Member

hugovk commented Feb 6, 2026

savannahostrowski#16 combines two Windows steps.

That will leave us with these Windows jobs:

#          - target: i686-pc-windows-msvc/msvc
#            architecture: Win32
#            runner: windows-2022
          - target: x86_64-pc-windows-msvc/msvc
            architecture: x64
            runner: windows-2025-vs2026
            build_flags: ""
            run_tests: true
          - target: x86_64-pc-windows-msvc/msvc-free-threading
            architecture: x64
            runner: windows-2025-vs2026
            build_flags: --disable-gil
            run_tests: false
#          - target: aarch64-pc-windows-msvc/msvc
#            architecture: ARM64
#            runner: windows-2022

Note Win32 and ARM64 are commented out, and have been since initial commit exactly one year ago! 🎂 cb640b6

Only Windows x64 architecture is run. I suggest we delete the commented code, we can re-add it when we're ready.

And after combining two of the three Windows steps into one, we have two remaining:

      - name: Native Windows MSVC (release)
        if: runner.os == 'Windows' && matrix.architecture != 'ARM64'
        shell: pwsh
        run: |
          $env:PlatformToolset = "v145"
          ./PCbuild/build.bat --tail-call-interp ${{ matrix.build_flags }} -c Release -p ${{ matrix.architecture }}
          if ("${{ matrix.run_tests }}" -eq "true") {
            ./PCbuild/rt.bat -p ${{ matrix.architecture }} -q --multiprocess 0 --timeout 4500 --verbose2 --verbose3
          }

      # No tests (yet):
      - name: Emulated Windows Clang (release)
        if: runner.os == 'Windows' && matrix.architecture == 'ARM64'
        shell: pwsh
        run: |
          choco install llvm --allow-downgrade --no-progress --version ${{ matrix.llvm }}.1.0
          $env:PlatformToolset = "clangcl"
          $env:LLVMToolsVersion = "${{ matrix.llvm }}.1.0"
          $env:LLVMInstallDir = "C:\Program Files\LLVM"
          ./PCbuild/build.bat --tail-call-interp -p ${{ matrix.architecture }}

I don't think that second one has ever been run? I suggest we delete that too. And when we're ready, we can re-add as needed. And that might be by combining into the first Windows step instead of a standalone one.

The && matrix.architecture != 'ARM64' guard in this first step is also redundant and can be deleted.

@savannahostrowski
Copy link
Member Author

Thanks for the pulls @hugovk! I was initially trying to be somewhat conservative but I'm inclined to remove the unused job and commented out platforms as well. I think it'll be obvious enough if/when we need to add them back in.

# architecture: ARM64
# runner: windows-2022
build_flags: --disable-gil
run_tests: false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Linux free-threading the tests are run. Shall we save CI resources there, too, or play safe here and run the tests?

@hugovk
Copy link
Member

hugovk commented Feb 7, 2026

savannahostrowski#17 removes the duplicate target list, and then I have one more suggestion that builds on top of that.

@savannahostrowski
Copy link
Member Author

We can use YAML anchors/aliases here to make things even cleaner but we need to bump actionlint first. See #144576

@hugovk
Copy link
Member

hugovk commented Feb 8, 2026

Okay, last suggestion! savannahostrowski#18

@webknjaz
Copy link
Member

webknjaz commented Feb 8, 2026

Ideally, this should be converted into a reusable module per convention I introduced a few years ago. Then the paths can go into the change detection and the jobs can be taken into account in alls-green.

windows:
name: ${{ matrix.target }}
runs-on: ${{ matrix.runner }}
timeout-minutes: 90
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need entire 90 mins here? Looking at https://github.com/python/cpython/actions/runs/21803290596, the jobs take under 15 mins? So setting this to 20 should be enough to cover occasional network flakiness while still catching the stuck state early enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants