Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PICARD-2953: Use strxfrm for sorting on Windows again #2537

Merged
merged 1 commit into from
Aug 24, 2024

Conversation

phw
Copy link
Member

@phw phw commented Aug 24, 2024

Summary

  • This is a…
    • Bug fix
    • Feature addition
    • Refactoring
    • Minor / simple change (like a typo)
    • Other
  • Describe this change in 1-2 sentences:

Problem

On Windows QCollator.sortKey is broken and results of wrong ordering of numbers, both in numeric and normal alphabetic mode.

This results in both the below conditions failing unexpectedly:

assert(i18n.sort_key("1") < i18n.sort_key("C"))
assert(i18n.sort_key("99", numeric=True) < i18n.sort_key("100", numeric=True))

See also QTBUG-128170.

Solution

Use locale.strxfrm for sorting on Windows again. We replaced this with QCollator in PICARD-2914 due to strxfrm segfaulting on macOS frequently. But since we did not get the crashes on Windows it seems safe to move back to strxfrm for Windows again.

On Windows QCollator.sortKey is broken and results of wrong ordering of
numbers, both in numeric and normal alphabetic mode.
@phw phw requested a review from zas August 24, 2024 15:04
@zas zas changed the title PICARD-2953: PICARD-2953: Use strxfrm for sorting on Windows again Aug 24, 2024
Copy link
Collaborator

@zas zas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hacky stuff, but no choice, let's hope it will be fixed upstream at some point.

@phw phw merged commit 7826f94 into metabrainz:master Aug 24, 2024
44 checks passed
@phw phw deleted the PICARD-2953-fix-windows-sorting branch August 24, 2024 15:52
@Sophist-UK
Copy link
Contributor

Cripes - what a stupid situation.

The net result of this is:

  1. Alternative code paths depending on O/S - one of the key features of Qt is that it is supposed to prevent this.
  2. On Mac it no longer crashes but there are all sorts of sorting issues.

@phw
Copy link
Member Author

phw commented Aug 24, 2024

2. On Mac it no longer crashes but there are all sorts of sorting issues.

It's not so bad. I'm not aware of any existing sorting issues on macOS, and I tested this quite thoroughly with the change in #2533 .

The big surprise was that the changes of using QCollator, which seemed like a nice platform independent solution without introducing any new dependency and which worked all great on Linux and macOS, broke the sorting on Windows so badly.

The core issue here seems to be that Qt can use libicu, but also can be built without libicu and then uses platform dependent code. It does so on macOS, but also on Windows and the implementation there is quite badly broken.

So another solution would be to do our own Qt builds and build with libicu on all platforms. This is actually what someone suggested on QTBUG-128170. But this then also involves building libicu itself, and PyICU and PyQt5|6 and likely a bunch of additional dependencies. And building for at least two platforms (macOS and Windows) for which we lack developers and testers. This just calls for a bunch of platform dependent issues with the builds.

Essentially I take care of the macOS and Windows builds currently, and I try to debug all the platform dependent trouble we already have right now. And I neither use macOS or Windows actively.

In theory there might be some advantage of taking control over the full build (probably would include Python then also). We can tweak everything to our needs, and react on security issues flexibly. E.g. when there was this webm vulnerability we applied workarounds (disabling webm until the binary builds inlcuded the fix). With custom builds we could have had a proper fix. But as it currently stands I have zero interest in maintaining this build pipeline and dealing with the unavoidable bug hunting this would cause. Not with someone else taking up a big part of the work involved.

@Sophist-UK
Copy link
Contributor

Ah - so aside from a little bit of added complexity it works correctly on all platforms without crashing?

@phw
Copy link
Member Author

phw commented Aug 25, 2024

@Sophist-UK yes, exactly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants