Bug 261522 - emulators/virtualbox-ose: Place vbox/networks.conf in the right place (hier(7))
Summary: emulators/virtualbox-ose: Place vbox/networks.conf in the right place (hier(7))
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Virtualbox Team (Nobody)
URL:
Keywords: needs-patch, needs-qa
Depends on:
Blocks:
 
Reported: 2022-01-27 18:51 UTC by Michael Gmelin
Modified: 2022-10-05 13:43 UTC (History)
3 users (show)

See Also:
madpilot: maintainer-feedback+
grembo: maintainer-feedback+


Attachments
Patch networks.conf location (1.46 KB, patch)
2022-01-27 18:51 UTC, Michael Gmelin
no flags Details | Diff
Patch as described in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=261522#c3 (4.40 KB, patch)
2022-02-01 10:51 UTC, Michael Gmelin
no flags Details | Diff
Updated patch to move networks.conf to LOCALBASE (7.13 KB, patch)
2022-03-10 09:10 UTC, Michael Gmelin
grembo: maintainer-approval?
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Gmelin freebsd_committer freebsd_triage 2022-01-27 18:51:47 UTC
Created attachment 231393 [details]
Patch networks.conf location

Virtualbox recently introduced a new configuration file called networks.conf, see https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=259399#c16 for details.

Right now this is expected to live in /etc/vbox, which goes against hier(7).

