bpo-39336: Allow packages to not let their child modules be set on them #18006

DinoV · 2020-01-14T23:51:06Z

This allows an import to proceed if a module raises AttributeError when attempting to set a child module on the parent package. This can occur if a loader wants to make its modules immutable, in which case the module cannot be set. In this case an ImportWarning is still logged in case this behavior is unexpected.

https://bugs.python.org/issue39336

Fix whitespace Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

brettcannon · 2020-01-15T18:36:07Z

Lib/importlib/_bootstrap.py

+        try:
+            setattr(parent_module, child, module)
+        except AttributeError:
+            msg = (f"Can't set child package '{child}' on "


Suggested change

msg = (f"Can't set child package '{child}' on "

msg = (f"Cannot set an attribute on {parent!r} for child module {child!r}"

brettcannon · 2020-01-15T18:41:05Z

Lib/test/test_import/__init__.py

@@ -1339,6 +1342,39 @@ def test_circular_from_import(self):
            str(cm.exception),
        )

+    def test_unwritable_module(self):
+        package = inspect.cleandoc('''


Any reason to not put this as a module in test_import/data/?

Nope, I just didn't realize that was a thing :)

bedevere-bot · 2020-01-15T18:41:37Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

DinoV · 2020-01-16T18:43:20Z

I have made the requested changes; please review again.

bedevere-bot · 2020-01-16T18:43:23Z

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

brettcannon · 2020-01-17T22:21:11Z

Lib/test/test_import/__init__.py

-            with open(os.path.join(os.path.dirname(path), 'x.py'), 'w+'):
-                pass
-
+        sys.path.insert(0, os.path.join(os.path.dirname(__file__), 'data'))


Why the path insertion instead of importing test.test_import.data.unwritable?

I was actually just following the pattern I saw in test_concurrency but this is certainly much cleaner!

brettcannon · 2020-01-17T22:23:57Z

Lib/test/test_import/__init__.py

            finally:
-                del sys.modules['spam.x']
-
+                sys.modules['unwritable.x']


If this is delete the module then you can use a helper in test.support and register it with addCleanup().

Thanks, will do!

bedevere-bot · 2020-01-17T22:24:12Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

DinoV

I have made the requested changes; please review again.

brettcannon · 2020-01-22T19:36:32Z

Looks like Bedevere/GitHub hiccuped, so I manually updated the status for another review.

brettcannon

Two minor things to clean up, but I trust you to do that before you hit the green button. 😄

Lib/test/test_import/__init__.py

Misc/NEWS.d/next/Core and Builtins/2020-01-15-01-39-29.bpo-39336.nJ7W9I.rst

Co-Authored-By: Brett Cannon <[email protected]>

…36.nJ7W9I.rst Co-Authored-By: Brett Cannon <[email protected]>

Remove unnecessary change in _ready_to_import

Remove white space change

DinoV · 2020-01-23T00:40:18Z

Made one other small tweak to remove the change to _ready_to_import which was no longer necessary

bedevere-bot · 2020-01-23T00:42:42Z

@DinoV: Please replace # with GH- in the commit message next time. Thanks!

bedevere-bot · 2020-01-23T01:09:28Z

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

Hi! The buildbot x86 Gentoo Installed with X 3.x has failed when building commit 9b6fec4.

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/128/builds/181) and take a look at the build logs.
Check if the failure is related to this commit (9b6fec4) 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/128/builds/181

Failed tests:

test_import

Failed subtests:

test_unwritable_module - test.test_import.CircularImportTests

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

== Tests result: FAILURE then FAILURE ==

406 tests OK.

1 test failed:
test_import

13 tests skipped:
test_asdl_parser test_check_c_globals test_clinic test_devpoll
test_gdb test_ioctl test_kqueue test_msilib test_startfile
test_winconsoleio test_winreg test_winsound test_zipfile64

1 re-run test:
test_import

Total duration: 24 min 4 sec

Click to see traceback logs

TracebackTests) ... ok


Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/test/test_import/__init__.py", line 1347, in test_unwritable_module
    import test.test_import.data.unwritable as unwritable
ModuleNotFoundError: No module named 'test.test_import.data.unwritable'

pablogsal · 2020-01-23T01:17:44Z

Hummmm, seems that that buildbot failure is legitimate as this is the new test:

Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/test/test_import/__init__.py", line 1347, in test_unwritable_module
    import test.test_import.data.unwritable as unwritable
ModuleNotFoundError: No module named 'test.test_import.data.unwritable'

brettcannon · 2020-01-24T18:42:56Z

@DinoV I think you forgot to update the Makefile and Windows-related build files to add test/test_impor/data/unwritable directory to an installation.

…em (python#18006) * bpo-39336: Allow setattr to fail on modules which aren't assignable When attaching a child module to a package if the object in sys.modules raises an AttributeError (e.g. because it is immutable) it causes the whole import to fail. This now allows immutable packages to exist and an ImportWarning is reported and the AttributeError exception is ignored.

the-knights-who-say-ni added the CLA signed label Jan 14, 2020

bedevere-bot added the awaiting core review label Jan 14, 2020

DinoV force-pushed the bpo_39336_error branch 3 times, most recently from feb58e2 to 6e45545 Compare January 15, 2020 01:21

DinoV and others added 2 commits January 14, 2020 17:49

bpo-39336: Allow setattr to fail on modules which aren't assignable

78a9631

Fix whitespace Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

📜🤖 Added by blurb_it.

862b536

DinoV force-pushed the bpo_39336_error branch 2 times, most recently from 6e45545 to 862b536 Compare January 15, 2020 01:52

brettcannon self-requested a review January 15, 2020 18:35

brettcannon requested changes Jan 15, 2020

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Jan 15, 2020

Move test to data folder and improve error formatting

f2cfb2f

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 16, 2020

bedevere-bot requested a review from brettcannon January 16, 2020 18:43

brettcannon requested changes Jan 17, 2020

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Jan 17, 2020

move to using addCleanup, and import from test.test_import

245fd87

DinoV commented Jan 22, 2020

View reviewed changes

brettcannon self-requested a review January 22, 2020 19:35

brettcannon added awaiting change review and removed awaiting changes labels Jan 22, 2020

brettcannon approved these changes Jan 22, 2020

View reviewed changes

Lib/test/test_import/__init__.py Outdated Show resolved Hide resolved

Misc/NEWS.d/next/Core and Builtins/2020-01-15-01-39-29.bpo-39336.nJ7W9I.rst Outdated Show resolved Hide resolved

bedevere-bot added awaiting merge and removed awaiting change review labels Jan 22, 2020

DinoV and others added 4 commits January 22, 2020 16:15

Update Lib/test/test_import/__init__.py

d17e87a

Co-Authored-By: Brett Cannon <[email protected]>

Update Misc/NEWS.d/next/Core and Builtins/2020-01-15-01-39-29.bpo-393…

98dba44

…36.nJ7W9I.rst Co-Authored-By: Brett Cannon <[email protected]>

Update __init__.py

bbf8a94

Remove unnecessary change in _ready_to_import

Update __init__.py

1808b98

Remove white space change

DinoV merged commit 9b6fec4 into python:master Jan 23, 2020

DinoV deleted the bpo_39336_error branch January 23, 2020 00:42

bedevere-bot removed the awaiting merge label Jan 23, 2020

	msg = (f"Can't set child package '{child}' on "
	msg = (f"Cannot set an attribute on {parent!r} for child module {child!r}"

Uh oh!

bpo-39336: Allow packages to not let their child modules be set on them #18006

bpo-39336: Allow packages to not let their child modules be set on them #18006

Uh oh!

Conversation

DinoV commented Jan 14, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brettcannon Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

brettcannon Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

DinoV Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 15, 2020

Uh oh!

DinoV commented Jan 16, 2020

Uh oh!

bedevere-bot commented Jan 16, 2020

Uh oh!

brettcannon Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

DinoV Jan 22, 2020

Choose a reason for hiding this comment

Uh oh!

brettcannon Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

DinoV Jan 22, 2020

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 17, 2020

Uh oh!

DinoV left a comment

Choose a reason for hiding this comment

Uh oh!

brettcannon commented Jan 22, 2020

Uh oh!

brettcannon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DinoV commented Jan 23, 2020

Uh oh!

bedevere-bot commented Jan 23, 2020

Uh oh!

bedevere-bot commented Jan 23, 2020

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

Uh oh!

pablogsal commented Jan 23, 2020

Uh oh!

brettcannon commented Jan 24, 2020

Uh oh!

Uh oh!

DinoV commented Jan 14, 2020 •

edited by bedevere-bot

Loading