openvpn3: v20 -> v22_dev #326623

JarvisCraft · 2024-07-12T18:27:57Z

Description of changes

This updates openvpn3 from v20 to v22_dev.
Considering that v20 and v21 currently don't work due to glib problems (see #235986 for details) it is worth jumping to _dev version for now (worth noting that Aur already uses this version).

This PR also adds the gdbuspp module which is a D-Bus library developed by OpenVPN team now used by openvpn3-linux.

Also, the corresponding NixOS module now generates the openvpn3 configs in /etc with the settings exposed via module's configuration.

I've tested the changes on my setup and they seem to fix issues previously observed on v20.

cc @dsommers as suggested by my colleague.

Things done

Add a 👍 reaction to pull requests you find important.

nixos/modules/programs/openvpn3.nix

JarvisCraft · 2024-07-21T21:47:28Z

@SCOTT-HAMILTON, @KFearsoff, hi there!

Since modules allow meta.maintainers, I've added you to that of openvpn3 module (copying the declaration from openvpn3 package). Please make sure to tell me if this is wrong, in which case I will remove you from it

nixos-discourse · 2024-07-21T21:57:02Z

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/4300

KFearsoff

Thank you for your contribution! I'm truly grateful for your effort, since my schedule and burnout made updating the package hard, and having it not been updated was a lot of psychological pressure. This is also why it took me a while to review, but I digress; excellent work here!

And the technical part is excellent. The Nixpkgs APIs are very well-researched, and the decisions made are rock solid. I couldn't find anything I disagree with, just a few small questions.

Happy to say: LGTM!

pkgs/by-name/op/openvpn3/package.nix

pkgs/by-name/op/openvpn3/patches/0001-customizable-asio-path.patch

JarvisCraft · 2024-07-22T07:42:20Z

@KFearsoff, thanks a lot for your review and the nice words!

Hope you are fine and will defeat the burnout!

It was thanks to @CertainLach who led me through accurately packaging the app and updating the module (it was an external chat with him), so a lot of credits go to him.

The best of luck to you!

nixos-discourse · 2024-07-22T08:04:39Z

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

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

JarvisCraft · 2024-07-22T09:33:53Z

@KFearsoff, could you please re-approve?
I've applied the following change via a rebase:

-echo '#define PACKAGE_GUIVERSION "${builtins.replaceStrings ["_"] [":"] version}_{unknown}__s"' >> ./src/build-version.h
+echo '#define PACKAGE_GUIVERSION "${builtins.replaceStrings ["_"] [":"] version}_unknown__s"' >> ./src/build-version.h

JarvisCraft · 2024-07-23T11:02:17Z

Thanks!

SuperSandro2000 · 2024-07-26T22:42:16Z

pkgs/by-name/op/openvpn3/patches/0002-customizable-installation-paths.patch

+ # NOTE: Can be replaced with install_emptydir() when Meson 0.60 or newer
+ # is available on all supported distros
+-meson.add_install_script('sh','-c', 'mkdir -p $DESTDIR@0@'.format(openvpn3_statedir / 'configs'))
+# meson.add_install_script('sh','-c', 'mkdir -p $DESTDIR@0@'.format(openvpn3_statedir / 'configs'))


please delete lines in patches instead of commenting them

SuperSandro2000 · 2024-07-26T22:42:59Z

pkgs/by-name/op/openvpn3/package.nix

+ maintainers = [
+ lib.maintainers.shamilton
+ lib.maintainers.kfears
+ lib.maintainers.progrm_jarvis
+ ];


Suggested change

maintainers = [

lib.maintainers.shamilton

lib.maintainers.kfears

lib.maintainers.progrm_jarvis

];

maintainers = with lib.maintainers; [

shamilton

kfears

progrm_jarvis

];

Thanks, done

SuperSandro2000 · 2024-07-26T22:43:22Z

pkgs/by-name/op/openvpn3/package.nix

+ glib.dev
+ jsoncpp.dev
+ libcap_ng.dev
+ libnl.dev
+ libuuid.dev
+ lz4.dev
+ openssl.dev


