-
-
Notifications
You must be signed in to change notification settings - Fork 9k
obs-scripting: Replace deprecated PySys_SetArgv #13014
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
base: master
Are you sure you want to change the base?
obs-scripting: Replace deprecated PySys_SetArgv #13014
Conversation
RytoEX
left a comment
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.
Commit message nits.
obs-scripting: Fix depreciated Python interpreter's command line arguments configuration
This commit title is too long. Use:
obs-scripting: Replace deprecated PySys_SetArgv
PySys_SetArgv has been deprecated since Python 3.11 and will be removed
in Python 3.15. Use PyConfig_InitPythonConfig instead, which was added
in Python 3.8.
References:
* https://docs.python.org/3.14/c-api/init.html#c.PySys_SetArgv
PR title nit:
depreciated -> deprecated
Or use the commit message title I have provided above.
Do not leave fixup commits in the PR. Do not use git merge to update branch history. Use git rebase to fix all of these issues.
Apologies. I am fairly new to GitHub |
9b0dcda to
37b21af
Compare
ff48d02 to
80031d0
Compare
|
|
||
| /* ---------------------------------------------- */ | ||
| /* Must set arguments for guis to work */ | ||
| //PySys_SetArgv is deprecated in 3.8+, so use the new initialization API if detected |
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.
This is incorrect. Per my original review:
PySys_SetArgv has been deprecated since Python 3.11 and will be removed
in Python 3.15. Use PyConfig_InitPythonConfig instead, which was added
in Python 3.8.
| // Classic 3.6/3.7 path | ||
| Py_Initialize(); | ||
| wchar_t *argv[] = {L"", NULL}; | ||
| PySys_SetArgv(0, argv); // or PySys_SetArgvEx if you have 3.7+ |
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.
It's not clear that this is even necessary. We only support building against Python 3.8-3.12. We need to determine if this code is affected by the user's Python runtime at all.
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.
This was added in Python 3.8, which means explicitly dropping support for Python 3.6 and 3.7, which cannot be done until a major release.
I'm confused by this comment then. I interpreted this as you saying that the new API was introduced in 3.8 but my code still needs to support 3.6 and 7. Are you now saying to use the new API and not worry about backwards compatibility?
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.
No. I'm saying that in this PR as it was originally submitted, it explicitly used Python 3.8 only code. For compile time dependencies, we only support Python 3.8-3.12 anyway, so this does not matter. For run time dependencies, we in theory support Python 3.6+, so it would need to be confirmed if a user can still load/use Python 3.6/3.7 with the original changes. If it is the case that users cannot use Python 3.6/3.7 at runtime with the original changes, then my comment was a note to mergers that "this cannot be merged until we're in a merge window for a major release as this would be a breaking change".
The open question is: does the code in this PR as it was originally or as it is now affect compile-time Python, run-time Python, or both?
Description
Updated the
PySys_SetArgv(argc, argv);line to have a more modern way of setting the arguments.Motivation and Context
PySys_SetArgvis depreciated and has already been removed in Python 3.13How Has This Been Tested?
Ran in Visual Studio code
Operating System: Kubuntu 25.10
KDE Plasma Version: 6.5.4
KDE Frameworks Version: 6.20.0
Qt Version: 6.9.2
Kernel Version: 6.18.1-061801-generic (64-bit)
Graphics Platform: Wayland
Processors: 16 × AMD Ryzen 7 5800X3D 8-Core Processor
Memory: 32 GiB of RAM (31.2 GiB usable)
Graphics Processor: AMD Radeon Graphics
Manufacturer: ASUS
Types of changes
Checklist: