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

Jump to bottom

Fix virt-manager on non-gnome systems #64627

Merged
merged 6 commits into from Jul 13, 2019
Merged

Fix virt-manager on non-gnome systems #64627

merged 6 commits into from Jul 13, 2019

Conversation

aske
Copy link
Member

@aske aske commented Jul 11, 2019

Motivation for this change
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.

@abbradar
Copy link
Member

@GrahamcOfBorg eval
@GrahamcOfBorg build virtmanager

worldofpeace
worldofpeace reviewed Jul 11, 2019
View reviewed changes
pkgs/development/libraries/spice-gtk/default.nix Outdated
@@ -87,10 +87,13 @@ stdenv.mkDerivation rec {
vala
];

propagatedBuildInputs = [
gst_all_1.gst-plugins-base
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a reason why this needed to be propagated. Perchance explain?

Copy link
Member Author

Choose a reason for hiding this comment

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

When SpiceGtkClient is loaded via GI it also needs GStreamer and cannot find it because needed paths are not in GI_TYPELIB_PATH. I was able to see the cause only after patching virtManager/details/viewer.py to show all exceptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this only required if spice is enabled? I thin it ahould the same as here, at least applying that change worked for me:

nixpkgs/pkgs/applications/virtualization/virt-viewer/default.nix

Line 31 in 7e71cd8

] ++ optionals spiceSupport [

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh nevermind, I can see that you patched spice-gtk directly, so should we remove these additional dependencies also in virt-viewer?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, gst-plugins-base could be removed, though there's also gst-plugins-good.
I'll add a commit after checking if it is needed.

@worldofpeace
Copy link
Contributor

worldofpeace commented Jul 11, 2019 •
edited

This appears to be slightly expanded from #64618

Needs to be confirmed if this fixes #62647

@aske
Copy link
Member Author

aske commented Jul 11, 2019

Oh, thanks, missed that one.
virt-manager now works fine for me after these changes, previously I saw the same errors as in #62647.

@abbradar
Copy link
Member

Can confirm that fixes the issue, thanks!

@worldofpeace
Copy link
Contributor

worldofpeace commented Jul 11, 2019 •
edited

When SpiceGtkClient is loaded via GI it also needs GStreamer and cannot find it because needed paths are not in GI_TYPELIB_PATH. I was able to see the cause only after patching virtManager/details/viewer.py to show all exceptions.

I think we're preferring, in this case, to make the typelibs available within it by hardcoding them with
a patch to the source code.

@aske
Copy link
Member Author

aske commented Jul 11, 2019

I'm not sure if this is possible given GObject Introspection dynamic nature, any ideas how it should be done?

@emmanuelrosa
Copy link
Contributor

I use virt-manager with the i3 window manager, on NixOS. I confirm that this pull requests fixes two issues. I can once again:

  1. Use Spice (instead of VNC) to connect to VMs
  2. Open VMs; to edit and/or run them.

Thank you for this fix :)

astsmtl
astsmtl reviewed Jul 12, 2019
View reviewed changes
pkgs/applications/virtualization/virt-manager/default.nix Outdated
@@ -25,7 +26,7 @@ python3Packages.buildPythonApplication rec {

buildInputs = [
libvirt-glib vte dconf gtk-vnc gnome3.adwaita-icon-theme avahi
gsettings-desktop-schemas libosinfo gtk3
gsettings-desktop-schemas libosinfo gtk3 gtksourceview4 librsvg
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need librsvg here?

Copy link
Member Author

@aske aske Jul 12, 2019 •
edited

Choose a reason for hiding this comment

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

Actually yes, it would make more sense to remove it and move wrapGAppsHook to buildInputs as it already propagates librsvg.
The problem is that virtmanager tries to update icon cache in the setup.py.
I'm not sure how to pass a flag to disable this behavior to self.distribution in the setup.py.

I can move gtk3 to nativeBuildInputs to make it work, but first want to try doing this via the setup.py flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

All you need to do here is

setupPyBuildFlags = [
  "--no-update-icon-cache"
];

We also should be able to drop the bit about compiling schemas below since that's done in setup.py.

Copy link
Member Author

@aske aske Jul 12, 2019 •
edited

Choose a reason for hiding this comment

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

Unfortunately that would not work as it passes flags to build_ext while virtmanager's setup.py checks self.distribution.

I've managed to make it work via custom buildPhase:

  buildPhase = ''
    runHook preBuild
    cp ${setuppy} nix_run_setup
    ${python3Packages.python.pythonForBuild.interpreter} nix_run_setup --no-update-icon-cache build_ext bdist_wheel
    runHook postBuild
  '';

Maybe we need to add a new setupPyDistFlags and modify build phases to include

... nix_run_setup ${(lib.concatStringsSep " " setupPyDistFlags)} ... build_ext ...

@FRidh What do you think about this? I can make a PR if it's an acceptable change. Would incur mass rebuild though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me @aske, I noticed that shortly after commenting this.

aske added 5 commits July 12, 2019 23:52
@aske
spice-gtk: propagate gstreamer plugins
2f58312 
Note: there is no sound without gst-plugins-good.
@aske
virtmanager: add missing GI dependencies
22e2acd
@aske
virtmanager: fix librsvg propagation from wrapGAppsHook
cdfc0b6 
Custom buildPhase should be removed once there is support
for setupPyDistFlags (not a final name).
@aske
virtmanager: remove redundant gsettings schema compilation
5e18c7c 
Schemas are now compiled in the setup.py.
@aske
virtviewer: remove gstreamer plugins already propagated by spice-gtk
6b68d9b
@aske
Copy link
Member Author

aske commented Jul 12, 2019

I think this PR is ready for now.

I've added a custom buildPhase so there's no need for mass-rebuild right now (if/when the change for setupPyDistFlags is accepted we can remove the phase).

Also added gst-plugins-good to the spice-gtk's propagatedBuildInputs as I found out while testing virt-viewer (thanks to @offlinehacker for directing attention there) sound was not working in either virtmanager or virtviewer without "good" plugins.

Funnily enough while testing I forgot to add gtk3 back to buildInputs but it seems everything works fine without it.

@ofborg ofborg bot requested a review from 7c6f434c July 12, 2019 21:08
@offlinehacker
Copy link
Contributor

Looks good to me, thanks for all the effort @aske :)

@aske aske mentioned this pull request Jul 12, 2019
Merged
10 tasks
@offlinehacker offlinehacker merged commit 7803ff3 into NixOS:master Jul 13, 2019
@offlinehacker
Copy link
Contributor

Ha, haven't thought #64682 is going to be merged so fast. @aske can you please make additional pull request without custom buildPhase?

@aske
Copy link
Member Author

aske commented Jul 13, 2019

Yeah, I'll make a PR, though we have some time before it's merged into master from staging.

@offlinehacker
Copy link
Contributor

Ok, yea i can see now it's for staging, fine then :)

@worldofpeace worldofpeace mentioned this pull request Jul 13, 2019
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astsmtl astsmtl astsmtl left review comments

@worldofpeace worldofpeace worldofpeace left review comments

@offlinehacker offlinehacker offlinehacker approved these changes

@qknight qknight Awaiting requested review from qknight

@xeji xeji Awaiting requested review from xeji

@fpletz fpletz Awaiting requested review from fpletz

@7c6f434c 7c6f434c Awaiting requested review from 7c6f434c

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
6 participants
@aske @abbradar @worldofpeace @emmanuelrosa @offlinehacker @astsmtl