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

v2ray: init at 4.21.3 #66089

Merged
merged 4 commits into from Dec 3, 2019

Conversation

@servalcatty
Copy link
Contributor

servalcatty commented Aug 5, 2019

Motivation for this change

v2ray: init at 4.20.0
v2ray: init at 4.21.3

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

Copy link
Contributor

jonringer left a comment

otherwise, LGTM
binary seems to work.
maybe add maintainer? :)

pkgs/tools/networking/v2ray/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/v2ray/default.nix Outdated Show resolved Hide resolved
@servalcatty

This comment has been minimized.

Copy link
Contributor Author

servalcatty commented Aug 6, 2019

Some more changes are made to pass asset files in.

@servalcatty

This comment has been minimized.

Copy link
Contributor Author

servalcatty commented Aug 13, 2019

ping @jonringer

@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Aug 13, 2019

unfortunately I'm not too experienced with go to know the best practices.

Nor do I have commit access :(

@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Aug 31, 2019

Also, i would squash all the v2ray commits, and have have the maintainer commit come before. Please adhere to CONTRIBUTING.md when doing the commit messages :)

@servalcatty servalcatty force-pushed the servalcatty:v2ray branch from c45dc3f to 4c4f0ca Aug 31, 2019
@servalcatty

This comment has been minimized.

Copy link
Contributor Author

servalcatty commented Aug 31, 2019

Rebased with some fix-up

@nixos-discourse

This comment has been minimized.

Copy link

nixos-discourse commented Aug 31, 2019

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

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/51

Copy link
Contributor

jonringer left a comment

please squash the commits into two commits, one which adds you, and then 1 for the package

maintainers: add servalcatty
v2ray: init at 4.20.0

if you want the lazy way to do this:

git reset HEAD~6 #dump everything into unstaged
git add  maintainers/maintainer-list.nix
git commit -m "maintainers: add servalcatty"
git add pkgs/
git commit -m "v2ray: init at 4.20.0"
git push servalcatty v2ray --force
@servalcatty servalcatty force-pushed the servalcatty:v2ray branch from 9f69636 to a4cbdc6 Oct 2, 2019
@servalcatty

This comment has been minimized.

Copy link
Contributor Author

servalcatty commented Oct 2, 2019

Squashed and rebased

'';
};

in runCommand "v2ray-${version}" {

This comment has been minimized.

Copy link
@jonringer

jonringer Oct 2, 2019

Contributor

I feel like this could be put into a postFixup phase.

@kalbasit for his go knowledge

This comment has been minimized.

Copy link
@servalcatty

servalcatty Oct 2, 2019

Author Contributor

I make this separated to avoid rebuilding the core when assets updated. Assets are data files (like builtin plugins) and I think they should more extensible.

This comment has been minimized.

Copy link
@jonringer

jonringer Oct 2, 2019

Contributor

that's fair... just seems like a lot going on within a single file. But I don't feel super strongly either way.

@contrun

This comment has been minimized.

Copy link
Contributor

contrun commented Nov 8, 2019

What is blocking this? It has been a while. Can we merge this?

@servalcatty servalcatty force-pushed the servalcatty:v2ray branch from a4cbdc6 to 155e050 Nov 8, 2019
@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Nov 8, 2019

Hmm, was hoping someone else would chime in, a lot of stuff going on here @kalbasit @marsam what are your thoughts?

@servalcatty servalcatty changed the title v2ray: init at 4.20.0 v2ray: init at 4.21.3 Nov 8, 2019
@servalcatty

This comment has been minimized.

Copy link
Contributor Author

servalcatty commented Nov 8, 2019

Upgraded to 4.21.3 and rebased to master.

servalcatty added 3 commits Oct 2, 2019
@servalcatty servalcatty force-pushed the servalcatty:v2ray branch from 155e050 to 740e85f Nov 21, 2019
@servalcatty servalcatty requested a review from Infinisil as a code owner Nov 21, 2019
@ofborg ofborg bot added the 6.topic: nixos label Nov 21, 2019
@servalcatty

This comment has been minimized.

Copy link
Contributor Author

servalcatty commented Nov 21, 2019

Bumped versions of assets and added v2ray service module.

Copy link
Contributor

marsam left a comment

Sorry for the delay, life has been keeping me quite busy these days.
I left a pair of comments, but looks good overall.

nixos/modules/services/networking/v2ray.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/v2ray/generic.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/v2ray/generic.nix Outdated Show resolved Hide resolved
@servalcatty

This comment has been minimized.

Copy link
Contributor Author

servalcatty commented Nov 27, 2019

@marsam
buildGoModule works. Thanks.

But since two modules v2ray.com/core/main and v2ray.com/core/infra/control/main have the same name part, using subPackages will overwrite outputs.
I don't know how to pass different -o flags between two builds.

So now I just keep the custom build and install phases.

@marsam
marsam approved these changes Dec 3, 2019
@marsam marsam merged commit 93ff044 into NixOS:master Dec 3, 2019
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
v2ray on aarch64-linux Success
Details
v2ray on x86_64-linux Success
Details
@marsam

This comment has been minimized.

Copy link
Contributor

marsam commented Dec 3, 2019

@servalcatty thanks for your contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.