Skip to content

add socket.__all__ #13044

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

Merged
merged 13 commits into from
Nov 20, 2024
Merged

add socket.__all__ #13044

merged 13 commits into from
Nov 20, 2024

Conversation

tungol
Copy link
Contributor

@tungol tungol commented Nov 18, 2024

I didn't realize there were so many errors being hidden for linux in here when I started. I only have access to MacOS and FreeBSD locally, so I just took the linux pipeline results at mostly face value. In working through all the things that don't exist in the Linux pipeline, I tested against my BSD machine to sort out what was for example, platform = "win32" versus platform != "darwin" and platform != "linux" or platform = "darwin" versus "platform != "win32" and platform != "linux", etc.

When it didn't seem like any system that had a thing, I commented it out in _socket and removed it from socket. I expect many of these exist on some linux system somewhere - there's a lot of variance in that space. It seemed like leaving them as a comment in _socket was a good placeholder for now.

@tungol tungol marked this pull request as draft November 19, 2024 00:05

This comment has been minimized.

This comment has been minimized.

1 similar comment

This comment has been minimized.

This comment has been minimized.

If I've done this correctly, this irons out the errors that are
different depending on python version, leaving just the errors
that show up for every version of linux.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@tungol tungol marked this pull request as ready for review November 19, 2024 06:55
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Let's not comment out any of these constants. They're all defined with IFDEF in CPython and presumably exist somewhere, though I suspect some may exist only on versions of FreeBSD from the mid-1990s compiled while Mars is in retrograde. If you want a rabbit hole to dig into, you could try to figure out exactly where those constants come from (e.g., by digging through CPython git blame or Googling for manpages from the right OS).

SOL_IPX: int
SOL_NETROM: int
SOL_ROSE: int
# if sys.platform != "win32" and sys.platform != "darwin":
Copy link
Member

Choose a reason for hiding this comment

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

Let's not comment these out; they definitely exist on some platforms.

@tungol tungol marked this pull request as draft November 19, 2024 19:29
@tungol
Copy link
Contributor Author

tungol commented Nov 19, 2024

I'll do some more digging on the mystery constants.

@JelleZijlstra
Copy link
Member

Thanks! To be clear, I don't think it's too important to figure out exactly when these constants exist, and I'd be fine with defining them under if sys.platform != "linux" and sys.platform != "win32" and sys.platform != "darwin" if we know they don't exist on any of the three major platforms.

@tungol
Copy link
Contributor Author

tungol commented Nov 19, 2024

Okay, that makes sense. In the case of the ones I commented out, I also confirmed that they didn't exist on (my local version of) FreeBSD either.

@tungol tungol marked this pull request as ready for review November 19, 2024 22:11
@tungol
Copy link
Contributor Author

tungol commented Nov 19, 2024

Updated, with 7 constants still commented out. 6 of these are linux-only NETLINK_* constants that haven't been in linux for almost 20 years now. One is the linux-only RDS_CMSG_RDMA_UPDATE which I couldn't find in any version of linux and I think is a typo for the actually existing RDS_CMSG_CONG_UPDATE.

Most of the rest I confirmed are in linux header files to this day, so I restored them to both _socket and socket without adding them to __all__. Those that I couldn't find in a linux header file went to platform != the big three branches.

This comment has been minimized.

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

Thanks!

@srittau srittau merged commit 308a09f into python:main Nov 20, 2024
3 of 4 checks passed
@@ -456,6 +497,10 @@ if sys.platform == "linux":
RDS_RDMA_USE_ONCE: int
RDS_RECVERR: int

# This is supported by cpython but doesn't seem to be a real thing.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# This is supported by cpython but doesn't seem to be a real thing.
# This is supported by CPython but doesn't seem to be a real thing.

Copy link
Member

Choose a reason for hiding this comment

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

If you want extra brownie points, you could consider opening an issue at CPython about this ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CPython issue for this constant: python/cpython#127069

NETLINK_XFRM: int
# Technically still supported by cpython
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Technically still supported by cpython
# Technically still supported by CPython

Copy link
Contributor Author

@tungol tungol Nov 20, 2024

Choose a reason for hiding this comment

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

And a CPython issue for these outdated Netlink constants as well: python/cpython#127072

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@tungol tungol deleted the socket branch November 20, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants