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

[corona10]

https://bugs.python.org/issue35923

[corona10]

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

cpython/Lib/test/test_importlib/builtin/test_finder.py

Line 20 in e42b705

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

[aeros]

Thanks for the PR @corona10.

I have a minor suggestion for the news entry:

[aeros]

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]

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

[aeros]

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]

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]

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]

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

[vstinner]

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]

Except for Azure's failing issue, ...

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

[corona10]

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

[aeros]

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]

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

[brettcannon]

Thanks!