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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

virtualbox & virtualboxGuestAdditions: cleanup #303790

Merged
merged 15 commits into from
Jun 1, 2024
Merged

virtualbox & virtualboxGuestAdditions: cleanup #303790

merged 15 commits into from
Jun 1, 2024

Conversation

FriedrichAltheide
Copy link
Contributor

Description of changes

Cleanup virtualbox and virtualboxGuestAdditions related nix files

  • Less includes
  • Removed legacy fixes
  • Changed case of draganddrop to dragAndDrop
  • Added service for dragAndDrop support to virtualboxGuestAdditions
  • Use finalAttrs

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 馃憤 reaction to pull requests you find important.

@ofborg ofborg bot requested a review from svanderburg April 13, 2024 07:38
@FriedrichAltheide FriedrichAltheide force-pushed the cleanup-virtualbox branch 3 times, most recently from b21c7f5 to 1c877e6 Compare April 15, 2024 09:09
blitz
blitz reviewed Apr 15, 2024
View reviewed changes
pkgs/applications/virtualization/virtualbox/guest-additions/builder.nix Outdated
nativeBuildInputs = [ patchelf makeWrapper pkg-config which yasm ];
buildInputs = kernel.moduleBuildDependencies ++ [ libxslt libX11 libXext libXcursor
glib nasm alsa-lib makeself pam libXmu libXrandr linuxHeaders openssl libpulseaudio xorg.xorgserver ];
nativeBuildInputs = [ patchelf pkg-config which yasm makeself nasm xorg.xorgserver openssl linuxHeaders xz ] ++ kernel.moduleBuildDependencies;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it really need yasm and nasm? Usually one of them is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't xz, openssl, linuxHeaders be in buildInputs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also xorg.xorgserver should probably be in buildInputs as it was before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

xz is not shown in ldd, yet a build dependency
openssl is not shown in ldd, yet a build dependency
linuxHeaders should only be required when prebuilding the guest additions
xorg.xorgserver should only be required while building

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it really need yasm and nasm? Usually one of them is fine.

nasm is now removed

@blitz
Copy link
Contributor

blitz commented Apr 25, 2024

I've tested this a bit:

  • Drag'n'drop still doesn't work.
  • Audio works.
  • Clipboard integration works.
  • Screen resizing works.

blitz
blitz reviewed Apr 25, 2024
View reviewed changes
pkgs/applications/virtualization/virtualbox/guest-additions/default.nix
@@ -34,16 +34,14 @@ in stdenv.mkDerivation {

env.NIX_CFLAGS_COMPILE = "-Wno-error=incompatible-pointer-types -Wno-error=implicit-function-declaration";

nativeBuildInputs = [ patchelf makeWrapper ];
buildInputs = [ virtualBoxNixGuestAdditionsBuilder ] ++ kernel.moduleBuildDependencies;
nativeBuildInputs = [ patchelf makeWrapper virtualBoxNixGuestAdditionsBuilder ] ++ kernel.moduleBuildDependencies;
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that we need the guest additions for the target architecture, why did you move it to nativeBuildInputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the guest additions are already build depending on the kernel I thought that the resulting build would be suitable for the same target architecture as well

blitz
blitz suggested changes Apr 25, 2024
View reviewed changes
Copy link
Contributor

@blitz blitz left a comment

Choose a reason for hiding this comment

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

Will approve when the unnecessary nativeBuildInputs / buildInput changes are explained/reverted.

@blitz
Copy link
Contributor

blitz commented May 3, 2024

Btw, drag'n'drop from guest to host fails with:

Drag and drop operation from guest to host failed.

DnD: Error: Requesting pending data from guest failed (VERR_TIMEOUT).
Result Code: | VBOX_E_DND_ERROR (0X80BB0011)
-- | --
Component: | GuestDnDSourceWrap
Interface: | IGuestDnDSource {dedfb5d9-4c1b-edf7-fdf3-c1be6827dc28}
Callee: | IDnDSource {d23a9ca3-42da-c94b-8aec-21968e08355d}

From host to guest fails with:

Drag and drop operation from host to guest failed.

DnD: Error: Moving to 189,208 (screen 0) failed (VERR_TIMEOUT).
Result Code: | VBOX_E_DND_ERROR (0X80BB0011)
-- | --
Component: | GuestDnDTargetWrap
Interface: | IGuestDnDTarget {50ce4b51-0ff7-46b7-a138-3c6e5ac946b4}
Callee: | IDnDTarget {ff5befc3-4ba3-7903-2aa4-43988ba11554}

The log of the drag'n'drop service in the guest says this:

image

So I would say this is an upstream issue?

@blitz
Copy link
Contributor

blitz commented May 3, 2024

Btw, the OVA is missing a text editor. This is really awkward. :-D

@ofborg ofborg bot requested a review from blitz May 10, 2024 06:39
@FriedrichAltheide
Copy link
Contributor Author

@blitz ping

@FriedrichAltheide FriedrichAltheide mentioned this pull request May 13, 2024
Merged
15 tasks
@blitz blitz requested a review from nikstur May 15, 2024 09:06
@pyrox0 pyrox0 mentioned this pull request May 22, 2024
Open
@FriedrichAltheide FriedrichAltheide mentioned this pull request May 23, 2024
Merged
13 tasks
@gabm
Copy link

gabm commented May 27, 2024

This PR is blocking the update of virtualbox to versions newer than 7.0.12. That would be required to resolve #312336, which is affecting many users (compatibility with kernels 6.9+).

What can be done to get this merged?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1695

@raboof raboof merged commit 61c1d28 into NixOS:master Jun 1, 2024
27 checks passed
@FriedrichAltheide FriedrichAltheide added the backport release-24.05 Backport PR automatically label Jun 1, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 1, 2024

Successfully created backport PR for release-24.05:

@github-actions github-actions bot mentioned this pull request Jun 1, 2024
Closed
1 task
SuperSandro2000
SuperSandro2000 reviewed Jun 2, 2024
View reviewed changes
nixos/modules/virtualisation/virtualbox-guest.nix
@@ -52,7 +52,7 @@ in
description = "Whether to enable seamless mode. When activated windows from the guest appear next to the windows of the host.";
};

draganddrop = mkOption {
dragAndDrop = mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

This should have received a mkRenamedOption

Copy link
Member

Choose a reason for hiding this comment

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

added #317756

@raboof raboof mentioned this pull request Jun 6, 2024
Merged
13 tasks
@lelgenio lelgenio mentioned this pull request Jun 8, 2024
Open
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raboof raboof raboof approved these changes

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@blitz blitz blitz approved these changes

@svanderburg svanderburg Awaiting requested review from svanderburg

@nikstur nikstur Awaiting requested review from nikstur

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 11-100 11.by: package-maintainer 12.approvals: 1 12.approved-by: package-maintainer backport release-24.05 Backport PR automatically
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@FriedrichAltheide @blitz @gabm @nixos-discourse @raboof @SuperSandro2000 @wegank