bpo-30512: add CAN Socket support for NetBSD by 0-wiz-0 · Pull Request #30066 · python/cpython

0-wiz-0 · 2021-12-11T23:51:17Z

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 · 2022-01-08T20:09:58Z

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 · 2022-01-08T20:10:45Z

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

0-wiz-0 · 2022-01-08T21:53:29Z

@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 · 2022-01-09T20:42:36Z

@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

Please update the documentation for the socket module. Update availability directives, add versionchanged directives.

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

serhiy-storchaka · 2022-01-10T09:41:24Z

Modules/socketmodule.c

+    PyModule_AddIntMacro(m, CAN_ERR_MASK);
+
+    PyModule_AddIntMacro(m, CAN_RAW_FILTER);
+    /* PyModule_AddIntMacro(m, CAN_RAW_ERR_FILTER); */


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.

serhiy-storchaka · 2022-01-10T09:41:31Z

Modules/socketmodule.c


    PyModule_AddIntMacro(m, J1939_FILTER_MAX);
 #endif
+#ifdef HAVE_NETCAN_CAN_H


Why not merge this with blocks for HAVE_LINUX_CAN_H and HAVE_LINUX_CAN_RAW_H?

serhiy-storchaka · 2022-01-10T09:41:46Z

Misc/NEWS.d/next/Core and Builtins/2021-12-12-00-49-19.bpo-30512.nU9E9V.rst

@@ -0,0 +1 @@
+Add CAN Socket support for NetBSD


Suggested change

Add CAN Socket support for NetBSD

Add CAN Socket support for NetBSD.

bedevere-bot · 2022-01-10T09:47:00Z

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 · 2022-01-13T16:56:56Z

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 · 2022-01-13T16:57:17Z

I have made the requested changes; please review again

bedevere-bot · 2022-01-13T16:57:20Z

Thanks for making the requested changes!

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

serhiy-storchaka · 2022-01-18T19:57:19Z

Modules/socketmodule.c

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


Suggested change

#if defined(CAN_RAW_ERR_FILTER)

#ifdef CAN_RAW_ERR_FILTER

For consistency.

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

Doc/library/socket.rst

serhiy-storchaka · 2022-01-18T20:15:47Z

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 · 2022-01-18T22:55:01Z

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

0-wiz-0 added 2 commits December 12, 2021 00:49

bpo-30512: add CAN Socket support for NetBSD

494b2d2

Fix unportable test(1) operator in configure script.

41c4574

the-knights-who-say-ni added the CLA signed label Dec 11, 2021

bedevere-bot added the awaiting review label Dec 11, 2021

taleinat requested a review from serhiy-storchaka January 8, 2022 20:10

Revert "Fix unportable test(1) operator in configure script."

c2ac8c1

This reverts commit 41c4574.

0-wiz-0 mentioned this pull request Jan 8, 2022

bpo-34602: Fix unportable test(1) operator in configure script #30490

Merged

serhiy-storchaka requested changes Jan 10, 2022

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jan 10, 2022

0-wiz-0 added 3 commits January 13, 2022 17:54

Merge Linux and NetBSD cases.

a63be0e

End sentence with dot.

c4441ce

Update socket documentation about NetBSD support.

87f6f79

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 13, 2022

bedevere-bot requested a review from serhiy-storchaka January 13, 2022 16:57

serhiy-storchaka reviewed Jan 18, 2022

View reviewed changes

0-wiz-0 added 2 commits January 18, 2022 23:51

ifdef vs. if defined consistency

0a2e273

Add Whatsnew entry for NetBSD support in socket module

921d0a3

serhiy-storchaka approved these changes Jan 21, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jan 21, 2022

serhiy-storchaka added the type-feature A feature request or enhancement label Jan 21, 2022

serhiy-storchaka merged commit 40fcd16 into python:main Jan 21, 2022

bedevere-bot removed the awaiting merge label Jan 21, 2022

erlend-aasland mentioned this pull request May 18, 2022

bpo-45172: netbsd socketmodule CAN protocol constants addition. #28288

Closed

jbogers mentioned this pull request Feb 6, 2025

socket.CAN_RAW_ERR_FILTER is not defined in Python 3.11 and later #129719

Closed

ZeroIntensity mentioned this pull request Feb 6, 2025

gh-129719: Restore missing socket.CAN_RAW_ERR_FILTER on Linux #129721

Merged

	Add CAN Socket support for NetBSD
	Add CAN Socket support for NetBSD.

Uh oh!

Conversation

0-wiz-0 commented Dec 11, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taleinat commented Jan 8, 2022

Uh oh!

taleinat commented Jan 8, 2022

Uh oh!

0-wiz-0 commented Jan 8, 2022

Uh oh!

taleinat commented Jan 9, 2022

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Jan 10, 2022

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Jan 10, 2022

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Jan 10, 2022

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 10, 2022

Uh oh!

0-wiz-0 commented Jan 13, 2022

Uh oh!

0-wiz-0 commented Jan 13, 2022

Uh oh!

bedevere-bot commented Jan 13, 2022

Uh oh!

serhiy-storchaka Jan 18, 2022

Choose a reason for hiding this comment

Uh oh!

0-wiz-0 Jan 18, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

serhiy-storchaka commented Jan 18, 2022

Uh oh!

0-wiz-0 commented Jan 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

0-wiz-0 commented Dec 11, 2021 •

edited by bedevere-bot

Loading