Skip to content
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

Connecting PYBIND11_INTERNALS_VERSION to PYBIND11_USE_SMART_HOLDER_AS_DEFAULT. #2939

Merged
merged 3 commits into from
Apr 16, 2021

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Apr 6, 2021

Extensions built with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT defined are incompatible with extensions built against master, or PYBIND11_USE_SMART_HOLDER_AS_DEFAULT undefined, therefore PYBIND11_INTERNALS_VERSION needs to be different. The approach chosen here is to add 1000 to the PYBIND11_INTERNALS_VERSION on master.

@rwgk rwgk requested a review from EricCousineau-TRI April 6, 2021 01:51
@rwgk
Copy link
Collaborator Author

rwgk commented Apr 6, 2021

@rhaschke the github web UI won't let me add you as a reviewer, I have no idea why. Tagging you here instead.

@rwgk
Copy link
Collaborator Author

rwgk commented Apr 6, 2021

#2879 is used for testing with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT defined.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

I would expect a user to enable PYBIND11_USE_SMART_HOLDER_AS_DEFAULT via a compiler define on the cmdline. I followed this approach in #2930, which enables both variants as two different workflows: CI and CI SH.

@@ -10,6 +10,7 @@
#pragma once

#include "../pytypes.h"
#include "smart_holder_sfinae_hooks_only.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is your approach to define PYBIND11_USE_SMART_HOLDER_AS_DEFAULT?
Why don't you use a compiler definition?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Initially mainly to have an easy way to run the CI with smart_holder as default. But then I thought it's good to keep as an option, to give people an easy way to experiment using smart_holder as default without having to go into their cmake/CI/IDE/whatever they may have. As-is, it works both ways. I'm reluctant to take one option away, especially because the cost is just a few extra lines in smart_holder_sfinae_hooks_only.h. With pytypes.h included first here, everything else is included already anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't asking to remove that option. I was just wondering. For CI, I suggest to use the cmake argument option to allow both checks to run on the same code base. I'm still struggling with some issues in #2930 though... But, I'm optimistic 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, good, thanks! And yes, it'll be awesome to trigger all testing with just one PR.

@rhaschke
Copy link
Contributor

rhaschke commented Apr 7, 2021

The github web UI won't let me add you as a reviewer, I have no idea why.

You can only tag people as a reviewer that are part of the maintainer team.

@rwgk rwgk force-pushed the sh_default_internals_bump branch from d353b45 to 4b9bc48 Compare April 16, 2021 13:38
@rwgk rwgk merged commit d368b72 into pybind:smart_holder Apr 16, 2021
@rwgk rwgk deleted the sh_default_internals_bump branch April 16, 2021 14:15
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Apr 16, 2021
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Apr 16, 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
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Sep 20, 2021
rwgk pushed a commit that referenced this pull request Sep 20, 2021
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