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

New package: nextdns-1.6.4 #20022

Closed
wants to merge 1 commit into from
Closed

New package: nextdns-1.6.4 #20022

wants to merge 1 commit into from

Conversation

gspe
Copy link
Contributor

@gspe gspe commented Mar 11, 2020

Close #19905

@smorimoto
Copy link
Contributor

This looks good. Can any of the maintainers review this?


# NextDNS custom configuration id
# change id with your NextDNS id
config id
Copy link
Member

Choose a reason for hiding this comment

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

Can this file be extracted from the download tarball?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,
this file doesn't exist in tarball.
Nextdns doesn't work with Void out of the box because it doesn't support runit system, yet. Nextdns has commands:

Usage: nextdns <command> [arguments]

The commands are:

    install         install service on the system
    uninstall       uninstall service from the system
    start           start installed service
    stop            stop installed service
    restart         restart installed service
    status          return service status
    log             show service logs
    run             run the daemon
    config          manage configuration
    activate        setup the system to use NextDNS as a resolver
    deactivate      restore the resolver configuration
    version         show current version

That interact directly with systemd or upstart or sysv, etc, but not runit

But it's easy to configure it to work with Void, user only have to configure config file manually and run the only NextDNS commands that work: nextdns run so you can start it with something similar:

sudo nextdns run \
  -config <your config id> ...

In order to use it with Void as system service, you have to put your configuration in
/etc/nextdns/nextdns_conf file and symlink /etc/sv/nextdnsd to /var/service

To setup the system to use NextDNS as a dns resolver, un-comment the last line in /etc/resolvconf.conf to name_servers=127.0.0.1 and restart resolvconf with resolvconf -u now NextDNS should be used as dns resolver

@@ -0,0 +1,2 @@
#!/bin/sh
exec /usr/bin/nextdns run -config-file /etc/nextdns/nextdns_conf 2>&1
Copy link
Member

Choose a reason for hiding this comment

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

exec nextdns run -config-file /etc/nextdns/nextdns_conf 2>&1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

homepage="https://nextdns.io/"
distfiles="https://github.com/${pkgname}/${pkgname}/archive/v${version}.tar.gz"
checksum=7a7c0829ca52a711f6fc813e6d913577670c95ddd8b3ab5d17da2b69440a3edc
conf_files="/etc/nextdns/nextdns_conf"
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason this is not .conf? Does upstream prefer _conf ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. The upstream accepts arbitrary file names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The upstream config file is created with the nextdns commands and stored internally somewhere, so I don't know its name exactly

@Vaelatern
Copy link
Member

Other than the small issues above, this seems reasonable

@@ -0,0 +1,19 @@
# Template file for 'nextdns'
pkgname=nextdns
version=1.4.35
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
version=1.4.35
version=1.6.3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

homepage="https://nextdns.io/"
distfiles="https://github.com/${pkgname}/${pkgname}/archive/v${version}.tar.gz"
checksum=7a7c0829ca52a711f6fc813e6d913577670c95ddd8b3ab5d17da2b69440a3edc
conf_files="/etc/nextdns/nextdns_conf"
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
conf_files="/etc/nextdns/nextdns_conf"
conf_files="/etc/nextdns/nextdns.conf"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,2 @@
#!/bin/sh
exec /usr/bin/nextdns run -config-file /etc/nextdns/nextdns_conf 2>&1
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
exec /usr/bin/nextdns run -config-file /etc/nextdns/nextdns_conf 2>&1
exec nextdns run -config-file /etc/nextdns/nextdns_conf 2>&1

@gspe gspe changed the title New package: nextdns-1.4.35 New package: nextdns-1.6.3 May 29, 2020
@smorimoto
Copy link
Contributor

Thanks @gspe!


# NextDNS custom configuration id
# change id with your NextDNS id
config id
Copy link
Member

Choose a reason for hiding this comment

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

I still don't like having this config file in our tree. Is there any way around this?

Copy link
Contributor

Choose a reason for hiding this comment

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

CC: @rs

Copy link

Choose a reason for hiding this comment

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

What would be your recommendation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Vaelatern there are two solutions:

  1. remove this config file from package and tell users to create one them self
  2. add runit support in nextdns, this would be quite easy task since runit is very simple init system to deal with. But some one have to do this

Copy link
Member

Choose a reason for hiding this comment

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

If upstream does not ship with a configuration file, neither do we.

It's ok for the runit script to not work out of the box, and require further reading to get it working. People are expected to be able to read their runit scripts and upstream documentation and make things work.

Copy link
Member

Choose a reason for hiding this comment

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

as of latest commit is not removed from conf_files or otherwise removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@gspe gspe Jun 12, 2020

Choose a reason for hiding this comment

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

Without an example of runit and how to write a config file that runit use this software doesn't work!
So why do you want to include a non working software in the void repo?

Copy link
Member

Choose a reason for hiding this comment

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

Simply put because it's not helpful. If you want to know more, read upstream's docs on getting started. Or read the runit script and figure out what you need. Problem solved.

Example packages that use this model:

  • hooktftp
  • tgt
  • autox
  • nomad
  • consul
  • seaweedfs
  • netauth

are all packages we have where you really should go read upstream's documentation on how to get started. This is a good thing because it means there is less effort necessary to maintain these packages. Lower effort per package means more can be done with our lives, and that's a good thing.

@gspe gspe changed the title New package: nextdns-1.6.3 New package: nextdns-1.6.4 Jun 10, 2020
@gspe gspe force-pushed the nextdns branch 2 times, most recently from 441b2b8 to b70eb11 Compare June 11, 2020 12:02

post_install() {
vsv nextdnsd
vdoc ${FILESDIR}/nextdns.conf.example
Copy link
Contributor

Choose a reason for hiding this comment

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

vdoc or vsconf?

Copy link
Member

Choose a reason for hiding this comment

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

Or total removal. I like total removal.

@@ -0,0 +1,2 @@
#!/bin/sh
exec nextdns run -config-file /etc/nextdns/nextdns.conf 2>&1
Copy link
Member

Choose a reason for hiding this comment

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

You can add a comment above this line, like # See https://example.com/doc for example configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There isn't a clear documentation about config file, the only helpful link is this https://github.com/nextdns/nextdns/wiki/Configuration-File-Format but it doesn't explain that you have to use your config id etc...
So I think that the best thing to do is to provide an example config-file in doc dir, so users are free to use it or do more research them self.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gspe: The best thing to do in this case is to ask upstream to improve their documentation, in the form of a well-commented example config file[1]. Upstream knows the software best, and can make sure such a file is properly maintained and contains the correct information. We shouldn't be trying to take on that task.

[1] Cf. e.g. the extensively-commented sample wpa_supplicant.conf file provided by upstream.

@ericonr ericonr added the new-package This PR adds a new package label Dec 20, 2020
@anubhavkini anubhavkini mentioned this pull request May 9, 2021
3 tasks
@github-actions
Copy link

Pull Requests become stale 90 days after last activity and are closed 14 days after that. If this pull request is still relevant bump it or assign it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-package This PR adds a new package Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package Request: nextdns
7 participants