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

Use different capsule names with and without -builtin #2238

Merged

Conversation

eltoder
Copy link
Contributor

@eltoder eltoder commented Mar 18, 2022

Types generated with and without -builtin are not compatible. Mixing
them in a common type list leads to crashes. Avoid this by using
different capsule names: "type_pointer_capsule" without -builtin and
"type_pointer_capsule_builtin" with.

See #1684

@eltoder eltoder force-pushed the feature/python-builtin-separate-runtime-data branch 2 times, most recently from 659941d to 07b6137 Compare March 18, 2022 15:59
Types generated with and without -builtin are not compatible. Mixing
them in a common type list leads to crashes. Avoid this by using
different capsule names: "type_pointer_capsule" without -builtin and
"type_pointer_capsule_builtin" with.

See swig#1684
@eltoder eltoder force-pushed the feature/python-builtin-separate-runtime-data branch from 07b6137 to f733efd Compare March 18, 2022 17:44
@ojwb ojwb added the Python label Mar 18, 2022
@wsfulton wsfulton merged commit f733efd into swig:master Mar 27, 2022
@wsfulton
Copy link
Member

Thanks for the patch @eltoder. Kudos for squeezing this into the test-suite and forcing/overriding when -builtin is used! I would have said the test framework was not designed for this type of flexibility. Although clever, the implementation for the test is not what I'd call particularly maintainable nor easy to follow, which I think is important, so I've put in an alternative, which is not ideal either, but I feel is at least more maintainable. See f2dd436 which enabled another improvement in c5f2097.

@eltoder eltoder deleted the feature/python-builtin-separate-runtime-data branch March 30, 2022 19:41
@eltoder
Copy link
Contributor Author

eltoder commented Mar 30, 2022

Thank you for merging @wsfulton. Yes, the test suite did not cooperate, but it felt bad to fix something very subtle without any tests. I did not want to spend a lot of time refactoring, since I wasn't sure if there was any interest in merging. I considered an approach similar to f2dd436. My approach seemed more general and not too messy, until I realized that I'm limited to sh syntax and cannot use bash's indirect variables. I'm fine with either option. Thank you for polishing the test, hopefully this will not regress in the future.

PatTheMav added a commit to PatTheMav/obs-studio that referenced this pull request Jul 13, 2022
Fixes issues introduced by swig/swig#2238. As
obs-scripting is not compatible with the SWIGPYTHON_BUILTIN, obspython
needs to disable this feature.
PatTheMav added a commit to PatTheMav/obs-studio that referenced this pull request Jul 13, 2022
Fixes issues introduced by swig/swig#2238. As
obs-scripting is not compatible with the SWIGPYTHON_BUILTIN, obspython
needs to disable this feature.
RytoEX pushed a commit to obsproject/obs-studio that referenced this pull request Jul 13, 2022
Fixes issues introduced by swig/swig#2238. As
obs-scripting is not compatible with the SWIGPYTHON_BUILTIN, obspython
needs to disable this feature.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants