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

Wrap Qt applications #54525

Merged
merged 3 commits into from Jul 5, 2019
Merged

Conversation

ttuegel
Copy link
Member

@ttuegel ttuegel commented Jan 24, 2019

Introduces wrapQtAppsHook similar to wrapGAppsHook to solve long-standing problems with Qt applications, particularly plugin search paths. Wrapped Qt applications can run directly from the Nix store without being installed and using nix-shell --pure. (Actually, applications that use OpenGL on NixOS cannot run from pure Nix shells because of Mesa; this is solved by setting LD_LIBRARY_PATH for the OpenGL drivers. I did not address this here because it is not a Qt problem.)

This does not apply the hook to every application. My intent is that package maintainers can make those changes can be merged to master. Ultimately, the burden of interactive testing will fall on them anyway. This is just the changes to Qt necessary to make that possible. Ark and Okular are wrapped, as proof-of-concept.

This also does not remove the patches from Qt that infers plugin and QML paths from PATH; that is still necessary for desktop environments (particularly themes) to work as expected. I worked on this pull request for more than a year, but I am still no closer to getting rid of that patch. Finally I decided to rip out everything but the simplest version of the wrapper. For the time being, we can use both the wrapper and the patch together; the latter is inelegant, but everyone's applications will work.

This would close #44047, as it addresses the same issue, but more completely: it solves problems with QT_PLUGIN_PATH, QML2_IMPORT_PATH, XDG_DATA_DIRS and others.

Closes: #53750, #44047, #51597, #42864 (probably others)

TODO: closure size analysis and documentation

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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@matthewbauer
Copy link
Member

matthewbauer commented Jan 25, 2019

I kind of thought that was an intentional design decision in qt infra to allow different qt runtimes…

This goes back to the debate on wrapping vs propagatedUserEnvPkgs. Wrapping seems like a simpler way to do things to me, but I’m not sure if everyone agrees. See discussion on this issue by @edolstra:

#43049 (comment)

@ttuegel
Copy link
Member Author

ttuegel commented Jan 31, 2019

I kind of thought that was an intentional design decision in qt infra to allow different qt runtimes

Dynamically loading Qt plugins at runtime is intentional, and this pull request does not alter that. This pull request addresses the fact that it is much, much too easy to mix Qt versions--which always ends badly.

@danbst
Copy link
Contributor

danbst commented Feb 1, 2019

@ttuegel would this help also for proprietary binaries, where Qt is patched into binary with autoPatchelfHook? (a recent example #54969 (comment))

@ttuegel
Copy link
Member Author

ttuegel commented Feb 2, 2019

@danbst This alone would not help the situation with binary distributions, but it would help not to put everything into propagatedUserEnvPkgs and this would let us do that.

@peterhoeg
Copy link
Member

Is there any reason we don't make wrapQtAppsHook part of the KDE specific mkDerivation and apply it unconditionally to avoid having to touch everything in kdeApplications?

@flokli flokli mentioned this pull request Feb 5, 2019
@flokli
Copy link
Contributor

flokli commented Feb 5, 2019

@peterhoeg this still won't fix packages outside KDE world, and there's a lot of QT applications out there - so I really prefer the approach in #44047 not requiring any wrapper.

@samueldr
Copy link
Member

samueldr commented Feb 5, 2019

Sorry if I sound grumpy or mad, but it's not my intention in the comment. Read the following in a neutral voice.

I may be out of line here (and definitely biased), but "similar to wrapGAppsHook" will mean that we're going to have to deal with the current assortment of failures in a less predictable fashion, just like #54278, where currently a bunch of mismatched gdk_pixbuf work, and a bunch fail.

Fixing the issues in the libraries directly would ensure no one has to remember to use that one hook on their software, and fixes the issue closer to the source, instead of papering over. The main issue being that most of the time not adding the hook works fine. There wouldn't be much of a developer experience (DX?) issue if it failed noisily in all circumstances when the hook is missing.

I have one concession to make to those wrappers: they require much less effort to maintain. Solving this particular issue at the root isn't really intrusive, and relatively self-contained, but I guess it's half part to good software and half part to luck.

@ttuegel
Copy link
Member Author

ttuegel commented Feb 9, 2019

Is there any reason we don't make wrapQtAppsHook part of the KDE specific mkDerivation and apply it unconditionally to avoid having to touch everything in kdeApplications?

@peterhoeg I haven't tried that, but it should be OK.

EDIT: For that matter, I don't see any reason not add wrapQtAppsHook to qt5.mkDerivation.

@ttuegel
Copy link
Member Author

ttuegel commented Feb 9, 2019

The main issue being that most of the time not adding the hook works fine. There wouldn't be much of a developer experience (DX?) issue if it failed noisily in all circumstances when the hook is missing.

@samueldr
I agree that a solution which requires no developer intervention--or is at least noisy about needing developer intervention--provides a better experience. Despite that, a number of Qt application maintainers are already rolling their own wrappers. I realized--thanks to @peterhoeg's remark above--that we could simply include the hook in mkDerivation; would that satisfy?

@peterhoeg
Copy link
Member

#44047 is a lot more invasive and I for one simply don't know enough about Qt internals to be able to say anything remotely intelligent about the viability of that approach.

This PR (with the qt5.mkDerivation variant) will solve the first 3 out of the following 4.

  • KDE applications in nixpkgs
  • Qt applications in nixpkgs
  • Qt applications out of tree
  • Binary Qt applications

Why don't we move ahead with this for now while working on #44047?

@timor
Copy link
Member

timor commented Feb 25, 2019

Thanks for the effort! I just recently thought of trying the exact same thing before I found this issue :).

@matthewbauer
Copy link
Member

@ttuegel pinging on this. Hoping we can get this in soon as it closes quite a few other issues around this.

delroth added a commit to delroth/nixpkgs that referenced this pull request Jul 20, 2019
Similar to pkgs.bitcoin, QT_PLUGIN_PATH is required in checkPhase to run
the "testcli" test (ironically).
pull bot pushed a commit to milibopp/nixpkgs that referenced this pull request Jul 20, 2019
@OPNA2608 OPNA2608 mentioned this pull request Jul 24, 2019
10 tasks
@jokogr jokogr mentioned this pull request Jul 28, 2019
10 tasks
mmahut added a commit to mmahut/nixpkgs that referenced this pull request Jul 28, 2019
Regression introduced in NixOS#54525, tracked in NixOS#65399.
@mmahut mmahut mentioned this pull request Jul 28, 2019
10 tasks
xaverdh added a commit to xaverdh/nixpkgs that referenced this pull request Jul 31, 2019
Use mkDerivation has instead of stdenv.mkDerivation (see NixOS#54525).
emmanuelrosa pushed a commit to emmanuelrosa/nixpkgs that referenced this pull request Aug 4, 2019
This commit fixes the error:

Could not find the Qt platform plugin "xcb" in ""

See NixOS#54525 and NixOS#56921
@edolstra
Copy link
Member

This caused significant closure size increases, see #69272.

@edolstra
Copy link
Member

To be honest the wrapQtApp approach (generally turning most build-time dependencies into runtime dependencies) seems fundamentally wrong to me. So we end up with cmake, pkgconfig, patchelf, flex, bison and dozens of -dev outputs in the runtime closure just because they happen to have a /share subdirectory.

Possibly better approach: only add those packages to XDG_DATA_DIRS that are already in the runtime closure.

@ttuegel
Copy link
Member Author

ttuegel commented Sep 23, 2019

To be honest the wrapQtApp approach (generally turning most build-time dependencies into runtime dependencies) seems fundamentally wrong to me.

The wrapper hook is designed only to pick up host dependencies, so I really don't understand why nativeBuildInputs are being captured.

As far as Qt is concerned, the correct solution is to comply with FHS so that resources have well-known canonical names, but that's not an option here.

Possibly better approach: only add those packages to XDG_DATA_DIRS that are already in the runtime closure.

I think it is unlikely that this would work because the Qt/KDE folks strongly prefer dynamic dependencies, but for the sake of argument, how can we detect during the build what packages are in the runtime closure?

@edolstra
Copy link
Member

The wrapper hook is designed only to pick up host dependencies

That doesn't prevent capturing -dev outputs, right?

Anyway, a short-term fix might be:

  • Filter out *-dev.
  • Be more selective than including every store path that has a /share subdirectory, but instead check for the actual paths that the app requires (e.g. /share/applications or /share/mime).

how can we detect during the build what packages are in the runtime closure?

Probably looking at the RPATH (patchelf --print-rpath) is a decent approximation.

@ttuegel
Copy link
Member Author

ttuegel commented Sep 23, 2019

That doesn't prevent capturing -dev outputs, right?

It prevents capturing build tools like cmake, and this:

Be more selective than including every store path that has a /share subdirectory, but instead check for the actual paths that the app requires (e.g. /share/applications or /share/mime).

We already select specific required paths in the setup hook, which should also eliminate -dev outputs.

Probably looking at the RPATH (patchelf --print-rpath) is a decent approximation.

This will break at least packages that: use plugins, use QML, or have kservice (D-Bus) dependencies.

Filter out *-dev.

This seems like a reasonable course of action.

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