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
bpo-46053: Fix OSS audio support on NetBSD #30065
Conversation
configure.ac
Outdated
case $ac_sys_system/$ac_sys_release in | ||
NetBSD*) | ||
OSSAUDIOLIB="-lossaudio";; | ||
*) | ||
OSSAUDIOLIB="";; | ||
esac |
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.
We keep feature checks and module definitions separate. Between LIBNSL
and LIBSQLITE3
looks like a nice spot. We also prefer m4 macros for new code:
AS_CASE([$ac_sys_system],
[NetBSD*], [OSSAUDIODEV_LIBS="-lossaudio"],
[OSSAUDIODEV_LIBS=""]
)
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Thanks for the suggestions! I have made the requested changes; please review again |
Thanks for making the requested changes! @tiran: please review the changes made to this pull request. |
This PR is stale because it has been open for 30 days with no activity. |
@serhiy-storchaka You were so kind to review my other NetBSD pull requests, can you please check this one as well? |
I am not experienced with autotools. Should not AC_CHECK_LIB be used here?
AFAIK only NetBSD provides libossaudio, and all versions of it (but definitely all supported versions) provide the library. That's why I thought it'd be easier just to add it this way. |
@@ -0,0 +1 @@ | |||
Fix OSS audio support on NetBSD |
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 should be in the "Library" section, not in "Core and Builtins". And please add a sentence-ending period.
Otherwise LGTM.
Thanks for the review, @serhiy-storchaka |
Note the ossaudiodev module is deprecated in 3.11 and set for removal in 3.13. See PEP 594 – Removing dead batteries from the standard library and #91217. |
There is also now a merge conflict (I don't know how large the merge conflict is). |
Here's the conflict: diff --cc configure.ac
index 7d194f263e,6a460cd94c..0000000000
--- a/configure.ac
+++ b/configure.ac
@@@ -6409,8 -6789,8 +6795,13 @@@ PY_STDLIB_MOD([_socket]
dnl platform specific extensions
PY_STDLIB_MOD([grp], [], [test "$ac_cv_func_getgrgid" = yes -o "$ac_cv_func_getgrgid_r" = yes])
PY_STDLIB_MOD([ossaudiodev],
++<<<<<<< HEAD
+ [], [test "$ac_cv_header_linux_soundcard_h" = yes -o "$ac_cv_header_sys_soundcard_h" = yes],
+ [], [$OSSAUDIODEV_LIBS])
++=======
+ [], [test "$ac_cv_header_linux_soundcard_h" = yes -o "$ac_cv_header_sys_soundcard_h" = yes])
+ PY_STDLIB_MOD([pwd], [], [test "$ac_cv_func_getpwuid" = yes -o "$ac_cv_func_getpwuid_r" = yes])
++>>>>>>> upstream/main
PY_STDLIB_MOD([resource], [], [test "$ac_cv_header_sys_resource_h" = yes])
PY_STDLIB_MOD([_scproxy],
[test "$ac_sys_system" = "Darwin"], [], |
Some more years will pass before this code is really removed from python, so I'd like to see this merged. I tried fixing the merge conflict on the ossaudio branch but my git-foo left me, the merged commits are now on https://github.com/0-wiz-0/cpython/tree/ossaudio-v2 instead (but it's straightforward - on main, a line was added that is in the context of the change I made, the context just needs to be adapted). |
If https://github.com/0-wiz-0/cpython/tree/ossaudio-v2 has what you want to merge, two options:
# Start from this branch
git checkout ossaudio
# Replace the contents of this branch with ossaudio-v2
git reset --hard ossaudio-v2
# Now because we've rewritten the history of this branch,
# we need to force push it
git push --force |
Thanks for the instructions, @hugovk ! I've just force-pushed to the ossaudio branch. |
@serhiy-storchaka, you previously approved this PR — reckon it can now be merged? :) |
I think that it is worth to to continue to fix bugs in deprecated modules. Especially if the solution is already ready. Requests for new features will be rejected of course. |
Thank you for merging this, @serhiy-storchaka ! |
Thanks @0-wiz-0 for the PR, and @serhiy-storchaka for merging it |
Thanks @0-wiz-0 for the PR, and @serhiy-storchaka for merging it |
Sorry, @0-wiz-0 and @serhiy-storchaka, I could not cleanly backport this to |
Sorry @0-wiz-0 and @serhiy-storchaka, I had trouble checking out the |
@0-wiz-0 do you think it is a good idea of backporting this change? |
@serhiy-storchaka if it's easy, I'd appreciate it (I think it should be easy) but if you don't want to spend the time on this, I understand and it's fine with me. |
I mean, do you want to spent your time on this? It is okay if you do not do it. I can make the backport myself, although I am not enough motivated to test it on NetBSD. |
https://bugs.python.org/issue46053