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

qt5*: Enforce strict compatible version paths #65526

Merged

Conversation

samueldr
Copy link
Member

@samueldr samueldr commented Jul 28, 2019

Motivation for this change

Ensure we don't see the following error anymore:

Cannot mix incompatible Qt library (version 0x50c00) with this library (version 0x50c03)

This PR forces the path to use the minor version (5.major.minor) in the plugin paths, ensuring that it's improbable for Qt to look into an incompatible minor version's path for plugins. This builds upon discoveries made in #44047, see the end of the initial PR body.

This was tested on top of eb4e067, which has Qt 5.12.3. This was tested running on a 19.03 system which has 5.12.0-reliant software installed in the system.

This was verified using multimc (EDIT) With #65486 applied!

This was verified by using strace and comparing what it tries to load, and validating it loads the equivalent right library for 5.12.3.

(EDIT) Additionally, missing wrapQtAppsHook now fails more decisively at runtime, rather than using whatever traces could be found in the environment. This is both an advantage and a drawback. On one hand, it will force wrapQtAppsHook to happen to every Qt packages, ensuring they stay functional with mismatched versions. On the other hand, it will break a bunch of Qt apps at runtime, until every one of them is fixed. I think the advantage far outweighs the drawback.


So, there is at least an input method impurity...

This is intentional: we don't want for users to rebuild the world to change input methods. 🙁

Good news, Qt is able to load the right 5.12.3 input method by name, as I thought while experimenting.

 5008 19814 openat(AT_FDCWD, "/nix/store/gk8injsql81blxkxvxc0wf7qzz7b7p0b-qtbase-5.12.3-bin/lib/qt-5.12.3/plugins/platforminputcontexts/libibusplatforminputcontextplugin.so", O_RDONLY|O_CLOEXEC) = 7
 5009 19814 read(7, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\0\0\0\0\0\0\0\0"..., 832) = 832

And since Qt now looks for 5.12.3, instead of 5.12, in plugin paths, we don't have to care that much for the impure environment caused by PATH, as even if it's shock full of incompatible minor versions, Qt will not look for them.


Things done
  • ✔️ Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • ✔️ NixOS
    • 🔲 macOS
    • 🔲 other Linux distributions
  • 🔲 Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • 🔲 Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • 🔲 Tested execution of all binary files (usually in ./result/bin/)
  • 🔲 Determined the impact on package closure size (by running nix path-info -S before and after)
  • ✔️ Ensured that relevant documentation is up to date
  • ✔️ Fits CONTRIBUTING.md.

ref #65399
cc @FRidh who reverted the 5.12.3 bump
cc @ttuegel

@samueldr samueldr requested a review from ttuegel as a code owner July 28, 2019 20:27
@samueldr
Copy link
Member Author

Possible improvements:

  • Removing qtCompatVersion entirely.
  • Ensuring qtCompatVersion is composed only of 5.X.Y, as right now an hypothetical qtbase-5.x.y-1 could realistically cause 5.x.y-1 paths to be looked up into... which may not even be an issue in the end.

@ttuegel
Copy link
Member

ttuegel commented Jul 28, 2019

Additionally, missing wrapQtAppsHook now fails more decisively at runtime, rather than using whatever traces could be found in the environment.

We should just make these fail at build-time to save everyone a lot of headaches.

This is going to break the following use-case: the user has Qt applications installed in their user and system profiles, then they update the system but not user profile. The user profile applications will not be able to find input methods, themes, etc. anymore. This case works right now, as long as all the version numbers go in the right direction; it's admittedly fragile.

Here are my controversial reasons we may not care about this breakage:

  1. nix-env and user profiles were always a bad idea and effort expended supporting them is wasted.
  2. The only way to make Qt applications work 100% correctly under Nix is to build them in an FHS chroot and Flatpak them, and everything is going to be broken for somebody until we do that anyway. (This is an exaggeration: we could just rewrite dlopen() instead.)

@samueldr
Copy link
Member Author

samueldr commented Jul 28, 2019

The user profile applications will not be able to find input methods, themes, etc. anymore.

At least for the input method, I found out that Qt is able to resolve the one I use. I assume, though, it may not end up being true for all of them, and for all things.

(This is an exaggeration: we could just rewrite dlopen() instead.)

I don't think it'd be enough; I think any file access would need to be checked. Isn't there more than .so resources that can be loaded, like QML resources and such? (Though I'm not sure.)


Here are my controversial reasons we may not care about this breakage:

  1. nix-env and user profiles were always a bad idea and effort expended supporting them is wasted.
  2. The only way to make Qt applications work 100% correctly under Nix is to build them in an FHS chroot and Flatpak them, and everything is going to be broken for somebody until we do that anyway. [...]

