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

Jump to bottom

bpo-30512: add CAN Socket support for NetBSD #30066

Merged
merged 8 commits into from Jan 21, 2022
Merged

bpo-30512: add CAN Socket support for NetBSD #30066

merged 8 commits into from Jan 21, 2022

Conversation

0-wiz-0
Copy link
Contributor

@0-wiz-0 0-wiz-0 commented Dec 11, 2021 •
edited by bedevere-bot

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

@taleinat
Copy link
Contributor

taleinat commented Jan 8, 2022

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.

Indeed, it would be better for that to be a separate PR. Thanks!

@taleinat taleinat requested a review from serhiy-storchaka Jan 8, 2022
@taleinat
Copy link
Contributor

taleinat commented Jan 8, 2022

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

@0-wiz-0 0-wiz-0 mentioned this pull request Jan 8, 2022
Merged
@0-wiz-0
Copy link
Contributor Author

0-wiz-0 commented Jan 8, 2022

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

@taleinat
Copy link
Contributor

taleinat commented Jan 9, 2022

@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
serhiy-storchaka requested changes Jan 10, 2022
View changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a 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.

Modules/socketmodule.c Outdated
PyModule_AddIntMacro(m, CAN_ERR_MASK);

PyModule_AddIntMacro(m, CAN_RAW_FILTER);
/* PyModule_AddIntMacro(m, CAN_RAW_ERR_FILTER); */
Copy link
Member

@serhiy-storchaka serhiy-storchaka Jan 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Modules/socketmodule.c Outdated
@@ -7790,6 +7790,20 @@ PyInit__socket(void)

PyModule_AddIntMacro(m, J1939_FILTER_MAX);
#endif
#ifdef HAVE_NETCAN_CAN_H
Copy link
Member

@serhiy-storchaka serhiy-storchaka Jan 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not merge this with blocks for HAVE_LINUX_CAN_H and HAVE_LINUX_CAN_RAW_H?

Misc/NEWS.d/next/Core and Builtins/2021-12-12-00-49-19.bpo-30512.nU9E9V.rst Outdated
@@ -0,0 +1 @@
Add CAN Socket support for NetBSD
Copy link
Member

@serhiy-storchaka serhiy-storchaka Jan 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Add CAN Socket support for NetBSD
Add CAN Socket support for NetBSD.

@bedevere-bot
Copy link

bedevere-bot commented Jan 10, 2022

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.

And if you don't make the requested changes, you will be put in the comfy chair!

@0-wiz-0
Copy link
Contributor Author

0-wiz-0 commented Jan 13, 2022

Thanks for the review, @serhiy-storchaka !
I've tried addressing all your comments. I'm not sure what you mean by

Add also an entry in the What's New file.

since I thought that's what the blurb is used for.

@0-wiz-0
Copy link
Contributor Author

0-wiz-0 commented Jan 13, 2022

I have made the requested changes; please review again

@bedevere-bot
Copy link

bedevere-bot commented Jan 13, 2022

Thanks for making the requested changes!

@serhiy-storchaka: please review the changes made to this pull request.

serhiy-storchaka
serhiy-storchaka reviewed Jan 18, 2022
View changes
Modules/socketmodule.c Outdated
PyModule_AddIntMacro(m, CAN_RAW_FILTER);
#if defined(CAN_RAW_ERR_FILTER)
Copy link
Member

@serhiy-storchaka serhiy-storchaka Jan 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#if defined(CAN_RAW_ERR_FILTER)
#ifdef CAN_RAW_ERR_FILTER

For consistency.

Copy link
Contributor Author

@0-wiz-0 0-wiz-0 Jan 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed as suggested. (A couple lines above that, it's #if defined - the file is not very consistent :) ).

Doc/library/socket.rst Show resolved Hide resolved
@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Jan 18, 2022

Blurb is used for creating full Changelog which includes all user-visible changed as they are made, including bugfixes.

There is a separate file Doc/whatsnew/3.11.rst. It is structured and only includes information about new features, deprecations and removals. In section "Improved Modules" add a subsection for "socket" and a description of the feature. E.g.:

socket
------
* Add CAN Socket support for NetBSD.
  (Contributed by Thomas Klausner in :issue:`30512`.)

@0-wiz-0
Copy link
Contributor Author

0-wiz-0 commented Jan 18, 2022

I've added a Whatsnew entry as suggested - thanks for the explanation & review!

@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Jan 21, 2022
@serhiy-storchaka serhiy-storchaka merged commit 40fcd16 into python:main Jan 21, 2022
12 checks passed
@erlend-aasland erlend-aasland mentioned this pull request May 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka

Assignees
No one assigned
Labels
type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@0-wiz-0 @taleinat @bedevere-bot @serhiy-storchaka @the-knights-who-say-ni