Skip to content

CI for smart holders as default #2930

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

Closed
wants to merge 1 commit into from

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Apr 1, 2021

This adds another CI workflow enabling PYBIND11_USE_SMART_HOLDER_AS_DEFAULT

@rhaschke rhaschke changed the base branch from master to smart_holder April 1, 2021 11:52
@rhaschke rhaschke force-pushed the smart-holder-ci branch 2 times, most recently from 4876b24 to 88f3474 Compare April 7, 2021 11:26
@rhaschke
Copy link
Contributor Author

rhaschke commented Apr 7, 2021

cmake implicitly sets CMAKE_CXX_FLAGS="/GR /EHsc" for MSVC. Manually setting CMAKE_CXX_FLAGS overrides this default and thus requires adding those flags explicitly.

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 really like that it's so easy, but I'm also really worried about introducing two copies of something so substantial. @henryiii, is there an obvious way to avoid the duplication?

Naive idea: have a small patch file somewhere else, and a bash script that applies that patch to generate this file, with a big comment at the top: "This is an auto-generated file, do not edit." And another comment with instructions how to run the bash script. — But I hope Henry knows of something more organized.

@rhaschke
Copy link
Contributor Author

rhaschke commented Apr 7, 2021

I tried to merge the new SH builds into the existing CI workflow before, but that ended up in a mess, because the workflow defines its jobs using matrix + includes. Doing all jobs with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT enabled required way more changes to the CI workflow. With the present approach, it should be easier to keep the CI and CI SH workflows in sync.

rwgk added a commit to rwgk/pybind11 that referenced this pull request Apr 20, 2021
rwgk added a commit that referenced this pull request Apr 20, 2021
* Adding ci_sh_def.yml, based on PR #2930.

* Adding exludes for .patch files.
@rwgk
Copy link
Collaborator

rwgk commented Apr 20, 2021

The work here was included in #2963. Closing. Thanks Robert!

@rwgk rwgk closed this Apr 20, 2021
@EricCousineau-TRI EricCousineau-TRI added the smart holder See: https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst label Apr 26, 2021
@rhaschke rhaschke deleted the smart-holder-ci branch May 19, 2021 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
smart holder See: https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants