Bug 266778 - emulators/virtualbox-ose[-legacy]: VM VirtualBox Manager: Help menu: unable to open external browser
Summary: emulators/virtualbox-ose[-legacy]: VM VirtualBox Manager: Help menu: unable t...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Vladimir Druzenko
URL: https://reviews.freebsd.org/D40119
Keywords:
Depends on:
Blocks: 266992
  Show dependency treegraph
 
Reported: 2022-10-02 23:31 UTC by martin
Modified: 2023-05-18 19:50 UTC (History)
2 users (show)

See Also:
bugzilla: maintainer-feedback? (vbox)


Attachments
patch to fix PATH (418 bytes, application/x-sh)
2022-10-02 23:31 UTC, martin
no flags Details
patch for virtualbox-ose and virtualbox-ose legacy (452 bytes, patch)
2022-10-14 18:27 UTC, martin
no flags Details | Diff
patch to fix PATH with better order of path locations (452 bytes, patch)
2022-10-15 19:16 UTC, martin
no flags Details | Diff
Add /usr/local/bin and /usr/local/sbin to PATH for both virtualbox-ose and virtualbox-ose-legacy (1.25 KB, patch)
2023-05-16 17:33 UTC, Vladimir Druzenko
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description martin 2022-10-02 23:31:45 UTC
Created attachment 237030 [details]
patch to fix PATH

In VirtualBox Manager, QT based GUI, there's a menu Help that has several help items. E.g. "VirtualBox Forums" which should navigate user to a proper webpage. Under FreeBSD user is presented with error dialogue blaming desktop environment (DE). Error occurs even if DE is fully capable of doing so (opening browser,etc.).

Short description of the problem: /usr/local/bin/virtualbox shell wrapper is setting PATH variable to a very generic one excluding /usr/local/bin. Due to this browsers outside this PATH are not found.

Handling URL is done by QT's QDesktopServices API. During the resolving  detectWebBrowser() is called:

https://codebrowser.dev/qt5/qtbase/src/platformsupport/services/genericunix/qgenericunixservices.cpp.html#_ZL16detectWebBrowserRK10QByteArraybP7QString

which calls findExecutable()

https://codebrowser.dev/qt5/qtbase/src/corelib/io/qstandardpaths.cpp.html#_ZN14QStandardPaths14findExecutableERK7QStringRK11QStringList

Here qgetenv("PATH") is called which is used as PATH to search executables.

It makes sense to expand the PATH to include FreeBSD's standard /usr/local/bin as that's native path for ports programs. 

I've added the patch towards the virtualbox-ose-6.1.36.
Comment 1 Graham Perrin freebsd_committer freebsd_triage 2022-10-13 22:33:54 UTC
Thanks, 

