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

Fix UI language selection in virtualbox #358400

Merged
merged 1 commit into from
Nov 27, 2024
Merged

Conversation

acmelabrat
Copy link
Contributor

Previously only English was available as UI language

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 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.

@acmelabrat
Copy link
Contributor Author

acmelabrat commented Nov 23, 2024

The fix is mimicked from Debian install of VirtualBox.

Tested this on NixOS by using nixos-rebuild -I nixpkgs=<path_to_local_repo> switch.

One peculiarity is that virtualbox installed under environment.systemPackages shows the language selection correctly (more than built-in/English) without this patch. Mind you, VirtualBox of course can't start any VMs if it is installed this way.

@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Nov 23, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 labels Nov 24, 2024
Copy link
Contributor

@FriedrichAltheide FriedrichAltheide left a comment

Choose a reason for hiding this comment

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

Thank you very much for the PR. Could you please reword your commit message to "virtualbox: Fix UI language selection in virtualbox"?

@acmelabrat
Copy link
Contributor Author

Thank you very much for the PR. Could you please reword your commit message to "virtualbox: Fix UI language selection in virtualbox"?

Sure, no problem

@FriedrichAltheide FriedrichAltheide added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Nov 25, 2024
@ofborg ofborg bot requested a review from FriedrichAltheide November 25, 2024 15:36
@Yarny0
Copy link
Contributor

Yarny0 commented Nov 26, 2024

Thanks, @acmelabrat, for fixing this!

Tested after cherry-picking onto current nixos-unstable:

  • virtualboxHardened builds
  • nixosTests.virtualbox all build/pass
  • GUI is finally localized!
  • very simple VM starts

This is amazing!

However, this breaks the nixosTests.virtualbox.net-hostonlyif vm test: It fails after 30 minutes with:

command `su - alice -c 'for i in $(seq 1000); do if ipaddr="$(VBoxManage guestproperty get test1 /VirtualBox/GuestInfo/Net/1/V4/IP | sed -n -e '"'"'s/^Value: //p'"'"')" && [ -n "$ipaddr" ]; then echo "$ipaddr"; exit 0; fi; sleep 1; done; echo '"'"'Could not get IPv4 address for test1!'"'"' >&2; exit 1'` failed (exit code 1)

From investiagting further, it seems it spends most of its time in wait_for_vm_boot_test{1,2}, but I failed to find out why this happens. I suspect VirtualBox now responds to one of the VBoxManaged calls with localized output which isn't recognized by the test script anymore.

@acmelabrat
Copy link
Contributor Author

acmelabrat commented Nov 26, 2024

First off, thank you for your feedback @Yarny0 !

However, this breaks the nixosTests.virtualbox.net-hostonlyif vm test: It fails after 30 minutes with:

command `su - alice -c 'for i in $(seq 1000); do if ipaddr="$(VBoxManage guestproperty get test1 /VirtualBox/GuestInfo/Net/1/V4/IP | sed -n -e '"'"'s/^Value: //p'"'"')" && [ -n "$ipaddr" ]; then echo "$ipaddr"; exit 0; fi; sleep 1; done; echo '"'"'Could not get IPv4 address for test1!'"'"' >&2; exit 1'` failed (exit code 1)

From investiagting further, it seems it spends most of its time in wait_for_vm_boot_test{1,2}, but I failed to find out why this happens. I suspect VirtualBox now responds to one of the VBoxManaged calls with localized output which isn't recognized by the test script anymore.

Just to cover all bases: Does this test pass without the cherry-picked commit?

If you're correct and it is because of the localization, this would mean that the test would need to be adjusted.
(tested manually on existing VM and it is not localized)

If it expects files at a certain location, either the files need to be distributed more fine-grained (not seeing forward to that) or the test needs to be changed accordingly.

@Yarny0
Copy link
Contributor

Yarny0 commented Nov 27, 2024

tl;dr: All tests pass, I approve this PR without any reservations, I'm sorry for the false alarm.


Does this test pass without the cherry-picked commit?

Yes. To avoid nix just fetching the test's result from the binary cache, I added an extra empty line somewhere in the test's python code, and it built flawlessly.

Today I tried again, with the exact same tree as yesterday. Now simple-gui failed, also after 15 or more minutes, with a different error: VirtualBox VM didn't shut down within 2 minutes. After adding some debug print statements, simple-gui passed, but simple-cli timed out (didn't note the error this time). Then I retried from pure nixos-unstable and re-ran the tests again (by adding empty lines to the code). Again, net-hostonlyif failed (with the same error as yesterday).

sigh

Maybe there is some -- very time consuming and therefore very frustrating -- flakeyness somewhere in the test's logic. However, after searching for "nixos.tests.virtualbox" in https://hydra.nixos.org/jobset/nixos/trunk-combined#tabs-jobs , it seems Hydra is not experiencing that flakeyness.

In any case, I no longer see this PR at fault for any test-related trouble. Sorry for the confusion, @acmelabrat . From my point of view, your PR can (and should) be merged!

@0x4A6F 0x4A6F merged commit 4a989c6 into NixOS:master Nov 27, 2024
35 of 36 checks passed
@acmelabrat
Copy link
Contributor Author

tl;dr: All tests pass, I approve this PR without any reservations, I'm sorry for the false alarm.

@Yarny0 , no problem. It's still valuable that you looked into it. I am too novice to know for sure, but it sounds like you might've found an issue with the tests themselves.

@acmelabrat
Copy link
Contributor Author

Is it possible to backport this PR to 24.11 by any chance? 😅

@bjornfor bjornfor added the backport release-24.11 Backport PR automatically label Dec 12, 2024
@nix-backports
Copy link

nix-backports bot commented Dec 12, 2024

Successfully created backport PR for release-24.11:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package 12. first-time contribution This PR is the author's first one; please be gentle! backport release-24.11 Backport PR automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants