bpo-44771: Apply changes from importlib_resources 5.2.1

[jaraco]

• bpo-44771: Apply changes from importlib_resources@3b24bd6307
  • That rev is synced with importlib_resources v5.2.1.
  • Moves resources utilities from test_importlib.util to test_importlib.resources.util for easier synchronization.
• Add blurb

https://bugs.python.org/issue44771

Automerge-Triggered-By: GH:jaraco

[FFY00]

As I kind-of pointed out in python/importlib_resources#221 (comment), I think this implementation is fine to go in.

I will try to find out if there are any non-TraversableResources readers being used internally and, if so, see if they would need (more like benefit from) a specialized reader. This should hopefully uncover any issues that may arise from changing the importlib.resources implementation, but I think even if I find anything, it will probably be very minor.

👍

[jaraco]

The failing tests do seem to be implicated in the change. I'll investigate the cause later.

[jaraco]

The test failure on Windows was due to the introduction of a previously absent test for Namespace packages... and corresponding data in non-namespace packages had a special exclusion in .gitattributes, so I added the same exclusion for the namespace data.

[jkloth]

The change to .gitattributes will not trigger an update to already checked out files, so buildbots that have already have the eol converted files will continue to fail without manual intervention by the owners. A quick search indicates that short of actually modifying the affected files, a notice needs to be given to the buildbot owners about the change to .gitattributes.

Edit: I have already updated the namespace data files on my buildbot so it is now passing.

[jaraco]

...a notice needs to be given to the buildbot owners about the change to .gitattributes.

I did a quick search of the docs and neither "contact" nor ".gitattributes" appears in that page.

I don't have an established channel of communication to the buildbot owners, and it seems there isn't a published channel, so I'm sort-of forced to fall back to using "break the build" as my sole means of communication with them. Fortunately, that's what I've already done, so mission accomplished?

Joking aside, I really am at a loss. I guess I'll send an e-mail to python-dev and ask buildbot owners. I've filed bpo-44777 to track the need for a communications channel.

Now I've got to figure out what guidance to provide to the owners. Run a manual conversion on the file? Delete and re-clone the repo? I guess I'll just ask them to figure it out and propose some things I think might work.

I did some searching and found this recipe and confirms it works as advertised. I guess I'll recommend that.

I've also filed bpo-44779 to track the root cause (footgun) that made this failure possible.

[pablogsal]

Unfortunately, seems that this PR has broken several buildbots. For example:

https://buildbot.python.org/all/#/builders/350/builds/474

[jkloth]

Now I've got to figure out what guidance to provide to the owners. Run a manual conversion on the file? Delete and re-clone the repo? I guess I'll just ask them to figure it out and propose some things I think might work.

For me it was as simple as deleting the affected files and letting the normal build steps take care of the rest.

[pablogsal]

I don't have an established channel of communication to the buildbot owners, and it seems there isn't a published channel, so I'm sort-of forced to fall back to using "break the build" as my sole means of communication with them. Fortunately, that's what I've already done, so mission accomplished?

There is an official channel: https://mail.python.org/mailman3/lists/python-buildbots.python.org/

There is also the possibility of contacting release managers and python-dev, but breaking the build is not a recommended or acceptable practice because buildbot owners don't regularly check the outcome of the build but core devs do.

Unfortunately, this is against our policy on how to handle buildbot failures, so I am sadly being forced to revert this commit to avoid masking other builds and avoid breaking the test-with-buildbot labels

[FFY00]

@pablogsal I think we could just rename the affected files instead of reverting the whole PR.

[pablogsal]

@pablogsal I think we could just rename the affected files instead of reverting the whole PR.

The revert commit is there in case the situation is not fixed in 24h. If the buildbots are green in 24 hours, we won't need to revert the PR.

[FFY00]

Sure, I will open a PR atempting to fix this then.

[jkloth]

Simply having Git recognize a change to the files is enough to trigger the EOL filter.

Edit: SO answers seem to suggest a stat change may be enough (last-modified updated).

[FFY00]

That'd be more disruptive given the way the tests are written.

[FFY00]

Edit: SO answers seem to suggest a stat change may be enough (last-modified updated).

Oh, let's try.

[FFY00]

I don't think git can set that.

[jkloth]

If a quick stat change doesn't suffice, modifying the content of the files would work (and should be recommended practice for files that have their EOL attributed changed after initial commit).

[FFY00]

That'd be more disruptive given the way the tests are written.

We use the same contents for data01 and re-use the tests changing only the source, so we'd have to change it there too. I feel it's better to simply rename namespacedata01 to namespacedata given that we do not have any more namespace package data, it was just named that way to follow data01, which does 😛

[jkloth]

By changing the source, it could be as simple as committing one change, then committing another change which changes it back to the original. That is all that should be required to have Git re-run the EOL filters on the downstream hosts.