(In reply to martin from comment #0)

Can the patch be broadened, to include emulators/virtualbox-ose-legacy, without difficulty?
Comment 2 martin 2022-10-14 18:27:21 UTC
Created attachment 237306 [details]
patch for virtualbox-ose and virtualbox-ose legacy
Comment 3 martin 2022-10-14 18:27:48 UTC
My patch is fixing src/VBox/Installer/freebsd/VBox.sh. As I was testing if I can use this in legacy I've noticed that actually VBox.sh exists because of a patch files/patch-src-VBox-Installer-freebsd-VBox.sh for given package. I.e. it's patch that creates this file for FreeBSD. 

So instead of patching file that as created as a result of a patch it makes sense to patch that patch. While files are different diff produces the same patch as only very small portion of the script has to be modified and that part is the same.
Comment 4 Graham Perrin freebsd_committer freebsd_triage 2022-10-15 08:49:21 UTC
(In reply to martin from comment #3)

Nice find! Thank you. 

Whilst here, a nit: is it worth making the order more like what's normally seen? 

So, 

PATH="/sbin:/bin:/usr/sbin:/usr/bin:/usr/local/sbin:/usr/local/bin"


----

That is, based partly on the result of a fairly recent installation of 13.1. 

root@fuji:~ # uname -aKU
FreeBSD fuji 13.1-RELEASE-p2 FreeBSD 13.1-RELEASE-p2 GENERIC amd64 1301000 1301000
root@fuji:~ # whoami ; echo $PATH
root
/sbin:/bin:/usr/sbin:/usr/bin:/usr/local/sbin:/usr/local/bin:/root/bin
root@fuji:~ #
Comment 5 martin 2022-10-15 15:45:49 UTC
Personally I like it the way it is in patch. There's no reason for VirtualBox to expand path outside of standard path (e.g. ~/bin; note not /root/bin as most likely vbox is not launched as root).
Comment 6 Graham Perrin freebsd_committer freebsd_triage 2022-10-15 16:09:43 UTC
(In reply to martin from comment #5)

Thanks, and please note that the suggested path included neither ~/bin nor /root/bin …

… the (full) output from    echo $PATH    was only to demonstrate the order of the subset.
Comment 7 martin 2022-10-15 19:16:26 UTC
Created attachment 237357 [details]
patch to fix PATH with better order of path locations
Comment 8 martin 2022-10-15 19:17:40 UTC
Ok, my bad.

Well, it does make sense to look in "bin" first as it's most likely location of user programs. So following that logic it makes sense to shuffle them probably even more so that those path locations are searched first.
Comment 9 martin 2023-01-01 20:13:44 UTC
What is the reason of state change? Is there something else required for the patch to be ready?
Comment 10 Graham Perrin freebsd_committer freebsd_triage 2023-01-02 20:34:05 UTC
(In reply to martin from comment #9)

> What is the reason of state change?

Thanks for asking. 

Removal of keywords is due to their deprecation. Related: 

* the new descriptions (click 'Keywords') – these were outdated around the 
  time of my first change

* <https://bugs.freebsd.org/bugzilla/> ▶ Latest FreeBSD Bugzilla Changes

* <https://wiki.freebsd.org/Bugzilla#Resources> ▶ 
  <https://wiki.freebsd.org/Bugzilla/Keywords#Process_based_keywords>

* <https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=227147>.

----

> Is there something else required for the patch to be ready?

I do not know, but we have vbox@ flagged for maintainer feedback.
Comment 11 Vladimir Druzenko freebsd_committer freebsd_triage 2023-05-16 17:33:58 UTC
Created attachment 242218 [details]
Add /usr/local/bin and /usr/local/sbin to PATH for both virtualbox-ose and virtualbox-ose-legacy

(In reply to Graham Perrin from comment #10)
> I do not know, but we have vbox@ flagged for maintainer feedback.
vbox@ are half-dead (as far as I see last years).
Comment 12 Vladimir Druzenko freebsd_committer freebsd_triage 2023-05-16 19:29:21 UTC
I hope vbox@ allow me resole several PRs with emulators/virtualbox-ose*.
Comment 13 commit-hook freebsd_committer freebsd_triage 2023-05-18 19:49:21 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=8b9f42573ed3ebcd6a84f7ccd1ba2ef000357332

commit 8b9f42573ed3ebcd6a84f7ccd1ba2ef000357332
Author:     Vladimir Druzenko <vvd@FreeBSD.org>
AuthorDate: 2023-05-18 19:41:30 +0000
Commit:     Vladimir Druzenko <vvd@FreeBSD.org>
CommitDate: 2023-05-18 19:48:47 +0000

    emulators/virtualbox-ose[-legacy]: VM VirtualBox Manager: Help menu: unable to open external browser

    While here sort out LOCALBASE, PREFIX, VBOX_DIR, VBOX_ETC

    PR:                     266778
    Approved by:            arrowd (mentor)
    Differential Revision:  https://reviews.freebsd.org/D40119

 emulators/virtualbox-ose-legacy/Makefile                     |  6 ++++--
 .../files/patch-src-VBox-Installer-freebsd-VBox.sh           | 10 +++++-----
 emulators/virtualbox-ose/Makefile                            | 10 ++++++----
 .../files/patch-src-VBox-Installer-freebsd-VBox.sh           | 12 ++++++------
 .../patch-src_VBox_HostDrivers_adpctl_VBoxNetAdpCtl.cpp      |  2 +-
 emulators/virtualbox-ose/files/pkg-message.in                |  6 +++---
 6 files changed, 25 insertions(+), 21 deletions(-)