bpo-30512: add CAN Socket support for NetBSD#30066
Conversation
Indeed, it would be better for that to be a separate PR. Thanks! |
|
@serhiy-storchaka, since according to the b.p.o. issue you've worked on this in the past, it would be good to get your review of this! |
This reverts commit 41c4574.
|
@taleinat I've made a separate pull request for the configure script fix, but I don't know how to mark it as "skip news", please advise. Thanks! |
Thanks for doing that! Don't worry about the skip-news labels; only triagers and core devs can add those, but a mention in a comment is great 👍 |
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Please update the documentation for the socket module. Update availability directives, add versionchanged directives.
Add also an entry in the What's New file.
| PyModule_AddIntMacro(m, CAN_ERR_MASK); | ||
|
|
||
| PyModule_AddIntMacro(m, CAN_RAW_FILTER); | ||
| /* PyModule_AddIntMacro(m, CAN_RAW_ERR_FILTER); */ |
There was a problem hiding this comment.
Is it not defined on NetBSD? Then after merging with the HAVE_LINUX_CAN_RAW_H case you can add #ifdef CAN_RAW_ERR_FILTER.
|
|
||
| PyModule_AddIntMacro(m, J1939_FILTER_MAX); | ||
| #endif | ||
| #ifdef HAVE_NETCAN_CAN_H |
There was a problem hiding this comment.
Why not merge this with blocks for HAVE_LINUX_CAN_H and HAVE_LINUX_CAN_RAW_H?
| @@ -0,0 +1 @@ | |||
| Add CAN Socket support for NetBSD | |||
There was a problem hiding this comment.
| Add CAN Socket support for NetBSD | |
| Add CAN Socket support for NetBSD. |
|
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 And if you don't make the requested changes, you will be put in the comfy chair! |
|
Thanks for the review, @serhiy-storchaka ! since I thought that's what the blurb is used for. |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @serhiy-storchaka: please review the changes made to this pull request. |
| #ifdef HAVE_LINUX_CAN_RAW_H | ||
| #if defined(HAVE_LINUX_CAN_RAW_H) || defined(HAVE_NETCAN_CAN_H) | ||
| PyModule_AddIntMacro(m, CAN_RAW_FILTER); | ||
| #if defined(CAN_RAW_ERR_FILTER) |
There was a problem hiding this comment.
| #if defined(CAN_RAW_ERR_FILTER) | |
| #ifdef CAN_RAW_ERR_FILTER |
For consistency.
There was a problem hiding this comment.
Changed as suggested. (A couple lines above that, it's #if defined - the file is not very consistent :) ).
|
Blurb is used for creating full Changelog which includes all user-visible changed as they are made, including bugfixes. There is a separate file |
|
I've added a Whatsnew entry as suggested - thanks for the explanation & review! |
This also fixes a small unrelated bug in the configure script - test(1) only supports "=" as a comparison operator, "==" is a bash extension. If this should be a separate pull request, please let me know.
https://bugs.python.org/issue30512