The attached patch corrects this to /usr/local/etc/vbox (it's not looking at PREFIX, as the vbox installer script is also hardcoded to /usr/local - this way things are at least consistent).
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2022-01-28 01:01:30 UTC
Can't hardcode /usr/local, its a variable user-defined path. Patch hardcoded value using REINPLACE_CMD to substitute in ${LOCALBASE}. Thanks! Also, is quarterly affected?
Comment 2 Michael Gmelin freebsd_committer freebsd_triage 2022-01-28 09:51:14 UTC
(In reply to Kubilay Kocak from comment #1)

> Can't hardcode /usr/local, its a variable user-defined path.
> Patch hardcoded value using REINPLACE_CMD to substitute in ${LOCALBASE}. 
> Thanks!

Hi, I know the dance, but I explained why I didn't do that in the bug description, the existing patch hardcoding LOCALBASE to be /usr/local is [0].

Either it's corrected there as well, or we hardcode it in my patch (having parts in /usr/local/etc/vbox and parts in $LOCALBASE/etc/vbox doesn't make things better). As I'm not the maintainer I went the route that was less work and aligned with how things are done in the port at the moment.

I *could* of course do that extra work, but before I do it I would need to know it will be looked at by maintainers (who seem to overlap with portmgr).

> Also, is quarterly affected?

Yes


[0] https://cgit.freebsd.org/ports/tree/emulators/virtualbox-ose/files/patch-src-VBox-Installer-freebsd-VBox.sh?h=2022Q1&id=6348afa6983147beb3d46f9afdd72c9fbc46ca49#n20
Comment 3 Michael Gmelin freebsd_committer freebsd_triage 2022-02-01 10:49:21 UTC
(In reply to Kubilay Kocak from comment #1)

Okay, ignore what I wrote, I just realized that /usr/local is actually replaced in Makefile without using something like %%LOCALBASE%% in the patch. The advantage of doing it this way is that it plays nicely with `make makepatch`, the downside is, that it causes warnings ("REINPLACE_CMD ran, but did not modify file contents") in case LOCALBASE == "/usr/local".

So I just added the patched file to the same REINPLACE_CMD.

While there, I changed the patch so that vbox actually looks for networks.conf in two places - first in $LOCALBASE/etc/vbox and if it's not in there, then in /etc/vbox. This will make sure people's existing setups won't break on upgrade.

I also wrapped REINPLACE_CMDs that are noops in case of default LOCALBASE/PREFIX values in if-statements to make stage-qa pass without warnings.

Patch will follow shortly.
Comment 4 Michael Gmelin freebsd_committer freebsd_triage 2022-02-01 10:51:26 UTC
Created attachment 231490 [details]
Patch as described in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=261522#c3
Comment 5 Michael Gmelin freebsd_committer freebsd_triage 2022-02-22 18:37:21 UTC
@vbox ping?
Comment 6 Michael Gmelin freebsd_committer freebsd_triage 2022-03-08 18:39:33 UTC
Maintainer timeout?
Comment 7 Guido Falsi freebsd_committer freebsd_triage 2022-03-08 20:21:59 UTC
Hi,

as a member of vbox@ I'll say that I disagree with the conditionals in the makefile around the REINPLACE_CMD.

The warning message is not beatiful but does no harm, OTOH the Makefile is already overly complex as is.

Regarding the source file patch it looks reasonable, but why are you adding:

#define VBOX_GLOBAL_NETWORK_CONFIG_PATH2 "/etc/vbox/networks.conf"


AFAIK the only place where vbox looks for all it's files is /usr/local/etc/vbox.

Does other virtualbox source code parts look in /etc/vbox in addition to local?
Comment 8 Michael Gmelin freebsd_committer freebsd_triage 2022-03-10 09:10:22 UTC
Created attachment 232367 [details]
Updated patch to move networks.conf to LOCALBASE

@madpilot Please see updated the patch based on our private conversation.
Comment 9 Guido Falsi freebsd_committer freebsd_triage 2022-03-10 17:43:40 UTC
(In reply to Michael Gmelin from comment #8)

Thanks for the revised patch.

It looks fine.

I'd like to test these locally before committing it, so please allow me some time for testing.

Hope to be able to be done in a day or two, unluckily compiling a lot of ports on my build machine and it's quite busy.
Comment 10 commit-hook freebsd_committer freebsd_triage 2022-03-11 21:51:57 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=66635b2061daf5993d8bbf80457ee59946d91da4

commit 66635b2061daf5993d8bbf80457ee59946d91da4
Author:     Michael Gmelin <grembo@FreeBSD.org>
AuthorDate: 2022-03-11 21:44:02 +0000
Commit:     Guido Falsi <madpilot@FreeBSD.org>
CommitDate: 2022-03-11 21:51:31 +0000

    emulators/virtualbox-ose: Put vbox/networks.conf in the right place

    Since virtualbox-ose 6.1.26 a new networks.conf file has been
    introduced and the software looks for it in /etc/vbox.

    This commit introduces a patch to make virtualbox look for it in
    PREFIX/etc/vbox.

    To help users with configurations already using this file in /etc/vbox
    notes to UPDATING, pkg-message and to the main virtualbox executable
    script have been added.

    Also adding some comments to the Makefile to note that QA warning
    due to reinplace being run but not having changed any file are
    expected due to the design of this port Makefile.

    PR:             261522

 UPDATING                                                  |  8 ++++++++
 emulators/virtualbox-ose/Makefile                         |  6 ++++++
 .../files/patch-src-VBox-Installer-freebsd-VBox.sh        | 13 ++++++++++---
 ...ch-src_VBox_HostDrivers_adpctl_VBoxNetAdpCtl.cpp (new) | 11 +++++++++++
 .../virtualbox-ose/{pkg-message => files/pkg-message.in}  | 15 +++++++++++++++
 5 files changed, 50 insertions(+), 3 deletions(-)
Comment 11 Guido Falsi freebsd_committer freebsd_triage 2022-03-11 21:53:48 UTC
Patch committed. Thanks for your work!
Comment 12 Michael Gmelin freebsd_committer freebsd_triage 2022-03-12 21:54:50 UTC
(In reply to Guido Falsi from comment #11)

@Guido Thanks for review, testing, and committing.
Comment 13 Graham Perrin freebsd_committer freebsd_triage 2022-03-13 11:15:48 UTC
(In reply to Michael Gmelin from comment #12)

+1

More broadly, thanks to _everyone_ in Bugzilla, and elsewhere, who helps to move
things forward for FreeBSD. 

(There's no 'thank you' or 'thumbs-up' button/feature in Bugzilla; visualise it being clicked, clicked, clicked, clicked, clicked …)