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

virtualbox: Fix #65564 #67968

Merged
merged 1 commit into from Sep 4, 2019
Merged

virtualbox: Fix #65564 #67968

merged 1 commit into from Sep 4, 2019

Conversation

ambrop72
Copy link
Contributor

@ambrop72 ambrop72 commented Sep 2, 2019

When hardening is enabled, we cannot use a wrapper for VirtualBoxVM, so patch
the source code to set QT_PLUGIN_PATH as required.

Motivation for this change
Things done

Tested that virtual machines run via the GUI on nixos-unstable both with and without hardening.

  • 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 @flokli @svanderburg

@ambrop72 ambrop72 mentioned this pull request Sep 2, 2019
Closed
@ofborg ofborg bot requested review from svanderburg and flokli September 2, 2019 21:48
@flokli
Copy link
Contributor

flokli commented Sep 2, 2019

@ambrop72 nice, thanks a lot for this!

I gave this a test run, and could get the virtualbox tests to work. However, some assertion inside pkgs/os-specific/linux/phc-intel/default.nix seemed to prevent running the tests:

assertion failed at /home/flokli/dev/nixos/nixpkgs-virtualbox-fix/pkgs/os-specific/linux/phc-intel/default.nix:4:1

I was lazy, commented-out the assertion, and the tests did pass 🎉

I'm not sure why this resurfaced now.
Did you run into it as well?

What about that line here in nixos/tests/virtualbox?

    # XXX: Remove this once TSS location detection has been fixed in VirtualBox
    boot.kernelPackages = pkgs.linuxPackages_4_9;

is this fixed by now? Can we move to a more recent kernel?

@flokli
Copy link
Contributor

flokli commented Sep 2, 2019

Also CC @ttuegel on QT_PLUGIN_PATH, as he probably knows best on whether this is the right approach to do this.

worldofpeace
worldofpeace reviewed Sep 3, 2019
View reviewed changes
pkgs/applications/virtualization/virtualbox/qt-env-vars.patch Outdated
+
+ /*
+ * NixOS hack: Set QT_PLUGIN_PATH to make Qt find plugins. The placeholder
+ * on the right will be replaced with the correct value by the builder.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use substituteAll instead of the manual sed? Many examples of how we use this in gtk packaging fix-paths.patchs

worldofpeace
worldofpeace reviewed Sep 3, 2019
View reviewed changes
pkgs/applications/virtualization/virtualbox/qt-env-vars.patch Outdated
+ * NixOS hack: Set QT_PLUGIN_PATH to make Qt find plugins. The placeholder
+ * on the right will be replaced with the correct value by the builder.
+ */
+ setenv("QT_PLUGIN_PATH", "<QT_PLUGIN_PATH>", /*overwrite=*/ 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Prepare for this to be a reference point for when anyone else bumps into an issue like this 😄
(and suddenly there's 15 more patches like it)

worldofpeace
worldofpeace reviewed Sep 3, 2019
View reviewed changes
pkgs/applications/virtualization/virtualbox/default.nix Outdated
@@ -37,7 +37,7 @@ in stdenv.mkDerivation {
nativeBuildInputs = [ pkgconfig which docbook_xsl docbook_xml_dtd_43 patchelfUnstable ]
++ optional (!headless) wrapQtAppsHook;

# Wrap manually because we just need to wrap one executable
# Wrap manually because we just need to wrap one or two executables.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Wrap manually because we just need to wrap one or two executables.
# Wrap manually because we have limit which executables are wrapped.

@ambrop72
Copy link
Contributor Author

ambrop72 commented Sep 3, 2019

Updated according to comments.

@flokli I have not tried the tests, just that it works from the desktop, and I don't know about that kernel version. This change couldn't have cause that assertion error. Let's deal with the tests in #66405.

@ambrop72 ambrop72 mentioned this pull request Sep 3, 2019
Merged
10 tasks
@ambrop72 @flokli
virtualbox: Make sure that Qt finds plugins to make VMs run again.
b52dfd3 
Fixes NixOS#65564. When hardening is enabled, we cannot use a wrapper for
VirtualBoxVM, so patch the source code to set QT_PLUGIN_PATH as required.
@flokli flokli merged commit 432e552 into NixOS:master Sep 4, 2019
@flokli
Copy link
Contributor

flokli commented Sep 4, 2019

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samueldr samueldr samueldr left review comments

@worldofpeace worldofpeace worldofpeace left review comments

@svanderburg svanderburg Awaiting requested review from svanderburg

@flokli flokli Awaiting requested review from flokli

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@ambrop72 @flokli @samueldr @worldofpeace