Edit: I'm looking at this problem from a recommended solution standpoint (which can be then included in the devguide), not necessarily from an expedient solution to this PR. Renaming may be the right solution, but, at times, Naming Is Hard, and the best name is the one already chosen, so it might not be the right solution.

[jaraco]

I don't even think it's necessary to rename the file - only to get the builldbots to reinitialize the file, so it may be sufficient just to create a PR that deletes the file, make sure all of the build bots attempt to build that revision, then when the buildbots go to build another revision, they'll get main with the .gitattributes installed. The only edge case there is that there are some branches already made prior to the .gitattributes being added, so if those branches are run, it could bring back the bad state.

[pablogsal]

Seems that this PR also introduced file descriptors leaks:

https://buildbot.python.org/all/#/builders/320/builds/107

OK (skipped=18, expected failures=1)
......
test_importlib leaked [1, 1, 1] file descriptors, sum=3
1 test failed again:
    test_importlib

[jaraco]

The refleak is occurring in test_path (removing test_path causes the refleak errors to go away). I suspect the issue is in how CompatibilityFiles adapts for path.

[jaraco]

I've isolated it to test_path.CommonTests.test_useless_loader:

cpython main $ git diff
diff --git a/Lib/test/test_importlib/test_path.py b/Lib/test/test_importlib/test_path.py
index 4436d7f34e..871c6d7a0c 100644
--- a/Lib/test/test_importlib/test_path.py
+++ b/Lib/test/test_importlib/test_path.py
@@ -11,6 +11,10 @@ def execute(self, package, path):
         with resources.path(package, path):
             pass
 
+    def test_useless_loader(self):
+        # Temporarily disable test as it's causing refleaks
+        return
+
 
 class PathTests:
     def test_reading(self):

cpython main $ ./python.exe Lib/test/regrtest.py -R : test_importlib
0:00:00 load avg: 2.18 Run tests sequentially
0:00:00 load avg: 2.18 [1/1] test_importlib
beginning 9 repetitions
123456789
.........
test_importlib passed in 38.5 sec

== Tests result: SUCCESS ==

1 test OK.

Total duration: 38.5 sec
Tests result: SUCCESS
cpython main $ git stash
Saved working directory and index state WIP on main: 48a62559df [bpo-44648](https://bugs.python.org/issue44648): Fix error type in inspect.getsource() in interactive session (GH-27171)
cpython main $ ./python.exe Lib/test/regrtest.py -R : test_importlib
0:00:00 load avg: 2.53 Run tests sequentially
0:00:00 load avg: 2.53 [1/1] test_importlib
beginning 9 repetitions
123456789
.........
test_importlib leaked [1, 1, 1, 1] file descriptors, sum=4
test_importlib failed (reference leak) in 38.0 sec

== Tests result: FAILURE ==

1 test failed:
    test_importlib

1 re-run test:
    test_importlib

Total duration: 38.0 sec
Tests result: FAILURE

[jaraco]

This refleak seems familiar, but when I search Github for it, I don't find it.

Looking at

cpython/Lib/importlib/_common.py

Line 92 in 48a6255

del reader

, I'm reminded that _tempfile had to be particularly aggressive about removing references to the input objects for some reason, possibly to avoid refleaks.

[jaraco]

Now I'm thinking the issue is unrelated to deleting the reader, but instead with the file descriptor, where if the reader raises an exception during:

cpython/Lib/importlib/_common.py

Line 90 in 48a6255

os.write(fd, reader())

Then the file descriptor created by mkstemp never gets closed. That may also be why the PermissionError occurred on Windows.

I've confirmed this patch fixes the refleak:

diff --git a/Lib/importlib/_common.py b/Lib/importlib/_common.py
index 74654b34ed..9b126f3174 100644
--- a/Lib/importlib/_common.py
+++ b/Lib/importlib/_common.py
@@ -87,14 +87,16 @@ def _tempfile(reader, suffix=''):
     # properly.
     fd, raw_path = tempfile.mkstemp(suffix=suffix)
     try:
-        os.write(fd, reader())
-        os.close(fd)
+        try:
+            os.write(fd, reader())
+        finally:
+            os.close(fd)
         del reader
         yield pathlib.Path(raw_path)
     finally:
         try:
             os.remove(raw_path)
-        except (FileNotFoundError, PermissionError):
+        except FileNotFoundError:
             pass

I'm going to apply that change to importlib_resources and see if the tests still pass there as well.

I suspect this bug was always present, but with the enhanced test coverage, that code path is now exercised in the test suite.

[jaraco]

Fix is released as importlib_resources 5.2.2 and #27497 applies that change to CPython.

[jaraco]

Tests are passing and the issues with buildbots appears to be addressed now:

[pablogsal]

Thanks a lot, @jaraco for the investigation and for the response. I have scheduled builds in the remaining red buildbots to check if everything is ok, but I assume it will be based on your comments.

[jaraco]

See #27497 for resolution (all but one buildbot succeeded. Failure unrelated).