ci: reduced runs on draft PRs#5707
Conversation
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> Update ci.yml
rwgk
left a comment
There was a problem hiding this comment.
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:
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.
|
It's all ci, so not sure |
Sounds great to me, not in this PR I guess? I think I'd pick |
3ee50e8 to
aaa8440
Compare
|
I like short1, especially if it was to be in the 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
|
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? |
3a0ff62 to
e8546b9
Compare
|
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. |
de8826d to
d62a648
Compare
There was a problem hiding this comment.
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_HOLDERoption and compile-time flags to test CMake configurations - Introduce a reusable CI workflow and adjust
ci.yml/configure.ymlto 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
argsno longer exists; this placeholder should referencematrix.cmake-argsor be removed to avoid an empty job name.
name: Configure C++11 ${{ matrix.args }}
5116d25 to
30f2260
Compare
4c98587 to
20e1ecf
Compare
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> style: pre-commit fixes
e68f204 to
6b83bf0
Compare
There was a problem hiding this comment.
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'
|
Okay, fixed tests in C++20 on Windows, C++23. Added color compile output.
I like this, but let's do it in a followup instead of rebuilding this one again. |
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.
PYBIND11_TEST_SMART_HOLDERto testsSuggested changelog entry: