bpo-34788: Add support for scoped IPv6 addresses #13772

opavlyuk · 2019-06-03T08:57:15Z

https://bugs.python.org/issue34788

Automerge-Triggered-By: @asvetlov

Support for scoped IPv6 addresses (including network and interface addresses) is required according to RFC 4007 (https://tools.ietf.org/html/rfc4007).

aeros

@opavlyuk Welcome and thanks for the contribution!

I would advise breaking up this PR into multiple smaller ones (approximately the size of the commits or smaller, but as self-contained chunks starting with the least dependent ones). This would allow for both easier review and debugging of the integration tests. Otherwise, this process will likely take significantly longer.

Also from the perspective of readability, I would recommend replacing the longer code blocks of repeating assertBadSplit() with iteration.

For example on lines 359-368, instead of using this:

        assertBadSplit("3ffe::1::1%scope")
        assertBadSplit("1::2::3::4:5%scope")
        assertBadSplit("2001::db:::1%scope")
        assertBadSplit("3ffe::1::%scope")
        assertBadSplit("::3ffe::1%scope")
        assertBadSplit(":3ffe::1::1%scope")
        assertBadSplit("3ffe::1::1:%scope")
        assertBadSplit(":3ffe::1::1:%scope")
        assertBadSplit(":::%scope")
        assertBadSplit('2001:db8:::1%scope')

I would recommend something like this:

# since this is just an example, I didn't include all of them
bad_split_asserts = ["3ffe::1::1%scope", "1::2::3::4:5%scope", "2001::db:::1%scope",]
for assertion in bad_split_asserts:
    assertBadSplit(assertion)

This would significantly improve the overall readability of the code and more quickly conveys the message of what is being done. This doesn't have to be done when using a similar format for 2-3 lines, but it starts to add an unnecessary number of extra lines and characters when it's done 5+ times.

Edit: Also I forgot to mention that we generally use underscores to represent spaces, not camelCase. So assertBadSplit() should be assert_bad_split().

opavlyuk · 2019-07-16T12:27:36Z

@aeros167 Thanks for your review!

I would advise breaking up this PR into multiple smaller ones (approximately the size of the commits or smaller, but as self-contained chunks starting with the least dependent ones).

As for me, this PR could not be broken into multiple smaller ones, because it refers to and covers a single issue. Also, all changes depend on each other and could not be merged separately (e.g. tests after main code or vice versa). According to devguide, tests and documentation should be included to PR. So, seems it would not be right to make separate PR`s.
May be, some tests could be considered redundant, but devguide insists on writing tests both for normal behaviour and for error conditions.

Also from the perspective of readability, I would recommend replacing the longer code blocks of repeating assertBadSplit() with iteration.

Please, take a look at

cpython/Lib/test/test_ipaddress.py

Lines 348 to 357 in fb99b6b

    
           assertBadSplit("3ffe::1::1") 
        
           assertBadSplit("1::2::3::4:5") 
        
           assertBadSplit("2001::db:::1") 
        
           assertBadSplit("3ffe::1::") 
        
           assertBadSplit("::3ffe::1") 
        
           assertBadSplit(":3ffe::1::1") 
        
           assertBadSplit("3ffe::1::1:") 
        
           assertBadSplit(":3ffe::1::1:") 
        
           assertBadSplit(":::") 
        
           assertBadSplit('2001:db8:::1')

In my PR I only extend this test with cases, consisting scope IPv6 addresses. It was already written in such a style. According to devguide, it isn't recommended to make cosmetic changes to unrelated code. PEP8 also tells that, if an existing library has a different style, internal consistency is preferred. So, to keep existing style of test, I haven't put assertions in loop. But, if it is a must, I will fix that lines, of course.

Edit: Also I forgot to mention that we generally use underscores to represent spaces, not camelCase. So assertBadSplit() should be assert_bad_split().

Again,

cpython/Lib/test/test_ipaddress.py

Line 343 in fb99b6b

def assertBadSplit(addr):

was written before me, so I consider not to fix naming style. Also, as far as unitest library is a port of JUnit, it inherits Java naming style in assertion methods. Just take a look at all other assertion method names, they are camelCased. In my opinion, it would be right to keep existing code style in the module.

aeros · 2019-07-16T21:25:16Z

@opavlyuk

As for me, this PR could not be broken into multiple smaller ones, because it refers to and covers a single issue. Also, all changes depend on each other and could not be merged separately (e.g. tests after main code or vice versa). According to devguide, tests and documentation should be included to PR. So, seems it would not be right to make separate PR`s.
May be, some tests could be considered redundant, but devguide insists on writing tests both for normal behaviour and for error conditions.

Usually, for more involved issues, a single PR represents a self-contained step towards the resolution. Self-contained meaning that the PR also contains the respective documentation changes, test coverage, and is compatible with the existing code-base by itself. This doesn't mean that the entire issue has to be covered at once.

As for this PR specifically, I'm not entirely certain into what self-contained pieces it could be further fragmented into, it's just a general guideline. If it's not reasonably possible to do this, it's not at all required. The most substantial benefit of fragmenting it into multiple PRs is easier debugging when the integration tests aren't passing.

In my PR I only extend this test with cases, consisting scope IPv6 addresses. It was already written in such a style. According to devguide, it isn't recommended to make cosmetic changes to unrelated code. PEP8 also tells that, if an existing library has a different style, internal consistency is preferred. So, to keep existing style of test, I haven't put assertions in loop.

Upon reconsideration and further inspection of the existing tests, keeping the current style of the library is understandable, especially given the sheer volume. Perhaps I'll work on refactoring it in a future PR, but it'd likely have to be done all at once for consistency. When I had first submitted my initial review, I had underestimated the size of the existing tests. Improving the readability would likely be a project on it's own. I wouldn't expect you to refactor all of the entire existing tests within the same issue.

The above also applies to the naming conventions. I'm far more used to our more recent unit tests using the underscore naming. My apologies, it looks like I focused a bit too much on your additions rather than the context.

But, if it is a must, I will fix that lines, of course.

As someone who isn't a core dev, my reviews are just recommendations based on my experiences thus far working within the Python repositories. So if you have reasons for doing anything a particular way and the core devs agree with your justification, you can definitely keep it as it is.

opavlyuk · 2019-07-17T09:51:46Z

@aeros167, I've added entry to Misc/NEWS.d and resolved issues with suspicious lines in documentation, which caused checks to fail. Now, all checks are passing.
Looking forward to make all necessary fixes and have an approved PR .

aeros

The documentation and changes to ipaddress look good as far as I can tell. Next, we just have give the core devs some time to review the PR. Based on the experts index pmoody is the expert for ipaddress, but there is not a github account listed on their issue tracker profile. As a result, I won't be able to mention them in the PR's comment. They're already on the issue's nosy list though.

As a minor fix, the news entry column width is supposed to be 80 characters according to the devguide, so I split it into two lines. Also, I trimmed the path to ipaddress down by replacing it with reST inline markup (mod is an abbreviation of module).

To simply a conditional, I made a minor suggestion for line 1863 of ipaddress.py.

Misc/NEWS.d/next/Library/2019-07-17-08-26-14.bpo-34788.pwV1OK.rst

Lib/ipaddress.py

Co-Authored-By: Kyle Stanley <aeros167@gmail.com>

terryjreedy · 2020-02-13T18:46:45Z

@opavlyuk There are 3 issues more important at the moment than the whatsnew item.

CLA: bot initially said 'signed', then revoked that without explanation, more that a day ago, though I no longer see the initial revocation notice.
Commit #: header says you want 'to merge 294 commits', which is wrong. We usually close PRs opened like this and ask for a resubmission. But I believe mistake came later, apparently after initial reviews, which we do not want to close, perhaps with an unneeded and bad force push. (Github is not showing the history of your commits here like it usually does.) I hoped that update merges would fix this (as it did on another issue), but no.
Open change request: click [+-] box after gnprice to find them again.

Please ask on core-mentorship list how to deal with 1 and 2 and anything about 3 that you are not sure of.

opavlyuk · 2020-02-14T13:46:10Z

@terryjreedy , thank you for your help with this PR, I really appreciate your efforts.
About issues:

Fixed while resolving 2-nd issue;
I figured out, that extra commits appeared after I merged master branch from cpython repo directly to current issue branch, instead of merge it first to master branch of my repo and futher merge to issue branch. Fixed it with force push.
As for now, I have a lack of time to write expalnations why I haven't listened to every gnprice's advice, but I`ve already applied all suggestions, that I consider reasonable.

terryjreedy · 2020-02-14T18:19:45Z

@gnprice Of your many suggestions, opavlyuk has "applied all suggestions, that I consider reasonable". Of the rest, are there any you consider critical, or would you be comfortable merging as is?

asvetlov

Looks good and ready to merge.
Please drop a few lines from whatsnew.

Sorry, for the long review, I was pretty busy.
Thanks for your patience.

asvetlov · 2020-02-26T10:43:17Z

Doc/whatsnew/3.9.rst

+* Provide :c:func:`Py_EnterRecursiveCall` and :c:func:`Py_LeaveRecursiveCall`
+  as regular functions for the limited API. Previously, there were defined as
+  macros, but these macros didn't work with the limited API which cannot access
+  ``PyThreadState.recursion_depth`` field. Remove ``_Py_CheckRecursionLimit``
+  from the stable ABI.
+  (Contributed by Victor Stinner in :issue:`38644`.)
+


Please drop these lines, they are added by accident.
Victor decided to move this text to another place.

Thanks a lot! Fixed and also corrected typo in my own surname (what a shame).

miss-islington · 2020-02-26T14:20:11Z

@opavlyuk: Status check is done, and it's a failure ❌ .

miss-islington · 2020-02-26T14:33:54Z

@opavlyuk: Status check is done, and it's a success ✅ .

asvetlov · 2020-02-26T14:43:03Z

Thanks!

bpo-34788: Add support for scoped IPv6 addresses (pythonGH-13772)

bedevere-bot · 2020-02-26T20:28:01Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86-64 macOS 3.x has failed when building commit 21da76d.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/275/builds/286) and take a look at the build logs.
Check if the failure is related to this commit (21da76d) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/275/builds/286

Summary of the results of the build (if available):

Click to see traceback logs

remote: Enumerating objects: 25, done.        
remote: Counting objects:   4% (1/25)        
remote: Counting objects:   8% (2/25)        
remote: Counting objects:  12% (3/25)        
remote: Counting objects:  16% (4/25)        
remote: Counting objects:  20% (5/25)        
remote: Counting objects:  24% (6/25)        
remote: Counting objects:  28% (7/25)        
remote: Counting objects:  32% (8/25)        
remote: Counting objects:  36% (9/25)        
remote: Counting objects:  40% (10/25)        
remote: Counting objects:  44% (11/25)        
remote: Counting objects:  48% (12/25)        
remote: Counting objects:  52% (13/25)        
remote: Counting objects:  56% (14/25)        
remote: Counting objects:  60% (15/25)        
remote: Counting objects:  64% (16/25)        
remote: Counting objects:  68% (17/25)        
remote: Counting objects:  72% (18/25)        
remote: Counting objects:  76% (19/25)        
remote: Counting objects:  80% (20/25)        
remote: Counting objects:  84% (21/25)        
remote: Counting objects:  88% (22/25)        
remote: Counting objects:  92% (23/25)        
remote: Counting objects:  96% (24/25)        
remote: Counting objects: 100% (25/25)        
remote: Counting objects: 100% (25/25), done.        
remote: Compressing objects:  25% (1/4)        
remote: Compressing objects:  50% (2/4)        
remote: Compressing objects:  75% (3/4)        
remote: Compressing objects: 100% (4/4)        
remote: Compressing objects: 100% (4/4), done.        
remote: Total 29 (delta 21), reused 21 (delta 21), pack-reused 4        
From https://github.com/python/cpython
 * branch                  master     -> FETCH_HEAD
Reset branch 'master'

Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/4.2.1

make: *** No rule to make target `distclean'.  Stop.

terryjreedy · 2020-02-26T21:40:45Z

Error message for build 286 is

command timed out: 1200 seconds without output running ['./configure', '--prefix', '$(PWD)/target', '--with-pydebug'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=2048.933014
checking for linux/vm_sockets.h...

Since patch did not add a C include, it is not obvious that this is really related. Since there have not been any subsequent merges and builds, I triggered a rebuild.
https://buildbot.python.org/all/#/builders/275/builds/287

opavlyuk · 2020-02-26T22:35:59Z

@terryjreedy thanks!

--- <exception caught here> ---
  File "/data/buildbot/venv/lib/python3.8/site-packages/buildbot/process/build.py", line 375, in startBuild
    ping_success_or_failure = yield workerforbuilder.ping()
twisted.spread.pb.PBConnectionLost: [Failure instance: Traceback (failure with no frames): <class 'twisted.internet.error.ConnectionLost'>: Connection to the other side was lost in a non-clean fashion.
]

I have no idea how my patch could cause such things. It is likely an environment issue.

terryjreedy · 2020-02-27T04:44:32Z

The next build, 288, passed, so I think we are good for this issue.

asvetlov · 2020-02-27T10:31:52Z

Agree

opavliuk added 8 commits May 31, 2019 14:40

bpo-34788 Add tests for ipv6 scope_id support

eb4a407

bpo-34788 Add support for scoped IPv6 addresses

8a5c94f

Support for scoped IPv6 addresses (including network and interface addresses) is required according to RFC 4007 (https://tools.ietf.org/html/rfc4007).

bpo-34788 Add documentation for scope id support

a841e9a

bpo-34788 Refactor address split and validation

9a3f7ec

bpo-34788 Convert scope_id to property

a17538a

bpo-34788 Update documentation

6da269e

bpo-34788 Remove redundant check

35750de

bpo-34788 Fix newline

681cfa5

the-knights-who-say-ni added the CLA signed label Jun 3, 2019

bedevere-bot added the awaiting review label Jun 3, 2019

bpo-34788 Documentation fix

fb99b6b

aeros reviewed Jul 15, 2019

View reviewed changes

blurb-it bot and others added 5 commits July 17, 2019 08:26

📜🤖 Added by blurb_it.

d8bb674

bpo-34788 Update NEWS

ccd83ed

bpo-34788 Fix NEWS filename

dc540e9

bpo-34788 Add sample IPv6 to ignore list

c03d4a5

bpo-34788 Add sample IPv6 to ignore list

c12e533

aeros approved these changes Jul 18, 2019

View reviewed changes

Misc/NEWS.d/next/Library/2019-07-17-08-26-14.bpo-34788.pwV1OK.rst Outdated Show resolved Hide resolved

Lib/ipaddress.py Outdated Show resolved Hide resolved

bedevere-bot added awaiting core review and removed awaiting review labels Jul 18, 2019

opavlyuk and others added 3 commits July 18, 2019 10:26

bpo-34788 Update Misc/NEWS.d

033f031

Co-Authored-By: Kyle Stanley <aeros167@gmail.com>

bpo-34788 Refactor condition

27c5cf4

Co-Authored-By: Kyle Stanley <aeros167@gmail.com>

bpo-34788 Fix excessive property call

3b495c0

opavlyuk requested review from 1st1, abalkin, asvetlov and berkerpeksag as code owners August 5, 2019 11:04

opavlyuk force-pushed the fix-issue-34788 branch from 3eb3d42 to e041c2e Compare February 14, 2020 12:17

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Feb 14, 2020

Merge remote-tracking branch 'origin/master' into fix-issue-34788

0e250bf

python deleted a comment from the-knights-who-say-ni Feb 14, 2020

python deleted a comment from codecov bot Feb 14, 2020

asvetlov approved these changes Feb 26, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Feb 26, 2020

Cleanup Whatsnew

2c4d3d5

asvetlov added 🤖 automerge and removed DO-NOT-MERGE labels Feb 26, 2020

Fix typo

555f59f

miss-islington merged commit 21da76d into python:master Feb 26, 2020

bedevere-bot removed the awaiting merge label Feb 26, 2020

sthagen added a commit to sthagen/python-cpython that referenced this pull request Feb 26, 2020

Merge pull request #273 from python/master

3a3d58f

bpo-34788: Add support for scoped IPv6 addresses (pythonGH-13772)

kristjanvalur mentioned this pull request Jan 6, 2024

Add 'key' argument to list.index() #113773

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-34788: Add support for scoped IPv6 addresses #13772

bpo-34788: Add support for scoped IPv6 addresses #13772

opavlyuk commented Jun 3, 2019 •

edited by miss-islington

Loading

aeros left a comment •

edited

Loading

opavlyuk commented Jul 16, 2019

aeros commented Jul 16, 2019

opavlyuk commented Jul 17, 2019

aeros left a comment •

edited

Loading

terryjreedy commented Feb 13, 2020

opavlyuk commented Feb 14, 2020 •

edited

Loading

terryjreedy commented Feb 14, 2020

asvetlov left a comment

asvetlov Feb 26, 2020

opavlyuk Feb 26, 2020

miss-islington commented Feb 26, 2020

miss-islington commented Feb 26, 2020

asvetlov commented Feb 26, 2020

bedevere-bot commented Feb 26, 2020

terryjreedy commented Feb 26, 2020 •

edited

Loading

opavlyuk commented Feb 26, 2020

terryjreedy commented Feb 27, 2020

asvetlov commented Feb 27, 2020

bpo-34788: Add support for scoped IPv6 addresses #13772

bpo-34788: Add support for scoped IPv6 addresses #13772

Conversation

opavlyuk commented Jun 3, 2019 • edited by miss-islington Loading

aeros left a comment • edited Loading

Choose a reason for hiding this comment

opavlyuk commented Jul 16, 2019

aeros commented Jul 16, 2019

opavlyuk commented Jul 17, 2019

aeros left a comment • edited Loading

Choose a reason for hiding this comment

terryjreedy commented Feb 13, 2020

opavlyuk commented Feb 14, 2020 • edited Loading

terryjreedy commented Feb 14, 2020

asvetlov left a comment

Choose a reason for hiding this comment

asvetlov Feb 26, 2020

Choose a reason for hiding this comment

opavlyuk Feb 26, 2020

Choose a reason for hiding this comment

miss-islington commented Feb 26, 2020

miss-islington commented Feb 26, 2020

asvetlov commented Feb 26, 2020

bedevere-bot commented Feb 26, 2020

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

terryjreedy commented Feb 26, 2020 • edited Loading

opavlyuk commented Feb 26, 2020

terryjreedy commented Feb 27, 2020

asvetlov commented Feb 27, 2020

opavlyuk commented Jun 3, 2019 •

edited by miss-islington

Loading

aeros left a comment •

edited

Loading

aeros left a comment •

edited

Loading

opavlyuk commented Feb 14, 2020 •

edited

Loading

terryjreedy commented Feb 26, 2020 •

edited

Loading