I'm still waiting on the controversial bit. I much rather we decisively break under those circumstances, than have almost working, but brittle solutions. It's not ideal to break what seems to be working now, but in the end it should end up being easier on end-users. I'm not even sure that nix-env will break that much, from the limited testing I did.

@ttuegel
Copy link
Member

ttuegel commented Jul 28, 2019

At least for the input method, I found out that Qt is able to resolve the one I use. I assume, though, it may not end up being true for all of them.

It looks like you are using I-Bus, right? I-Bus is built into Qt. I would be very impressed if you broke it 😀 I would not have a problem if we wanted to say "I-Bus is the only officially-supported input method for Qt," but we have others (at least fcitx) in Nixpkgs.

I don't think it'd be enough; I think any file access would need to be checked. Isn't there more than .so resources that can be loaded, like QML resources and such? (Though I'm not sure.)

Qt only detects version mismatches in the shared object files. The mismatch only occurs because of the order that dlopen searches for dependencies. Basically, plugin shared objects get the opportunity to override the RUNPATH of the process that's loading them, and that's why you get a mismatch. If dlopen processed search paths in the opposite order, the plugin would be forced to use the application's library versions—this is OK because Qt maintains strict ABI compatibility rules for plugins, and it's actually how it works correctly on other distros. But you're right, the only way to be strict about compatibility is to rewrite the kernel (or at least libc) or use a chroot solution like Flatpak.

@teto
Copy link
Member

teto commented Jul 29, 2019

this PR plus my changes to wireshark (to use the new Qt mechanism) fixed my issue.

@ttuegel
Copy link
Member

ttuegel commented Jul 30, 2019

Should we change this to go into staging, due to the size of rebuild?

@samueldr samueldr force-pushed the feature/qt-strict-compatible-versions branch from f61beba to 6cb99e7 Compare July 31, 2019 00:33
@samueldr
Copy link
Member Author

(Oops, I tried to use the git merge-base trick to not ping people, but accidentally used staging when rebasing instead of the merge-base)


@ttuegel the commits are now rebased on the merge-base of staging/master; it can be switched as needed.

@samueldr samueldr changed the base branch from master to staging July 31, 2019 00:36
@peterhoeg
Copy link
Member

Considering Qt says:

Patch releases are both backwards and forwards binary and source compatible.

Why is this even an issue in the first place?

@samueldr
Copy link
Member Author

samueldr commented Jul 31, 2019

Sorry for the totally unprofessional response, but ¯\_(ツ)_/¯

I think @ttuegel has a better handle on the explanation for that.

It may be that specifics to NixOS and the way it load both Qt versions at the same time is the issue, if it loaded any one (and only one) of the patch-level versions at once, I guess it would be fine? Though, as I initially stated, I don't exactly know. I'm working off of evidence that (1) it's an issue with Qt as packaged withing Nixpkgs, (2) it demonstrably fixes the issue with this PR and (3) missing the wrapper decisively breaks instead of attempting to use a compatible other-patchlevel Qt from the environment.

@peterhoeg
Copy link
Member

I'm not at all disputing that there is an issue - and this does seem to be an elegant way to fix it. I just can't help think that there's something we're missing.

@samueldr
Copy link
Member Author

Oh, I wasn't aggravated or anything. I, too, am wondering the exact reasons this acts as it does. I would rather know the details to better understand the issue, than using only the observed effects.

@ttuegel
Copy link
Member

ttuegel commented Aug 2, 2019

Why is this even an issue in the first place?

Although applications and plugins are compatible across patch-level versions, all of the parts of Qt itself need to be of the same version exactly. So, if an application compiled with Qt 5.12.3 tries to load a plugin compiled with Qt 5.12.0, its RUNPATH will be contaminated and it might load mis-matched Qt libraries. This isn't an issue with other distributions because they build the entire Qt installation at once and they have one canonical path (/usr/lib) where all the latest libraries are found.

Another way we could deal with this problem is to compile a monolithic Qt (so there is one canonical path where the Qt libraries are) and use patchelf to remove that path from the RUNPATH of other libraries and plugins, effectively delegating linking to the application. I didn't want to do this because it's not the Nixpkgs Way, even though it is what dynamic linking is for.

@ttuegel ttuegel merged commit 421782e into NixOS:staging Aug 2, 2019
@samueldr samueldr deleted the feature/qt-strict-compatible-versions branch August 2, 2019 20:39
@ttuegel ttuegel mentioned this pull request Aug 7, 2019
10 tasks
@alxpettit
Copy link

I'm getting this problem in 2023, for when I use stable QT, but try to use cura and krita from unstable via flake. :/

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

Successfully merging this pull request may close these issues.

None yet

5 participants