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

fix: PYBIND11_INTERNALS_VERSION bump for MSVC #4819

Merged
merged 1 commit into from
Aug 30, 2023

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Aug 26, 2023

Description

Piggy-backed on PR #4779. See comments there.

Suggested changelog entry:

Also bump ``PYBIND11_INTERNALS_VERSION`` for MSVC, which unlocks two new features without creating additional incompatibilities.

@henryiii
Copy link
Collaborator

Since we have to1 bump this for MSVC anyway, would it make sense to go head and bump everything to 5? Or would it be better to use this as a testing ground, to see how many Windows users are affected by a bump?

Footnotes

  1. Bump meaning these are no longer cross-compatible anyway.

@rwgk
Copy link
Collaborator Author

rwgk commented Aug 30, 2023

Since we have to1 bump this for MSVC anyway, would it make sense to go head and bump everything to 5?

Good question, that crossed my mind, too. I think it would be much more straightforward to say "2.12 unconditionally uses 5". What I don't know: how much trouble will that cause for the community at large? Who feels strongly about making the call one way or another? I'm fine either way, happier with using 5 universally.

I do believe though that we should merge this PR straightaway, because holding it back while we make up our minds doesn't do any good for anyone.

@rwgk rwgk marked this pull request as ready for review August 30, 2023 16:36
@henryiii
Copy link
Collaborator

What I don't know: how much trouble will that cause for the community at large?

I don't know. I don't really have a feel for this. I know PyTorch uses this, but they disable our protections for cross-compilers, since they want to mix and match compilers. CUDA users sometimes disable those too, since they want to mix and match a CUDA compiler with a host compiler.

I do believe though that we should merge this PR straightaway, because holding it back while we make up our minds doesn't do any good for anyone.

Yes, but it was draft. :)

@rwgk
Copy link
Collaborator Author

rwgk commented Aug 30, 2023

Sorry I simply forgot to mark it as ready for review before.

Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

This already is incompatible with previous versions, due to the previous PR. So an easy change.

@rwgk rwgk merged commit 1adac5a into pybind:master Aug 30, 2023
82 checks passed
@rwgk rwgk deleted the msvc_internals_v5 branch August 30, 2023 17:05
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Aug 30, 2023
@henryiii henryiii changed the title PYBIND11_INTERNALS_VERSION bump for MSVC fix: PYBIND11_INTERNALS_VERSION bump for MSVC Nov 15, 2023
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Mar 27, 2024
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