plugins/thumbnails: fix FFI with GIO on s390x #5708
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
2c6c932 to
7f1eb66
Compare
There was a problem hiding this comment.
Wow, that copy-and-paste error (I presume) seems to have been around forever (10 years, to be precise: 530b421); thanks for the fix 🎉
Given https://docs.gtk.org/gio/type_func.File.new_for_path.html, this appears to be correct.
Discuss the behavior in case a string is passed. I've currently updated the test to reflect the exception being raised, but I did not do a thorough check of the code to know if that exception would need to be caught elsewhere in the code, or if that API might be used externally.
While undocumented, unchecked and untyped (and I'm unfamiliar with this part of the code), I'm fairly confident that this is only expected to take bytestrings. That's (historically) the internal representation of paths that beets chooses (with the caveat that I'm quite sure that was never 100% correctly implemented). Additionally, with the previous code, ctypes would pass the argument as wchar_t *, which presumably ended up being interpreted as b"\\\0" on little-endian systems (and the empty string on big-endian systems, which might explain the test failures). In any case, the result would definitely be broken thumbnails (or rather failure to even write them).
So, the only sense in which this could regress seems to be by different failures modes, in particular during import: Logging a warning/silent failure due to the broken code (before) and a crash with ArgumentError (after).
Given that the thumbnail plugin seems to be essentially unmaintained (the only contributions since its inception seem to be small bug fixes by drive-by contributors, and repo-wide refactorings), I'd argue to just merge this and deal with potential fall-out later. @snejus @JOJ0 any objections?
Eventually, it would be nice if the thumbnails plugin would also be subject to typing (slowly, beets is getting type annotations throughout its codebase) to highlight incorrect usage of these functions.
I'm also wondering whether gio is even required here these days: From a quick glance at the PR which added it, there used to be some discrepancy between Python and glib, but that's really a long time ago. Maybe, the much simpler pathlib implementation is sufficient nowadays?
|
Would you mind adding a quick changelog entry? |
7f1eb66 to
db671b6
Compare
|
Thanks for the review, and the hindsight on that little-endian/big-endian thing. That was more or less my conclusion too, but I really don't know enough about these things to be confident in a clear explanation. I've added an entry to the changelog, hope it's good enough :-) |
Using the correct function signature for g_file_new_for_path fixes the
tests on s390x.
I do not have the full story on why this failed consistently only on
s390x, but I guess the big endian might have something to play with
this.
Here is how the tests were failing:
```
169s ___________________________ ThumbnailsTest.test_uri ____________________________
169s
169s self = <test.plugins.test_thumbnails.ThumbnailsTest testMethod=test_uri>
169s
169s def test_uri(self):
169s gio = GioURI()
169s if not gio.available:
169s self.skipTest("GIO library not found")
169s
169s > assert gio.uri("/foo") == "file:///" # silent fail
169s E AssertionError: assert '' == 'file:///'
169s E
169s E - file:///
169s
169s test/plugins/test_thumbnails.py:268: AssertionError
```
You can see a full log here [1] and a history of consistent failure
here [2]. Both links are bound to expire at some point, sorry future
archeologist 🤷.
[1]: https://autopkgtest.ubuntu.com/results/autopkgtest-plucky/plucky/s390x/b/beets/20250403_162414_5d1da@/log.gz#S5
[2]: https://autopkgtest.ubuntu.com/packages/beets/plucky/s390x
060137a to
f4de44f
Compare
Description
Using the correct function signature for g_file_new_for_path fixes the tests on s390x.
I do not have the full story on why this failed consistently only on s390x, but I guess the big endian might have something to play with this.
Here is how the tests were failing:
You can see a full log here 1 and a history of consistent failure here 2. Both links are bound to expire at some point, sorry future archeologist 🤷.
To Do