Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

bpo-35923: Update the BuiltinImporter to use loader._ORIGIN#15651

Merged
brettcannon merged 4 commits into
python:masterpython/cpython:masterfrom
corona10:bpo-35923corona10/cpython:bpo-35923Copy head branch name to clipboard
Sep 11, 2019
Merged

bpo-35923: Update the BuiltinImporter to use loader._ORIGIN#15651
brettcannon merged 4 commits into
python:masterpython/cpython:masterfrom
corona10:bpo-35923corona10/cpython:bpo-35923Copy head branch name to clipboard

Conversation

@corona10

@corona10 corona10 commented Sep 2, 2019

Copy link
Copy Markdown
Member

@corona10

corona10 commented Sep 2, 2019

Copy link
Copy Markdown
Member Author

To core-developers, The unit test for BuiltinImporter is already done by the below codes.

self.assertEqual(found.origin, 'built-in')

@corona10 corona10 force-pushed the bpo-35923 branch 2 times, most recently from a327f57 to ea958b1 Compare September 2, 2019 17:00
@aeros aeros added the type-feature A feature request or enhancement label Sep 2, 2019

@aeros aeros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR @corona10.

I have a minor suggestion for the news entry:

Comment thread Misc/NEWS.d/next/Library/2019-09-03-01-41-35.bpo-35923.lYpKbY.rst Outdated
@aeros

aeros commented Sep 2, 2019

Copy link
Copy Markdown
Contributor

Also as a side note, I think a long enough duration of time has passed since nnja assigned themselves to the issue for someone else to work on it. The last update they made to the issue was 7 months ago, and a PR was never opened for it.

@corona10

corona10 commented Sep 2, 2019

Copy link
Copy Markdown
Member Author

@aeros167 Thank's for the review. I've applied your suggestion inline.

@aeros

aeros commented Sep 3, 2019

Copy link
Copy Markdown
Contributor

I've applied your suggestion inline.

Thanks for making the changes @corona10. 👍

As a minor adjustment, would you mind splitting the first line of the news entry into the next line? It looks like the recent changes put it above the 80 maximum for reST:

>>> len("Update :class:`importlib.machinery.BuiltinImporter` to use ``loader._ORIGIN`` instead of a")
90

Changing the news entry to this will fix it:

Update :class:`importlib.machinery.BuiltinImporter` to use ``loader._ORIGIN``
instead of a hardcoded value. Patch by Dong-hee Na.

@aeros

aeros commented Sep 3, 2019

Copy link
Copy Markdown
Contributor

Looks like Azure is failing from the mac tests timing out:

macOS PR Tests: "The job running on agent Hosted Agent has exceeded the maximum execution time of 60. For more information, see https://go.microsoft.com/fwlink/?linkid=2077134 "

Looking a bit more in-depth at the failure, it looks the mac tests timed out during the following test:

test_pending_calls_race (test.test_concurrent_futures.ThreadPoolWaitTests) ...

Previously I'd have closed and opened the PR to retrigger the tests, but I think it would be better to consult the CI experts before doing so. I wonder if this is related to https://bugs.python.org/issue18049. More specifically the changes made in https://github.com/python/cpython/pull/15065/files to THREAD_STACK_SIZE in Python/thread_pthread.h.

/cc @vstinner @pablogsal

Edit: Since the previous tests were passing before the news entry was modified, it's likely the CI failure isn't related to the PR. Let's wait on their feedback before moving forward though. This might be revealing a bug in the macOS tests.

@vstinner

vstinner commented Sep 4, 2019

Copy link
Copy Markdown
Member

Previously I'd have closed and opened the PR to retrigger the tests, but I think it would be better to consult the CI experts before doing so. I wonder if this is related to https://bugs.python.org/issue18049. More specifically the changes made in https://github.com/python/cpython/pull/15065/files to THREAD_STACK_SIZE in Python/thread_pthread.h.

Comment https://bugs.python.org/issue18049 to ask people who were involved in these changes to have a look. If they consider that it's unrelated, a new issue should be opened.

@corona10

corona10 commented Sep 4, 2019

Copy link
Copy Markdown
Member Author

@vstinner
Except for Azure's failing issue, Can you recommend the reviewer for this PR?

@vstinner

vstinner commented Sep 4, 2019

Copy link
Copy Markdown
Member

Can you recommend the reviewer for this PR?

On https://devguide.python.org/experts/ there is only @brettcannon who is already marked as a reviewer. @ncoghlan and @warsaw may also be available to review importlib changes.

@vstinner

vstinner commented Sep 4, 2019

Copy link
Copy Markdown
Member

Except for Azure's failing issue, ...

I filled https://bugs.python.org/issue37245 in June. It seems like the issue is still occurring.

@corona10

corona10 commented Sep 11, 2019

Copy link
Copy Markdown
Member Author

@brettcannon I've repushed the PR. Please take a look after jobs are done :)

@aeros aeros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One last minor nit, can you adjust the Misc/NEWS entry to fit the max words per line within the reST 80 char limit?

Current:

Update :class:importlib.machinery.BuiltinImporter to use
loader._ORIGIN instead of a hardcoded value. Patch by Dong-hee Na.

Suggested:

Update :class:importlib.machinery.BuiltinImporter to use loader._ORIGIN
instead of a hardcoded value. Patch by Dong-hee Na.

@corona10

Copy link
Copy Markdown
Member Author

@aeros167 Thank you for the review. I 've updated it.

@brettcannon brettcannon merged commit 145cf1f into python:master Sep 11, 2019
@brettcannon

Copy link
Copy Markdown
Member

Thanks!

@corona10 corona10 deleted the bpo-35923 branch September 11, 2019 16:33
DinoV pushed a commit to DinoV/cpython that referenced this pull request Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.