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

smartdns: init at 30 #80931

Merged
merged 3 commits into from Mar 15, 2020
Merged

smartdns: init at 30 #80931

merged 3 commits into from Mar 15, 2020

Conversation

LEXUGE
Copy link
Contributor

@LEXUGE LEXUGE commented Feb 24, 2020 •
edited

Motivation for this change

For Mainland China NixOS users like me. It is very troublesome to have a fast while un-polluted DNS server. Some local servers like stubby + dnsmasq isn't fast enough in terms of either resolving or using the result. And SmartDNS, A local DNS server to obtain the fastest website IP for the best Internet experience, is capable to solve this issue. Thus I packaged it.

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.

@LEXUGE LEXUGE changed the title Add smartdns package smartdns: init at 29 Feb 24, 2020
@LEXUGE LEXUGE requested a review from aanderse February 24, 2020 03:53
jonringer
jonringer reviewed Feb 24, 2020
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.

should be 2 commits:

maintainers: add lexuge
smartdns: init at 29

pkgs/tools/networking/smartdns/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/smartdns/default.nix Outdated Show resolved Hide resolved
@LEXUGE
Copy link
Contributor Author

LEXUGE commented Feb 24, 2020 •
edited

should be 2 commits:

maintainers: add lexuge
smartdns: init at 29

I have squashed the commits.

@LEXUGE
Copy link
Contributor Author

LEXUGE commented Feb 26, 2020

@jtojnar I think patchFlag is OK since upstream may fix this issue after one or two releases. So this temporary solution is acceptable because the situation would not hold long. Now, the patch format, as I said, prevents me from getting around flag because of the .old and .new format. So I think we should keep it temporarily.

@LEXUGE LEXUGE requested a review from jtojnar February 26, 2020 03:36
@jtojnar
Copy link
Contributor

jtojnar commented Feb 26, 2020

Verified that the FHS branch works correctly with the following patch:

--- a/pkgs/tools/networking/smartdns/default.nix
+++ b/pkgs/tools/networking/smartdns/default.nix
@@ -4,41 +4,28 @@ stdenv.mkDerivation rec {
   pname = "smartdns";
   version = "30";
 
-  src = fetchFromGitHub {
-    owner = "pymumu";
-    repo = pname;
-    rev = "Release${version}";
-    sha256 = "1zj1ajvdnpmdal87j7fyn0xdd0ydjii1nvgwgacv2j311b6fdrk7";
-  };
-
-  patches = [
-    # TODO: A patch is needed by now since the upstream doesn't follow the FHS guideline. It should not be the case in the future.
-    (fetchpatch {
-      url =
-        "https://git.archlinux.org/svntogit/community.git/plain/trunk/systemd.patch?h=packages/smartdns&id=c15e81742c93e24c47de59a956f1d0d0f7156f0a";
-      name = "systemd.nix";
-      sha256 = "0jxzsgxz6y1a4mvwk6d5carxv4nxsswpfs1a6jkplzy2gr0g8281";
-    })
-  ];
-
-  patchFlags = [ "-p1" "systemd/smartdns.service" ];
+  # src = fetchFromGitHub {
+  #   owner = "pymumu";
+  #   repo = pname;
+  #   rev = "Release${version}";
+  #   sha256 = "1zj1ajvdnpmdal87j7fyn0xdd0ydjii1nvgwgacv2j311b6fdrk7";
+  # };
 
-  postPatch = ''
-    substituteInPlace systemd/smartdns.service --replace /usr/bin/smartdns $out/bin/smartdns
-  '';
+  src = /home/jtojnar/Projects/smartdns;
 
-  buildInputs = [ openssl ];
+  buildInputs = [
+    openssl
+  ];
 
-  makeFlags = [ "-C" "src" ];
+  makeFlags = [
+    "PREFIX=${placeholder "out"}"
+    "SYSTEMDSYSTEMUNITDIR=${placeholder "out"}/lib/systemd/system"
+    "RUNSTATEDIR=/run"
+  ];
 
-  # TODO: Manual installation is needed by now due to upstream issue.
-  installPhase = ''
-    runHook preInstall
-    install -Dm644 systemd/smartdns.service $out/etc/systemd/system/smartdns.service
-    install -Dm644 etc/default/smartdns $out/etc/default/smartdns
-    install -Dm755 src/smartdns $out/bin/smartdns
-    runHook postInstall
-  '';
+  installFlags = [
+    "SYSCONFDIR=${placeholder "out"}/etc"
+  ];
 
   meta = with stdenv.lib; {
     description =

@LEXUGE
Copy link
Contributor Author

LEXUGE commented Feb 27, 2020 •
edited

@jtojnar But somehow the newly merged PR is not in the recent release， which means I should not use release version but use commit sha256 to pin the src?

@LEXUGE
Copy link
Contributor Author

LEXUGE commented Feb 29, 2020

@jonringer I have updated the commits with the newest commits in smartdns.

@LEXUGE
Copy link
Contributor Author

LEXUGE commented Mar 5, 2020

Any update here?

jtojnar
jtojnar reviewed Mar 12, 2020
View reviewed changes
nixos/modules/services/networking/smartdns.nix Outdated
Comment on lines 34 to 44
example = literalExample ''
settings = {
bind = ":5353 -no-rule -group example";
cache-size = 4096;
server-tls = [ "8.8.8.8:853" " 1.1.1.1:853" ];
server-https = "https://cloudflare-dns.com/dns-query -exclude-default-group";
prefetch-domain = true;
speed-check-mode = "ping,tcp:80";
};
'';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use

Suggested change
example = literalExample ''
settings = {
bind = ":5353 -no-rule -group example";
cache-size = 4096;
server-tls = [ "8.8.8.8:853" " 1.1.1.1:853" ];
server-https = "https://cloudflare-dns.com/dns-query -exclude-default-group";
prefetch-domain = true;
speed-check-mode = "ping,tcp:80";
};
'';
example = {
bind = ":5353 -no-rule -group example";
cache-size = 4096;
server-tls = [ "8.8.8.8:853" " 1.1.1.1:853" ];
server-https = "https://cloudflare-dns.com/dns-query -exclude-default-group";
prefetch-domain = true;
speed-check-mode = "ping,tcp:80";
};

Copy link
Member

Choose a reason for hiding this comment

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

This wouldn't render correctly in the manual

infinisil
infinisil approved these changes Mar 13, 2020
View reviewed changes
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Looking good to me!

@LEXUGE
Copy link
Contributor Author

LEXUGE commented Mar 13, 2020

Looking good to me!

Great. What should I do next then?

@infinisil
Copy link
Member

Let's wait a bit for @jtojnar's approval

@LEXUGE LEXUGE requested a review from jtojnar March 13, 2020 14:27
@infinisil infinisil merged commit 779b7ff into NixOS:master Mar 15, 2020
@LEXUGE
Copy link
Contributor Author

LEXUGE commented Mar 15, 2020

Thanks for a lot of help! @jonringer @infinisil

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/modifying-environement-etc-file-from-multiple-imports/6375/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar left review comments

@infinisil infinisil infinisil approved these changes

@doronbehar doronbehar doronbehar approved these changes

@jonringer jonringer Awaiting requested review from jonringer

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
@LEXUGE @jonringer @nixos-discourse @jtojnar @infinisil @doronbehar @Janik-Haag