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

bpo-46053: Fix OSS audio support on NetBSD #30065

Merged
merged 5 commits into from Apr 18, 2022
Merged

Conversation

Copy link
Contributor

@0-wiz-0 0-wiz-0 commented Dec 11, 2021

configure.ac Outdated
case $ac_sys_system/$ac_sys_release in
NetBSD*)
OSSAUDIOLIB="-lossaudio";;
*)
OSSAUDIOLIB="";;
esac
Copy link
Member

@tiran tiran Dec 13, 2021

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=""]
)

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Dec 13, 2021

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@0-wiz-0
Copy link
Contributor Author

@0-wiz-0 0-wiz-0 commented Dec 13, 2021

Thanks for the suggestions!

I have made the requested changes; please review again

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Dec 13, 2021

Thanks for making the requested changes!

@tiran: please review the changes made to this pull request.

@github-actions
Copy link

@github-actions github-actions bot commented Jan 13, 2022

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale label Jan 13, 2022
@0-wiz-0
Copy link
Contributor Author

@0-wiz-0 0-wiz-0 commented Jan 21, 2022

@serhiy-storchaka You were so kind to review my other NetBSD pull requests, can you please check this one as well?

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

I am not experienced with autotools. Should not AC_CHECK_LIB be used here?

@0-wiz-0
Copy link
Contributor Author

@0-wiz-0 0-wiz-0 commented Jan 21, 2022

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.

@github-actions github-actions bot removed the stale label Jan 22, 2022
@@ -0,0 +1 @@
Fix OSS audio support on NetBSD
Copy link
Member

@serhiy-storchaka serhiy-storchaka Feb 3, 2022

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.

@0-wiz-0
Copy link
Contributor Author

@0-wiz-0 0-wiz-0 commented Feb 6, 2022

Thanks for the review, @serhiy-storchaka
I've made the requested changes.

@hugovk
Copy link
Member

@hugovk hugovk commented Apr 12, 2022

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.

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Apr 12, 2022

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).

@hugovk
Copy link
Member

@hugovk hugovk commented Apr 12, 2022

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"], [],

@0-wiz-0
Copy link
Contributor Author

@0-wiz-0 0-wiz-0 commented Apr 15, 2022

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).

@hugovk
Copy link
Member

@hugovk hugovk commented Apr 16, 2022

If https://github.com/0-wiz-0/cpython/tree/ossaudio-v2 has what you want to merge, two options:

  • Create a new PR from that branch, close this

  • Reset this branch to that one, and force push to replace this:

# 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

@0-wiz-0
Copy link
Contributor Author

@0-wiz-0 0-wiz-0 commented Apr 18, 2022

Thanks for the instructions, @hugovk ! I've just force-pushed to the ossaudio branch.

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Apr 18, 2022

@serhiy-storchaka, you previously approved this PR — reckon it can now be merged? :)

@serhiy-storchaka serhiy-storchaka merged commit 2e7e3c4 into python:main Apr 18, 2022
13 checks passed
@0-wiz-0 0-wiz-0 mannequin mentioned this pull request Apr 18, 2022
@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Apr 18, 2022

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.

@0-wiz-0
Copy link
Contributor Author

@0-wiz-0 0-wiz-0 commented Apr 18, 2022

Thank you for merging this, @serhiy-storchaka !

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Apr 20, 2022

Thanks @0-wiz-0 for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒🤖

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Apr 20, 2022

Thanks @0-wiz-0 for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒🤖

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Apr 20, 2022

Sorry, @0-wiz-0 and @serhiy-storchaka, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 2e7e3c4c109928870c1e33d8af36b78e92895594 3.9

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Apr 20, 2022

Sorry @0-wiz-0 and @serhiy-storchaka, I had trouble checking out the 3.10 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 2e7e3c4c109928870c1e33d8af36b78e92895594 3.10

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Apr 20, 2022

@0-wiz-0 do you think it is a good idea of backporting this change?

@0-wiz-0
Copy link
Contributor Author

@0-wiz-0 0-wiz-0 commented Apr 20, 2022

@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.

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Apr 20, 2022

I mean, do you want to spent your time on this? 😉 I'll approve it immediately.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants