Skip to content

ci: reduced runs on draft PRs #5707

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

Merged
merged 6 commits into from
May 30, 2025
Merged

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented May 30, 2025

Description

This reduces the runs on draft PRs. I've also now listed all jobs explicitly, which should help both expand the variations we test, and reduce the repeated testing.

  • ci: reduce runs on draft PRs
  • ci: list all jobs explicitly
  • tests: support C++23 in tests
  • ci: rename cibw workflow file
  • tests: add PYBIND11_TEST_SMART_HOLDER to tests

Suggested changelog entry:

  • Support Windows C++20 and Linux C++23 in tests

Signed-off-by: Henry Schreiner <[email protected]>

Update ci.yml
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

I didn't look in detail, but I sure love the high-level direction.

I sent this prompt to ChatGPT:


I'm looking at this PR:

#5707

I only have very limited experience working on .github/workflows files.

High-level question: Is the file organization idiomatic?

What's on my mind: .github/workflows/ci.yml is basically a top-level workflow, while the new .github/workflows/reusable-standard.yml appears to be a akin to a helper function. My first instinct would be to put helpers into another directory, e.g. subdirectory.



It suggests adding a comment (I'd say not needed), and a slightly different filename:

reusable-ci-standard.yml

I like that, but what you have seems fine, too.

@henryiii
Copy link
Collaborator Author

It's all ci, so not sure ci makes much sense here. We should probably rename ci.yml to tests.yaml, then reusable-tests-standard isn't bad. (we should also standardize on yml vs. yaml, we've got one yaml). I've mostly seen these as reusable- prefixed and not in subdirectories (not sure what the requirements are, they might have to be top level for all I know).

@rwgk
Copy link
Collaborator

rwgk commented May 30, 2025

It's all ci, so not sure ci makes much sense here. We should probably rename ci.yml to tests.yaml, then reusable-tests-standard isn't bad. (we should also standardize on yml vs. yaml, we've got one yaml). I've mostly seen these as reusable- prefixed and not in subdirectories (not sure what the requirements are, they might have to be top level for all I know).

Sounds great to me, not in this PR I guess?

I think I'd pick build_and_test.yml as the new name.

@henryiii henryiii force-pushed the henryiii/ci/draft branch from 3ee50e8 to aaa8440 Compare May 30, 2025 17:00
@henryiii
Copy link
Collaborator Author

henryiii commented May 30, 2025

I like short1, especially if it was to be in the reusable filename too. How can you test without building, isn't that implied?

Currently working on getting the CI check names shorter, as you can't read anything past a certain length in the GitHub UI. It's adding both names together with the params added too...

Footnotes

  1. Within limits. classh is too short. :P

@rwgk
Copy link
Collaborator

rwgk commented May 30, 2025

How can you test without building, isn't that implied?

I'm totally fine if you want to keep it short.

Just to explain:

In cuda-python, we build first, separately, then run tests from another workflow, using the artifacts from the build step (for more platforms than we build with).

With that mindset: if I see "test.yml", the question pops up: where do we build?

@henryiii henryiii force-pushed the henryiii/ci/draft branch 2 times, most recently from 3a0ff62 to e8546b9 Compare May 30, 2025 18:53
@henryiii
Copy link
Collaborator Author

Pybind11 doesn't have anything to build; libraries with a built component this makes more sense than a header-only library where tests are what you are building; they are the only thing you are building.

@henryiii henryiii force-pushed the henryiii/ci/draft branch 2 times, most recently from de8826d to d62a648 Compare May 30, 2025 19:06
@henryiii henryiii requested a review from Copilot May 30, 2025 19:12
Copy link
Contributor

@Copilot Copilot AI left a 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 reduces CI runs on draft pull requests, lists all jobs explicitly, and enhances test support for C++23 and a new “smart holder” mode.

  • Add PYBIND11_TEST_SMART_HOLDER option and compile-time flags to test CMake configurations
  • Introduce a reusable CI workflow and adjust ci.yml/configure.yml to skip draft PRs and split standard jobs
  • Update tests to use C++23 features and alignas-based buffers, and rename workflows for clarity

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/test_embed/CMakeLists.txt Added conditional add_compile_definitions for smart-holder flag
tests/test_cmake_build/CMakeLists.txt Added conditional add_compile_definitions in build tests
tests/pure_cpp/smart_holder_poc_test.cpp Included <cstddef> and replaced aligned_storage with alignas raw-byte array
tests/CMakeLists.txt Introduced PYBIND11_TEST_SMART_HOLDER option and per-target flags
.github/workflows/reusable-standard.yml New reusable “standard test” workflow
.github/workflows/configure.yml Skip cmake job on draft PRs and expand pull_request types
.github/workflows/ci.yml Split standard into small/large, integrate reusable workflow, skip draft PRs
Comments suppressed due to low confidence (3)

tests/CMakeLists.txt:504

  • Consider using PRIVATE instead of PUBLIC for test-only compile definitions to avoid leaking flags into dependent targets.
target_compile_definitions(${target} PRIVATE -DPYBIND11_TEST_BOOST)

tests/CMakeLists.txt:583

  • [nitpick] These repeated compile definition calls could be consolidated (e.g., via a function or variable) to reduce duplication and simplify future updates.
target_compile_definitions(mod_per_interpreter_gil PUBLIC -DPYBIND11_RUN_TESTING_WITH_SMART_HOLDER_AS_DEFAULT_BUT_NEVER_USE_IN_PRODUCTION_PLEASE)

.github/workflows/ci.yml:212

  • The matrix variable args no longer exists; this placeholder should reference matrix.cmake-args or be removed to avoid an empty job name.
name: Configure C++11 ${{ matrix.args }}

@henryiii henryiii force-pushed the henryiii/ci/draft branch 2 times, most recently from 5116d25 to 30f2260 Compare May 30, 2025 19:22
@henryiii henryiii marked this pull request as ready for review May 30, 2025 19:44
@henryiii henryiii force-pushed the henryiii/ci/draft branch 2 times, most recently from 4c98587 to 20e1ecf Compare May 30, 2025 21:33
henryiii added 5 commits May 30, 2025 17:51
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>

style: pre-commit fixes
@henryiii henryiii force-pushed the henryiii/ci/draft branch from e68f204 to 6b83bf0 Compare May 30, 2025 21:51
@henryiii henryiii requested a review from Copilot May 30, 2025 22:26
Copy link
Contributor

@Copilot Copilot AI left a 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 reduces unnecessary CI runs on draft pull requests while also expanding the explicit listing of jobs and enabling C++23 and smart holder support in tests. Key changes include:

  • Adding conditions to GitHub Actions workflows to bypass execution on draft PRs.
  • Updating test cases and CMake configuration to support C++23 and the PYBIND11_TEST_SMART_HOLDER option.
  • Refining job matrices and configuration settings across multiple CI workflow files.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/test_pytypes.cpp Updated lambda parameters for better const-correctness and efficient iteration.
tests/test_embed/CMakeLists.txt Added compile definitions for smart holder testing based on PYBIND11_TEST_SMART_HOLDER.
tests/test_cmake_build/CMakeLists.txt Similarly added conditional compile definitions for smart holder support.
tests/pure_cpp/smart_holder_poc_test.cpp Replaced aligned storage with an alignas-based raw byte array for memory block allocation.
tests/CMakeLists.txt Introduced an option to toggle smart holder testing and applied corresponding definitions.
CMakePresets.json Added settings such as CMAKE_COLOR_DIAGNOSTICS to enhance diagnostic output.
.github/workflows/reusable-standard.yml Provided a reusable workflow for standard tests with updated job inputs.
.github/workflows/configure.yml Added conditions to only run configuration when a PR is not in draft state.
.github/workflows/ci.yml Modified job definitions and matrix entries to reduce CI runs on draft PRs and adjust flags.
Comments suppressed due to low confidence (1)

.github/workflows/ci.yml:171

  • For clarity and consistency, consider renaming the 'python' key in the 'inplace' job matrix to 'python-version', matching the naming used in other jobs.
inplace:
    matrix:
      include:
        - runs-on: ubuntu-latest
          python: '3.9'

@henryiii
Copy link
Collaborator Author

Okay, fixed tests in C++20 on Windows, C++23. Added color compile output.

For clarity and consistency, consider renaming the 'python' key in the 'inplace' job matrix to 'python-version', matching the naming used in other jobs.

I like this, but let's do it in a followup instead of rebuilding this one again.

@henryiii henryiii merged commit 33fb533 into pybind:master May 30, 2025
82 checks passed
@henryiii henryiii deleted the henryiii/ci/draft branch May 30, 2025 22:30
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label May 30, 2025
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jun 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants