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

anki: use wrapQtAppsHook to fix execution #66796

Merged
merged 4 commits into from Aug 26, 2019

Conversation

@oxij
Copy link
Contributor

commented Aug 17, 2019

What?

At the moment running anki fails with

qt: Could not find the Qt platform plugin "xcb" in ""
qt: This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

This PR fixes it, properly, by inheriting the hook from the pyqtwebengine so that QT versions match.

Why?

The use of wrapQtAppsHook is now required for Qt apps. See #65399 for more info.

git log

  • pyqtwebengine: passthru wrapQtAppsHook

  • anki: use wrapQtAppsHook from pyqtwebengine

  • anki: add myself as a maintainer

    (Adding to the front of the list because it was I who added this expression to
    nixpkgs in e00c031 7 years ago. :])

nix-instantiate environment

  • Host OS: Linux 4.9, SLNOS 19.09
  • Nix: nix-env (Nix) 2.2.2
  • Multi-user: yes
  • Sandbox: yes
  • NIXPKGS_CONFIG:
{
  checkMeta = true;
  doCheckByDefault = true;
}

nix-env -qaP diffs

  • On x86_64-linux:
    • Updated (1):
      • anki
  • On aarch64-linux: noop
  • On x86_64-darwin:
    • Updated (1):
      • anki

/cc @Profpatsch @asymmetric @ttuegel

oxij added 3 commits Aug 17, 2019
(Adding to the front of the list because it was I who added this expression to
nixpkgs in e00c031 7 years ago. :])
@oxij oxij requested a review from FRidh as a code owner Aug 17, 2019
@ofborg ofborg bot added the 6.topic: python label Aug 17, 2019
@oxij

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

@oxij

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

@flokli

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

@@ -158,6 +161,9 @@ buildPythonApplication rec {
cp -rv anki aqt web $pp/
wrapPythonPrograms
for program in $out/bin/*; do

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Aug 18, 2019

Member

Why do we have to do wrapPythonPrograms in buildPythonApplication? I thought that'd happen automatically.

Also by doing this we get two wrappers, maybe give makeWrapperArgs "\${qtWrapperArgs[@]}"?

@asymmetric

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

From a quick run, this works as expected.

@symphorien

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

I think "keeping the version of qt in qtwebengine and anki in sync" is the job of libsForQt5.callPackage.

@oxij

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

# copy the manual into $doc
cp -r ${manual}/share/doc/anki/html $doc/share/doc/anki
'';

dontWrapQtApps = true;
makeWrapperArgs = [
''--prefix PATH ':' "${lame}/bin:${mplayer}/bin"''

This comment has been minimized.

Copy link
@asymmetric

asymmetric Aug 18, 2019

Contributor

Should we use the same string type for both array elements?

Suggested change
''--prefix PATH ':' "${lame}/bin:${mplayer}/bin"''
"--prefix PATH ':' ${lame}/bin:${mplayer}/bin"
@worldofpeace

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

@oxij How about calling anki with libsForQt5.callPackage instead of python3Packages?
You'd then have to use python3Packages directly in the expression, but you wouldn't have to use a passthru attribute in pyqtwebengine

@oxij

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

@ttuegel

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

I would rather not do that either until anki needs a lot, A LOT, of that passthru stuff.

I agree that the passthru attribute is probably the least-bad way to handle a one-off problem like this. My only reservation is that someone finds this example first and thinks it's the preferred solution, and then it proliferates. @oxij would you add a comment where the passthru is added that says something like, "this is a one-off solution for anki", so that it makes sense to someone looking at it out of context? Then I am happy to merge.

@flokli

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

@ttuegel isn't that the preferred/only solution for all python applications using some qt bindings in one way or another? In that case, it probably should be added to the documentation…

@ttuegel

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

@flokli If every Python package is going to start using this, then no: it is not the preferred solution and we need to actually solve the problem.

@oxij

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

@timokau

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

Can we merge this now? I have no opinion on the meta discussion, but for now it would be nice to have anki working on unstable again.

@timokau

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Given that it has been a few days without new discussion and this PR is a definite improvement for now, I'll merge. Thanks @oxij for taking care of this!

@timokau timokau merged commit 09cc908 into NixOS:master Aug 26, 2019
14 of 15 checks passed
14 of 15 checks passed
anki on aarch64-linux No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
anki on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@Profpatsch

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

Great, thanks @oxij

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.