The dev output should be choosen automatically

Fair enough, done

SuperSandro2000 · 2024-07-26T22:43:34Z

pkgs/by-name/op/openvpn3/package.nix

+ python3.pkgs.jinja2
+ python3.pkgs.dbus-python
+ wrapGAppsHook3
+ gobject-introspection.dev


Suggested change

gobject-introspection.dev

gobject-introspection

this is likely wrong or in the wrong input

Removed .dev, yet it is consciously on nativeBuildInputs

SuperSandro2000 · 2024-07-26T22:43:50Z

pkgs/by-name/op/openvpn3/package.nix

+ echo '#define PACKAGE_GUIVERSION "${builtins.replaceStrings ["_"] [":"] version}_unknown__s"' >> ./src/build-version.h
+ echo '#define PACKAGE_NAME "openvpn3-linux"' >> ./src/build-version.h
+
+ patchShebangs ** /*.py ** /*.sh \


Suggested change

patchShebangs ** /*.py ** /*.sh \

patchShebangs **/*.py **/*.sh \

do you mean this?

This was part of the original openvpn3 (when it rested in pkgs/tools) thus I have kept it as-is, but I will do a re-check.

UPD: I've actualized the call to patchShebangs, now only patches files which actually need it

SuperSandro2000 · 2024-07-26T22:44:47Z

pkgs/by-name/op/openvpn3/package.nix

+ echo '#define OPENVPN_VERSION "3.git:unknown:unknown"' > ./src/build-version.h
+ echo '#define PACKAGE_GUIVERSION "${builtins.replaceStrings ["_"] [":"] version}_unknown__s"' >> ./src/build-version.h
+ echo '#define PACKAGE_NAME "openvpn3-linux"' >> ./src/build-version.h


Suggested change

echo '#define OPENVPN_VERSION "3.git:unknown:unknown"' > ./src/build-version.h

echo '#define PACKAGE_GUIVERSION "${builtins.replaceStrings ["_"] [":"] version}_unknown__s"' >> ./src/build-version.h

echo '#define PACKAGE_NAME "openvpn3-linux"' >> ./src/build-version.h

echo '#define OPENVPN_VERSION "3.git:unknown:unknown"

#define PACKAGE_GUIVERSION "${builtins.replaceStrings ["_"] [":"] version}_unknown__s"

#define PACKAGE_NAME "openvpn3-linux"

' > ./src/build-version.h

True, thanks!

SuperSandro2000 · 2024-07-26T22:49:13Z

nixos/modules/programs/openvpn3.nix

+ description =
+ "Options stored in {file}`/etc/openvpn3/log-service.json` configuration file";


Suggested change

description =

"Options stored in {file}`/etc/openvpn3/log-service.json` configuration file";

description = "Options stored in {file}`/etc/openvpn3/log-service.json` configuration file";

This is (and the following) formatting is by nixfmt-rfc-style which is specified by default.nix so I am unsure if I should manually rollback the lines (given that it would move them back again). Correct me if I am wrong

Let's follow the nixfmt-rfc-style

SuperSandro2000 · 2024-07-26T22:49:23Z

nixos/modules/programs/openvpn3.nix

+ defaultText =
+ literalExpression "config.services.resolved.enable";


Suggested change

defaultText =

literalExpression "config.services.resolved.enable";

defaultText = literalExpression "config.services.resolved.enable";

(see note above about nixfmt-rfc-style)

SuperSandro2000 · 2024-07-26T22:49:34Z

nixos/modules/programs/openvpn3.nix

+ description =
+ "Options stored in {file}`/etc/openvpn3/netcfg.json` configuration file";


Suggested change

description =

"Options stored in {file}`/etc/openvpn3/netcfg.json` configuration file";

description = "Options stored in {file}`/etc/openvpn3/netcfg.json` configuration file";

(see note above about nixfmt-rfc-style)

SuperSandro2000 · 2024-07-26T22:49:48Z

nixos/modules/programs/openvpn3.nix

+in {
+ options.programs.openvpn3 = let
+ inherit (lib)
+ mkEnableOption mkPackageOption mkOption literalExpression max options
+ lists;
+ inherit (lib.types) bool submodule ints;
+ in {


Suggested change

in {

options.programs.openvpn3 = let

inherit (lib)

mkEnableOption mkPackageOption mkOption literalExpression max options

lists;

inherit (lib.types) bool submodule ints;

in {

in {

options.programs.openvpn3 = {

(see note below about imports' scope)

SuperSandro2000 · 2024-07-26T22:50:08Z

nixos/modules/programs/openvpn3.nix

 let
+ json = pkgs.formats.json { };
 cfg = config.programs.openvpn3;


Suggested change

cfg = config.programs.openvpn3;

cfg = config.programs.openvpn3;

inherit (lib) mkEnableOption mkPackageOption mkOption literalExpression max options lists;

inherit (lib.types) bool submodule ints;

Given that all of this imports are specific to building module options, is it worth importing them for the whole module?

I think using imports for the entire module is more readable, because you don't use nested let statements. It's also easier to understand - I mean, you don't usually scope the stdlib to module options, so choosing to do that raises some questions.

JarvisCraft · 2024-07-27T00:33:41Z

@SuperSandro2000, thanks for the detailed review! I've fixed most of the reported issues with the exception of a few specific ones (referenced in the comments).

As for the formatting, should I use nixfmt or nixfmt-rfc-style for the NixOS module?

Co-authored-by: Yaroslav Bolyukin <iam@lach.pw>

github-actions bot added 6.topic: nixos 8.has: module (update) 8.has: maintainer-list (update) labels Jul 12, 2024

JarvisCraft mentioned this pull request Jul 12, 2024

openvpn3 won't work with glib:2-2.76 or superior #235986

Open

CertainLach reviewed Jul 12, 2024

View reviewed changes

nixos/modules/programs/openvpn3.nix Outdated Show resolved Hide resolved

maintainers: add progrm_jarvis

e09b6c4

JarvisCraft force-pushed the openvpn3-v22_dev branch from 66f102a to ac170ae Compare July 12, 2024 18:44

JarvisCraft marked this pull request as draft July 12, 2024 19:00

ofborg bot added the 8.has: package (new) label Jul 12, 2024

ofborg bot requested review from KFearsoff and SCOTT-HAMILTON July 12, 2024 22:05

ofborg bot added 11.by: package-maintainer 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Jul 12, 2024

JarvisCraft force-pushed the openvpn3-v22_dev branch 14 times, most recently from f5431df to cf87dd1 Compare July 19, 2024 23:44

JarvisCraft marked this pull request as ready for review July 19, 2024 23:54

JarvisCraft force-pushed the openvpn3-v22_dev branch from cf87dd1 to 7ad804c Compare July 20, 2024 12:13

JarvisCraft force-pushed the openvpn3-v22_dev branch 6 times, most recently from a184703 to adbbffd Compare July 21, 2024 21:44

CertainLach approved these changes Jul 21, 2024

View reviewed changes

JarvisCraft force-pushed the openvpn3-v22_dev branch from adbbffd to 8b96e12 Compare July 21, 2024 23:25

KFearsoff approved these changes Jul 22, 2024

View reviewed changes

pkgs/by-name/op/openvpn3/package.nix Outdated Show resolved Hide resolved

pkgs/by-name/op/openvpn3/patches/0001-customizable-asio-path.patch Outdated Show resolved Hide resolved

JarvisCraft force-pushed the openvpn3-v22_dev branch from 8b96e12 to 641d1d9 Compare July 22, 2024 08:14

ofborg bot requested a review from KFearsoff July 22, 2024 09:01

KFearsoff approved these changes Jul 23, 2024

View reviewed changes

SuperSandro2000 reviewed Jul 26, 2024

View reviewed changes

JarvisCraft force-pushed the openvpn3-v22_dev branch 2 times, most recently from 1591f71 to 2f3ccd9 Compare July 27, 2024 00:25

gdbuspp: init at 1

9ee161c

JarvisCraft force-pushed the openvpn3-v22_dev branch from 2f3ccd9 to a650770 Compare July 27, 2024 00:46

JarvisCraft requested a review from SuperSandro2000 July 27, 2024 00:48

JarvisCraft force-pushed the openvpn3-v22_dev branch from a650770 to a36182a Compare July 27, 2024 00:57

openvpn3: 20 -> 22_dev

ed9d9eb

Co-authored-by: Yaroslav Bolyukin <iam@lach.pw>

JarvisCraft force-pushed the openvpn3-v22_dev branch from a36182a to ed9d9eb Compare July 27, 2024 01:04

ofborg bot requested a review from KFearsoff July 27, 2024 02:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

openvpn3: v20 -> v22_dev #326623

openvpn3: v20 -> v22_dev #326623

JarvisCraft commented Jul 12, 2024 •

edited

Loading

JarvisCraft commented Jul 21, 2024

nixos-discourse commented Jul 21, 2024

KFearsoff left a comment

JarvisCraft commented Jul 22, 2024

nixos-discourse commented Jul 22, 2024

JarvisCraft commented Jul 22, 2024 •

edited

Loading

JarvisCraft commented Jul 23, 2024

SuperSandro2000 Jul 26, 2024 •

edited

Loading

JarvisCraft Jul 26, 2024

SuperSandro2000 Jul 26, 2024

JarvisCraft Jul 26, 2024

SuperSandro2000 Jul 26, 2024

JarvisCraft Jul 26, 2024

SuperSandro2000 Jul 26, 2024

JarvisCraft Jul 26, 2024

SuperSandro2000 Jul 26, 2024

JarvisCraft Jul 26, 2024

JarvisCraft Jul 30, 2024

SuperSandro2000 Jul 26, 2024

JarvisCraft Jul 26, 2024

SuperSandro2000 Jul 26, 2024

JarvisCraft Jul 26, 2024 •

edited

Loading

KFearsoff Jul 31, 2024

SuperSandro2000 Jul 26, 2024

JarvisCraft Jul 27, 2024

SuperSandro2000 Jul 26, 2024

JarvisCraft Jul 27, 2024

SuperSandro2000 Jul 26, 2024

JarvisCraft Jul 27, 2024

SuperSandro2000 Jul 26, 2024

JarvisCraft Jul 26, 2024

KFearsoff Jul 31, 2024

JarvisCraft commented Jul 27, 2024

	patchShebangs ** /.py * /*.sh \
	patchShebangs */.py */.sh \

		description =
		"Options stored in {file}`/etc/openvpn3/log-service.json` configuration file";

	description =
	"Options stored in {file}`/etc/openvpn3/log-service.json` configuration file";
	description = "Options stored in {file}`/etc/openvpn3/log-service.json` configuration file";

		defaultText =
		literalExpression "config.services.resolved.enable";

	defaultText =
	literalExpression "config.services.resolved.enable";
	defaultText = literalExpression "config.services.resolved.enable";

openvpn3: v20 -> v22_dev #326623

Are you sure you want to change the base?

openvpn3: v20 -> v22_dev #326623

Conversation

JarvisCraft commented Jul 12, 2024 • edited Loading

Description of changes

Things done

JarvisCraft commented Jul 21, 2024

nixos-discourse commented Jul 21, 2024

KFearsoff left a comment

Choose a reason for hiding this comment

JarvisCraft commented Jul 22, 2024

nixos-discourse commented Jul 22, 2024

JarvisCraft commented Jul 22, 2024 • edited Loading

JarvisCraft commented Jul 23, 2024

SuperSandro2000 Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JarvisCraft Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JarvisCraft commented Jul 27, 2024

JarvisCraft commented Jul 12, 2024 •

edited

Loading

JarvisCraft commented Jul 22, 2024 •

edited

Loading

SuperSandro2000 Jul 26, 2024 •

edited

Loading

JarvisCraft Jul 26, 2024 •

edited

Loading