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

Jump to bottom

v2ray: init at 4.21.3 #66089

Merged
merged 4 commits into from Dec 3, 2019
Merged

v2ray: init at 4.21.3 #66089

merged 4 commits into from Dec 3, 2019

Conversation

servalcatty
Copy link
Contributor

@servalcatty servalcatty commented Aug 5, 2019 •
edited

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 @

jonringer
jonringer approved these changes Aug 5, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Some more changes are made to pass asset files in.

@servalcatty
Copy link
Contributor Author

ping @jonringer

@jonringer
Copy link
Contributor

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

Nor do I have commit access :(

@jonringer
Copy link
Contributor

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
Copy link
Contributor Author

Rebased with some fix-up

@nixos-discourse
Copy link

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

jonringer
jonringer reviewed Sep 30, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Squashed and rebased

jonringer
jonringer reviewed Oct 2, 2019
View reviewed changes
pkgs/tools/networking/v2ray/generic.nix
'';
};

in runCommand "v2ray-${version}" {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@kalbasit for his go knowledge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@contrun
Copy link
Contributor

contrun commented Nov 8, 2019

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

@jonringer
Copy link
Contributor

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
Copy link
Contributor Author

Upgraded to 4.21.3 and rebased to master.

@servalcatty
Copy link
Contributor Author

Bumped versions of assets and added v2ray service module.

marsam
marsam reviewed Nov 24, 2019
View reviewed changes
Copy link
Contributor

@marsam marsam left a comment

Choose a reason for hiding this comment

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

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
nixos/modules/services/networking/v2ray.nix 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
Copy link
Contributor Author

@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 merged commit 93ff044 into NixOS:master Dec 3, 2019
@marsam
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
Reviewers

@jonringer jonringer jonringer approved these changes

@aanderse aanderse aanderse left review comments

@marsam marsam marsam approved these changes

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 11.by: package-maintainer 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@servalcatty @jonringer @nixos-discourse @contrun @marsam @aanderse @Janik-Haag