| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
👍
Sorry, something went wrong.
|
The failing tests do seem to be implicated in the change. I'll investigate the cause later. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
Unfortunately, seems that this PR has broken several buildbots. For example: |
Sorry, something went wrong.
For me it was as simple as deleting the affected files and letting the normal build steps take care of the rest. |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
|
@pablogsal I think we could just rename the affected files instead of reverting the whole PR. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
Sure, I will open a PR atempting to fix this then. |
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
|
That'd be more disruptive given the way the tests are written. |
Sorry, something went wrong.
Oh, let's try. |
Sorry, something went wrong.
|
I don't think git can set that. |
Sorry, something went wrong.
This should fix the buildbots, see python#27436 (comment) Signed-off-by: Filipe Laíns <lains@riseup.net>
|
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). |
Sorry, something went wrong.
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 😛 |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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
|
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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
|
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
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 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:
passI'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. |
Sorry, something went wrong.
|
Fix is released as importlib_resources 5.2.2 and #27497 applies that change to CPython. |
Sorry, something went wrong.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
See #27497 for resolution (all but one buildbot succeeded. Failure unrelated). |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
https://bugs.python.org/issue44771
Automerge-Triggered-By: GH:jaraco