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

doh-proxy-rust: init at 0.3.8 #91318

Merged
merged 2 commits into from Apr 8, 2021
Merged

doh-proxy-rust: init at 0.3.8 #91318

merged 2 commits into from Apr 8, 2021

Conversation

stephank
Copy link
Contributor

@stephank stephank commented Jun 22, 2020 •
edited

Motivation for this change

Using this on my own NixOS server, thought I'd contribute back. Note that this is my first contribution!

Hopefully, doh-proxy-rust is the right naming for this package. It conflicts with facebookexperimental/doh-proxy. (already packaged)

I've added myself as maintainer, which I'm happy to do, but am also unsure how to best approach. For example, I don't actively monitor upstream for releases, but am curious if this is automated somehow. But I'm at least available to review issues/PRs whenever tagged, if that helps.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.

fzakaria
fzakaria reviewed Aug 17, 2020
View reviewed changes
Copy link
Contributor

@fzakaria fzakaria left a comment

Choose a reason for hiding this comment

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

the derivation looks good to me; but i don't have familiarity enough on NixOS to comment on the module.
I would rebase your commit though so it's:

  1. add maintainer
  2. init doh-proxy

Looks like nixpkgs is already getting into namespace issues :(

maintainers/maintainer-list.nix Show resolved Hide resolved
@stephank
Copy link
Contributor Author

Thanks for the review! I rebased on master, redid the commits, and also updated Cargo.lock for minor dep upgrades. (The original project doesn't have a lockfile.)

SuperSandro2000
SuperSandro2000 reviewed Jan 18, 2021
View reviewed changes
pkgs/servers/dns/doh-proxy-rust/default.nix Outdated
description = "Fast, mature, secure DoH server proxy written in Rust";
license = with licenses; [ mit ];
maintainers = with maintainers; [ stephank ];
platforms = platforms.all;
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
platforms = platforms.all;

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 rebased on master with this change.

@SuperSandro2000
Copy link
Member

@ofborg eval

@stephank stephank changed the title doh-proxy-rust: init at 0.3.3 doh-proxy-rust: init at 0.3.8 Apr 2, 2021
@stephank
Copy link
Contributor Author

stephank commented Apr 2, 2021

I rebased this on master and upgraded to 0.3.8, but the test still needs some work. Will need a bit of time to look into this.

@stephank
Copy link
Contributor Author

stephank commented Apr 2, 2021

Turns out, the original test was also flawed, but miraculously succeeded. This is now fixed.

SuperSandro2000
SuperSandro2000 requested changes Apr 2, 2021
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

@aanderse could you take a 2nd look at the module?

pkgs/servers/dns/doh-proxy-rust/cargo-lock.patch
@@ -0,0 +1,838 @@
diff --git a/Cargo.lock b/Cargo.lock
Copy link
Member

Choose a reason for hiding this comment

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

It would be great if you could try to upstream this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, upstream has rejected this in the past: DNSCrypt/doh-server#31

nixos/tests/doh-proxy-rust.nix Outdated
@@ -0,0 +1,43 @@
import ./make-test-python.nix ({ pkgs, ... }: {
name = "doh-proxy-rust";
meta = with pkgs.stdenv.lib.maintainers; {
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
meta = with pkgs.stdenv.lib.maintainers; {
meta = with lib.maintainers; {

nixos/tests/doh-proxy-rust.nix Outdated
@@ -0,0 +1,43 @@
import ./make-test-python.nix ({ pkgs, ... }: {
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
import ./make-test-python.nix ({ pkgs, ... }: {
import ./make-test-python.nix ({ lib, pkgs, ... }: {

aanderse
aanderse reviewed Apr 2, 2021
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

A nice and simple module 👍 Just a few points for discussion.

nixos/modules/services/networking/doh-proxy-rust.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/doh-proxy-rust.nix Outdated
description = "doh-proxy-rust";
after = [ "network.target" "nss-lookup.target" ];
wantedBy = [ "multi-user.target" ];
confinement = {
Copy link
Member

Choose a reason for hiding this comment

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

While I'm sure @aszlig would love to see this... we don't actually use confinement anywhere else in our module system.

@flokli @andir @arianvp @Mic92 what do you all think about using confinement in upstream NixOS modules?

nixos/modules/services/networking/doh-proxy-rust.nix Outdated Show resolved Hide resolved
@aanderse
Copy link
Member

aanderse commented Apr 8, 2021

Looks good @stephank 👍

@SuperSandro2000 SuperSandro2000 merged commit 26f16c1 into NixOS:master Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fzakaria fzakaria fzakaria left review comments

@aanderse aanderse aanderse left review comments

@SuperSandro2000 SuperSandro2000 SuperSandro2000 requested changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 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
5 participants
@stephank @SuperSandro2000 @aanderse @fzakaria @Janik-Haag