Skip to content

Conversation

@alesmrazek
Copy link

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Knot Resolver version 6 (knot-resolver6) is an actively developed version of the resolver, unlike the older version 5 (knot-resolver), where only security updates are maintained.

@github-actions github-actions bot added lua Lua use is a significant feature of the PR or issue new formula PR adds a new formula to Homebrew/homebrew-core meson Meson use is a significant feature of the PR or issue labels Dec 8, 2025
Comment on lines 186 to 191
assert_path_exists var/"knot-resolver"
assert_path_exists var/"cache/knot-resolver"
system bin/"kresctl", "--version"
system bin/"knot-resolver", "--version"
Copy link
Member

Choose a reason for hiding this comment

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

I know the current formula uses this test but I'd prefer to add more. See Formula Cookbook:

We want tests that don’t require any user input and test the basic functionality of the application. For example foo build-foo input.foo is a good test and (despite their widespread use) foo --version and foo --help are bad tests. However, a bad test is better than no test at all.

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately, I need a running service to properly test it, but maybe I'll think of something else.

Comment on lines +143 to +165
(bin/"knot-resolver").unlink if (bin/"knot-resolver").exist?
(bin/"knot-resolver").write_env_script libexec/"bin/knot-resolver",
PATH: "#{libexec}/bin:$PATH"

(bin/"kresctl").unlink if (bin/"kresctl").exist?
(bin/"kresctl").write_env_script libexec/"bin/kresctl",
ARGS: "--socket #{var}/knot-resolver/kres-api.sock"
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
(bin/"knot-resolver").unlink if (bin/"knot-resolver").exist?
(bin/"knot-resolver").write_env_script libexec/"bin/knot-resolver",
PATH: "#{libexec}/bin:$PATH"
(bin/"kresctl").unlink if (bin/"kresctl").exist?
(bin/"kresctl").write_env_script libexec/"bin/kresctl",
ARGS: "--socket #{var}/knot-resolver/kres-api.sock"
(bin/"knot-resolver").write_env_script libexec/"bin/knot-resolver",
PATH: "#{libexec}/bin:$PATH"
(bin/"kresctl").write_env_script libexec/"bin/kresctl",
ARGS: "--socket #{var}/knot-resolver/kres-api.sock"

Or remove if modifiers

Comment on lines 156 to 168
<<~EOS
workers: 1
rundir: #{var}/knot-resolver
network:
listen:
- interface:
- 127.0.0.1@53
- ::1@53
cache:
storage: #{var}/cache/knot-resolver
management:
unix-socket: #{var}/knot-resolver/kres-api.sock
EOS
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
<<~EOS
workers: 1
rundir: #{var}/knot-resolver
network:
listen:
- interface:
- 127.0.0.1@53
- ::1@53
cache:
storage: #{var}/cache/knot-resolver
management:
unix-socket: #{var}/knot-resolver/kres-api.sock
EOS
<<~YAML
workers: 1
rundir: #{var}/knot-resolver
network:
listen:
- interface:
- 127.0.0.1@53
- ::1@53
cache:
storage: #{var}/cache/knot-resolver
management:
unix-socket: #{var}/knot-resolver/kres-api.sock
YAML

And it would be nicer to either move config_yaml definition before def install block or just have

    file.write <<~YAML
      workers: 1
      rundir: #{var}/knot-resolver
      network:
        listen:
          - interface:
              - 127.0.0.1@53
              - ::1@53
      cache:
        storage: #{var}/cache/knot-resolver
      management:
        unix-socket: #{var}/knot-resolver/kres-api.sock
    YAML

where it is needed

depends_on "lmdb"
depends_on "luajit"
depends_on "protobuf-c"
depends_on "[email protected]"
Copy link
Member

Choose a reason for hiding this comment

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

why not the latest python?

Copy link
Author

@alesmrazek alesmrazek Dec 8, 2025

Choose a reason for hiding this comment

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

The latest python version (3.14) is not yet supported in 6.0.17 release. Support was added just today.

Comment on lines +172 to +168
(var/"knot-resolver").mkpath
(var/"cache/knot-resolver").mkpath
Copy link
Member

Choose a reason for hiding this comment

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

Move it to def install block

Comment on lines 151 to 152
(buildpath/"config.yaml").write(config_yaml)
(etc/"knot-resolver").install "config.yaml"
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
(buildpath/"config.yaml").write(config_yaml)
(etc/"knot-resolver").install "config.yaml"
(etc/"knot-resolver/config.yaml").write(config_yaml)

Copy link
Author

Choose a reason for hiding this comment

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

I'm getting an error with this suggestion:

Error: An exception occurred within a child process:
  RuntimeError: Will not overwrite /opt/homebrew/Cellar/knot-resolver6/6.0.17/bin/knot-resolver

@daeho-ro
Copy link
Member

daeho-ro commented Dec 9, 2025

Please use head install, I don't think non-stable version is suitable for a versioned formula.

@stefanb
Copy link
Member

stefanb commented Dec 9, 2025

Would it be better to rename existing formula to legacy format [email protected] and add the new v6 as knot-resolver.rb formula?

@daeho-ro
Copy link
Member

daeho-ro commented Dec 9, 2025

If v6 is stable, yes that is better but it doesn't.

@alesmrazek
Copy link
Author

Please use head install, I don't think non-stable version is suitable for a versioned formula.

Version 6 is basically stable and runs in several large production deployments. Unfortunately, we haven't officially released it yet. Version 6.1 will be released soon, which should be officially stable/latest whatever.

Would it be better to rename existing formula to legacy format [email protected] and add the new v6 as knot-resolver.rb formula?

This is something we want in the future, here and on other Linux distributions. For now, we want to keep knot-resolver6 and knot-resolver and slowly transition to knot-resolver and knot-resolver5.

Perhaps, if you think it would be better, we can wait for the release of version 6.1 with this formula.

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

Labels

lua Lua use is a significant feature of the PR or issue meson Meson use is a significant feature of the PR or issue new formula PR adds a new formula to Homebrew/homebrew-core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants