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.

Sign up for GitHub

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

Jump to bottom

Add missing Plover deps #71335

Merged
merged 3 commits into from Oct 21, 2019
Merged

Add missing Plover deps #71335

merged 3 commits into from Oct 21, 2019

Conversation

edef1c
Copy link
Member

@edef1c edef1c commented Oct 18, 2019

Motivation for this change

This fixes plover.dev crashing at startup due to missing pkg_resources and qtbase, and adds the optional dbus-python dependency to plover.stable.

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.
Notify maintainers

cc @Twey @KoviRobi

@edef1c
plover.stable: add missing dbus dependency
0f93c22
@edef1c
plover.dev: add missing setuptools dependency
b954169 
Without this, it crashes instantly at startup looking for
pkg_resources.
@ofborg ofborg bot requested a review from KoviRobi October 18, 2019 12:27
@KoviRobi
Copy link
Contributor

KoviRobi commented Oct 18, 2019 •
edited

  • plover.stable: add missing dbus dependency verified, it does indeed look for dbus and cannot find it.
  • plover.dev: add missing setuptools dependency. It is indeed a runtime dependency on setuptools, seems fine
  • plover.dev: apply wrapQtApp doesn't seem to work for me, when cherry-picked onto nixpkgs-19.09 (not sure if something changed on master, will try but it probably requires me to compile stuff) 
    wrapping `/nix/store/8pc09hxqw1rm7l1iqcypaza8w7ca50js-python3.6-plover-4.0.0.dev8/bin/plover'...
/nix/store/7skkd4n1kmkl9dmql1219fqwxbjcas81-stdenv-linux/setup: line 100: wrapQtApp: command not found
builder for '/nix/store/bbw7c8674mcsy3d5mgigx9558y7jvaii-python3.6-plover-4.0.0.dev8.drv' failed with exit code 127
error: build of '/nix/store/bbw7c8674mcsy3d5mgigx9558y7jvaii-python3.6-plover-4.0.0.dev8.drv' failed
    Edit: Actually, that fails on ofborg so probably not just me, I guess you need qtbase?

@KoviRobi
Copy link
Contributor

After looking at openshot-qt, I have some success with KoviRobi@a41f4a7 but the program seems to be double wrapped, perhaps it's worth looking at rapid-photo-downloader? Also #65399 seems relevant

@KoviRobi KoviRobi mentioned this pull request Oct 18, 2019
Closed
@KoviRobi
Copy link
Contributor

Also, and this is my mistake with 5a06658 but we should probably use buildPythonApplication not buildPythonPackage. Not sure if it should be a different PR or not.

worldofpeace
worldofpeace suggested changes Oct 18, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

@KoviRobi Is correct in that you need to use KoviRobi@a41f4a7. The issue was wrapQtAppsHook wasn't added, and mkDerivationWith will add this automatically to nativeBuildInputs.

Also correct in that, we should be using buildPythonApplication and not buildPythonPackage. Unless it really is being used as a python module.

Lastly, we have pinned python version's here (specifically python36Packages).
Is this strictly needed? Or can we use python2Packages && python3Packages.

@edef1c
plover.dev: apply the Qt wrapper
3b0bc63 
Without this, it crashes instantly at startup looking for the
Qt xcb plugin.

Co-authored-by: Kovacsics Robert <rmk35@cam.ac.uk>
worldofpeace
worldofpeace approved these changes Oct 20, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

LGTM, though I'm not sure if it's my setup but I cannot start it (dev version)

DisplayConnectionError: Can't connect to display ":0": No protocol specified

Gnome 3 x11

@edef1c
Copy link
Member Author

edef1c commented Oct 20, 2019 

DisplayConnectionError: Can't connect to display ":0": No protocol specified

I wrote a little Perl script to check if there were any environment variables that affected it. I could reproduce this by running it with rm -rf /tmp/home && mkdir /tmp/home && env -i DISPLAY=:0 HOME=/tmp/home result/bin/plover, but not if I added cp ~/.Xauthority /tmp/home into the mix. I'm not sure how to reproduce this on anything remotely resembling a normal system.

worldofpeace
worldofpeace approved these changes Oct 21, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

Couldn't reproduce aforementioned issue in a vm.
I believe my system's Xauthority is broken somewhat.

@worldofpeace worldofpeace merged commit 1a25b14 into NixOS:master Oct 21, 2019
@edef1c edef1c deleted the plover-deps branch October 21, 2019 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@worldofpeace worldofpeace worldofpeace approved these changes

@KoviRobi KoviRobi Awaiting requested review from KoviRobi

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@edef1c @KoviRobi @worldofpeace