-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Conversation
Signed-off-by: Henry Schreiner <[email protected]> Update ci.yml
There was a problem hiding this 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:
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.
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 referencematrix.cmake-args
or 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 <[email protected]>
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
e68f204
to
6b83bf0
Compare
There was a problem hiding this 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'
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_HOLDER
to testsSuggested changelog entry: