GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-35569: Expose RFC 3542 IPv6 socket options on macOS by erlend-aasland · Pull Request #19526 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Apr 14, 2020 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

From macOS SDK netinet6/in6.h:

    RFC 3542 define the following socket options in a manner incompatible
    with RFC 2292:
      IPV6_PKTINFO
      IPV6_HOPLIMIT
      IPV6_NEXTHOP
      IPV6_HOPOPTS
      IPV6_DSTOPTS
      IPV6_RTHDR
    
    To use the new IPv6 Sockets options introduced by RFC 3542
    the constant __APPLE_USE_RFC_3542 must be defined before
    including <netinet/in.h>
    
    [...]
    
    Note that eventually RFC 3542 is going to be the
    default and RFC 2292 will be obsolete.

https://bugs.python.org/issue35569

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Apr 14, 2020
@erlend-aasland
erlend-aasland force-pushed the fix-issue-35569 branch from 42deb8a to 0224de9 Compare April 18, 2020 19:12
@erlend-aasland

erlend-aasland commented Apr 18, 2020

Copy link
Copy Markdown
Contributor Author

FYI, force pushed to kick off stalled checks.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@remilapeyre

remilapeyre commented Apr 18, 2020

Copy link
Copy Markdown
Contributor

FYI, force pushed to kick off stalled checks.

I tested the path on Catalina and its works well. Thanks!

erlend-aasland and crfosse reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

remilapeyre
remilapeyre approved these changes Apr 18, 2020
View reviewed changes
@bedevere-bot bedevere-bot added the awaiting core review label Apr 18, 2020
@erlend-aasland

erlend-aasland commented Apr 18, 2020

Copy link
Copy Markdown
Contributor Author

Thank you, @remilapeyre !

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ned-deily
ned-deily requested changes Apr 27, 2020
View reviewed changes
Comment thread Lib/test/test_socket.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting changes and removed awaiting core review labels Apr 27, 2020
@bedevere-bot

bedevere-bot commented Apr 27, 2020

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ned-deily
ned-deily requested a review from ronaldoussoren April 27, 2020 08:46
ned-deily
ned-deily reviewed Apr 27, 2020
View reviewed changes
Comment thread configure.ac Resolved

Uh oh!

There was an error while loading. Please reload this page.

Erlend E. Aasland added 4 commits April 27, 2020 12:27
Erlend E. Aasland
bpo-35569: Expose RFC 3542 socket options on macOS
ae17650
From macOS SDK netinet6/in6.h:
RFC 3542 define the following socket options in a manner incompatible
with RFC 2292:
  IPV6_PKTINFO
  IPV6_HOPLIMIT
  IPV6_NEXTHOP
  IPV6_HOPOPTS
  IPV6_DSTOPTS
  IPV6_RTHDR

To use the new IPv6 Sockets options introduced by RFC 3542
the constant __APPLE_USE_RFC_3542 must be defined before
including <netinet/in.h>

[...]

Note that eventually RFC 3542 is going to be the
default and RFC 2292 will be obsolete.
Erlend E. Aasland
bpo-35569: Add unit tests
fe71bb7
Erlend E. Aasland
Use socket_helper (see PR python#19603)
ebfde1f
Erlend E. Aasland
Only define __APPLE_USE_RFC_3542 when building socket lib
f6bda5d
@erlend-aasland
erlend-aasland force-pushed the fix-issue-35569 branch from 0224de9 to f6bda5d Compare April 27, 2020 10:40
@erlend-aasland

erlend-aasland commented Apr 27, 2020

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again.

FYI: test_peg_generator fails on my machine (master HEAD => 4044c84). I pushed this to GitHub to see if the CI also fails.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Apr 27, 2020
@bedevere-bot

bedevere-bot commented Apr 27, 2020

Copy link
Copy Markdown

Thanks for making the requested changes!

@ned-deily: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot
bedevere-bot requested a review from ned-deily April 27, 2020 10:43
@erlend-aasland

erlend-aasland commented Apr 27, 2020

Copy link
Copy Markdown
Contributor Author

Ref. my previous comment (and completely irrelevant to this PR): I see that the CI has no problems with test_peg_generator, so that problem is on my side.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Apr 27, 2020

Copy link
Copy Markdown
Contributor Author

BTW, detect_socket() in setup.py could be restructured a little bit. Something like this would be cleaner:

    def detect_socket(self):
        # socket(2)
        kwargs = {'depends': ['socketmodule.h']}
        if VXWORKS:
            kwargs['libraries'] = ['net']
        elif MACOS:
            # Issue #35569: Expose RFC 3542 socket options.
            kwargs['extra_compile_args'] = ['-D__APPLE_USE_RFC_3542']

        self.add(Extension('_socket', ['socketmodule.c'], **kwargs))

I guess that would need to go in a separate issue?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ned-deily

ned-deily commented May 17, 2020

Copy link
Copy Markdown
Member

Your restructered detect_socket() looks nicer but, unless I'm missing something, it doesn't seem to behave the same as the original code. The current code doesn't attempt to build _socket on VxWorks if it can't find the net lib directory, no? Let's go with how the PR stands currently.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ned-deily
ned-deily approved these changes May 17, 2020
View reviewed changes

@ned-deily ned-deily left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM, thanks! That's a lot cleaner.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels May 17, 2020
@ned-deily
ned-deily merged commit 9a45bfe into python:master May 17, 2020
@bedevere-bot bedevere-bot removed the awaiting merge label May 17, 2020
@erlend-aasland

erlend-aasland commented May 17, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Thanks, @ned-deily !

Your restructered detect_socket() looks nicer but, unless I'm missing something, it doesn't seem to behave the same as the original code. The current code doesn't attempt to build _socket on VxWorks if it can't find the net lib directory, no?

You're right, I mistook the elif for an else!

This should do the trick:

    def detect_socket(self):
        # socket(2)
        if VXWORKS:
            if not self.compiler.find_library_file(self.lib_dirs, 'net'):
                return
            kwargs = {'libraries': ['net']}
        elif MACOS:
            # Issue #35569: Expose RFC 3542 socket options.
            kwargs = {'extra_compile_args': ['-D__APPLE_USE_RFC_3542']}
        else:
            kwargs = {}

        kwargs['depends'] = ['socketmodule.h']
        self.add(Extension('_socket', ['socketmodule.c'], **kwargs))

Or we could just initialize kwargs first, and drop the last else

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
erlend-aasland deleted the fix-issue-35569 branch May 17, 2020 07:56
@erlend-aasland erlend-aasland mentioned this pull request May 17, 2020
bpo-40656: Clean up detect_socket() #20148
Merged
arturoescaip pushed a commit to arturoescaip/cpython that referenced this pull request May 24, 2020
Erlend Egeberg Aasland @arturoescaip
bpo-35569: Expose RFC 3542 IPv6 socket options on macOS (pythonGH-19526)
3d848d3
@santigimeno santigimeno mentioned this pull request May 14, 2026
udp: add ECN, PMTUD, pktinfo, and GSO/GRO extensions to uv_udp_t libuv/libuv#5116
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ned-deily ned-deily ned-deily approved these changes
@ronaldoussoren ronaldoussoren Awaiting requested review from ronaldoussoren
+1 more reviewer
@remilapeyre remilapeyre remilapeyre approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@erlend-aasland @remilapeyre @bedevere-bot @ned-deily @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.