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

v2raya: add cliPackage option #319526

Closed
wants to merge 2 commits into from
Closed

v2raya: add cliPackage option #319526

wants to merge 2 commits into from

Conversation

iopq
Copy link
Contributor

@iopq iopq commented Jun 13, 2024

v2raya can now be run using the xray cli package as well

Description of changes

fixes #271818

Things done

added a cliPackage option

  • 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.11 Release Notes (or backporting 23.11 and 24.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.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 13, 2024
@kirillrdy
Copy link
Member

traling whitespace check fails

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels Jun 13, 2024
@iopq iopq force-pushed the v2raya branch 2 times, most recently from 253a550 to 4730889 Compare June 13, 2024 13:51
@iopq
Copy link
Contributor Author

iopq commented Jun 13, 2024

traling whitespace check fails

fixed

@SuperSandro2000 SuperSandro2000 changed the title v2raya: adding a cliPackage option v2raya: add cliPackage option Jun 26, 2024
@@ -6,11 +6,22 @@ with lib;
options = {
services.v2raya = {
enable = options.mkEnableOption "the v2rayA service";

cliPackage = mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cliPackage = mkOption {
package = mkOption {

Please use mkPackageOption

};
};

config = mkIf config.services.v2raya.enable {
environment.systemPackages = [ pkgs.v2raya ];
environment.systemPackages = [ (pkgs.v2raya.override { v2ray = config.services.v2raya.cliPackage; }) ];
Copy link
Member

Choose a reason for hiding this comment

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

This seems unnecessary complex and inflexible.

Suggested change
environment.systemPackages = [ (pkgs.v2raya.override { v2ray = config.services.v2raya.cliPackage; }) ];
environment.systemPackages = [ cfg.package ];

This makes the configuration more straight forward and flexible, and people can also just overwrite the cli package or the entire package

@@ -33,7 +44,7 @@ with lib;

serviceConfig = {
User = "root";
ExecStart = "${getExe pkgs.v2raya} --log-disable-timestamp";
ExecStart = "${getExe (pkgs.v2raya.override { v2ray = config.services.v2raya.cliPackage; })} --log-disable-timestamp";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ExecStart = "${getExe (pkgs.v2raya.override { v2ray = config.services.v2raya.cliPackage; })} --log-disable-timestamp";
ExecStart = "${getExe cfg.package; })} --log-disable-timestamp";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this doesn't do what you think it does, since the package is v2raya, not xray so this change will break the execstart

Comment on lines 10 to 24
cliPackage = mkOption {
type = types.package;
default = pkgs.v2ray;
defaultText = literalExpression "pkgs.v2ray";
example = literalExpression "pkgs.xray";
description = ''
The package used for the cli binary.
'';
relatedPackages = [ "xray" "v2ray" ];
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cliPackage = mkOption {
type = types.package;
default = pkgs.v2ray;
defaultText = literalExpression "pkgs.v2ray";
example = literalExpression "pkgs.xray";
description = ''
The package used for the cli binary.
'';
relatedPackages = [ "xray" "v2ray" ];
};
package = options.mkPackageOption pkgs "v2raya" { }
cliPackage = options.mkOption {
type = types.package;
default = pkgs.v2ray;
defaultText = literalExpression "pkgs.v2ray";
example = literalExpression "pkgs.xray";
description = ''
The package used for the cli binary.
'';
relatedPackages = [ "xray" "v2ray" ];
};

package almost always refers to the main package used. Since you also need cliPackage, use a separate option for it.
To avoid rebuilds, make sure the package's default v2ray attribute is set to the default for cliPackage.

};
};

config = mkIf config.services.v2raya.enable {
environment.systemPackages = [ pkgs.v2raya ];
environment.systemPackages = [ (pkgs.v2raya.override { v2ray = config.services.v2raya.cliPackage; }) ];
Copy link
Contributor

@SigmaSquadron SigmaSquadron Aug 15, 2024

Choose a reason for hiding this comment

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

Suggested change
environment.systemPackages = [ (pkgs.v2raya.override { v2ray = config.services.v2raya.cliPackage; }) ];
environment.systemPackages = [ (cfg.package.override { v2ray = cfg.cliPackage; }) ];

Ditto for the other instance of the override.

Copy link
Contributor Author

@iopq iopq Aug 15, 2024

Choose a reason for hiding this comment

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

error: undefined variable 'cfg'

after I added

let
cfg = config.services.v2raya;
in

at the top the next error is

error: function 'anonymous lambda' called with unexpected argument 'v2ray'

the one that works is pkgs.v2raya.override

Copy link
Contributor

Choose a reason for hiding this comment

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

Please push your changes, even if they're broken, so we can find the error.

@iopq
Copy link
Contributor Author

iopq commented Aug 15, 2024

I pushed a version where I already fixed the errors

risicle and others added 2 commits August 15, 2024 22:13
@SigmaSquadron
Copy link
Contributor

SigmaSquadron commented Aug 15, 2024

It looks like you accidentally mass-pinged a bunch of people by accidentally requesting
their review, which are now subscribed and getting notifications for everything in this
pull request. Unfortunately, they cannot be automatically unsubscribed from the issue
(removing review request does not unsubscribe), therefore development cannot continue in
this pull request anymore.

Please open a new pull request with your changes, link back to this one and ping the
people actually involved in here over there.

In order to avoid this in the future, there are instructions for how to properly
rebase between branches in our contribution guidelines.
Setting your pull request to draft prior to rebasing is strongly recommended.
In draft status, you can preview the list of people that are about to be requested
for review, which allows you to sidestep this issue.
This is not a bulletproof method though, as OfBorg still does review requests even on draft PRs.

@iopq iopq mentioned this pull request Aug 15, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: policy discussion 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add package option for v2raya so it can use the xray package
5 participants