Comment 1 Nuno Teixeira 2023-02-06 10:29:28 UTC

---
Checking patch dns/knot-resolver/Makefile...
error: while searching for:
PORTNAME=      knot-resolver
DISTVERSION=   5.5.2
PORTREVISION=  2
CATEGORIES=    dns
MASTER_SITES=  https://secure.nic.cz/files/knot-resolver/ \
               https://dns.company/downloads/knot-resolver/

error: patch failed: dns/knot-resolver/Makefile:1
---

tabs replaced by spaces

Comment 2 Nuno Teixeira 2023-02-06 11:09:57 UTC

Hello,

Is there any specific reason to options TEST and TESTUNIT be enabled by default?

Cheers

Comment 3 Nuno Teixeira 2023-02-06 11:12:56 UTC

(In reply to Nuno Teixeira from comment #2)
(...)

DOCS EXAMPLES (NLS IPV6) are enabled by default and could be removed from OPTIONS_DEFAULT

Created attachment 239944 [details]
knot-resolver 5.6.0 minus defaults

(In reply to Nuno Teixeira from comment #1)

> tabs replaced by spaces
>
That's super annoying.
It happens when I copy/paste the output of `git diff`
When I do `git diff > file`
and then copy paste from `cat file` it's fine.

> reason to options TEST and TESTUNIT be enabled by default
>
They are all different tests (next to TESTCONF TESTEXTRA).
I enabled them because I thought it was a reasonable level, while the TESTCONF TESTEXTRA are tests of less importance during install.

> DOCS EXAMPLES (NLS IPV6) are enabled by default and could be removed from OPTIONS_DEFAULT
>
Good catch! Thanks.
Did so in this patch (identical to earlier patch, except for this mutation).

Comment 5 Nuno Teixeira 2023-02-06 12:46:21 UTC

(In reply to Leo Vandewoestijne from comment #4)

I forgot to say that tests options are usually off by default since user can do further tests using test targets, e.g, `make test`, etc, and they will do extra build on build servers.

My recommendation is to turn off tests by default.

Created attachment 239945 [details]
knot-resolver 5.6.0 minus defaults minus tests

(In reply to Nuno Teixeira from comment #5)

OK, this patch removes them.
(and should apply w/o tab/space problem)

Comment 7 Nuno Teixeira 2023-02-06 13:55:56 UTC

(In reply to Leo Vandewoestijne from comment #6)

OK, almost there:

TESTUNIT, the only test working (TESTCONF and TESTEXTRA are defined as broken) works as expected when TESTUNIT option is ON (or `make test` after build):

---
optional components
    ...
    unit_tests:         enabled
    config_tests:       disabled
    extra_tests:        disabled

[  0% 1/1] /usr/local/bin/meson test --no-rebuild --print-errorlogs
1/9 unit.array   OK              0.03s
2/9 unit.lru     OK              0.03s
3/9 unit.pack    OK              0.02s
4/9 unit.queue   OK              0.02s
5/9 unit.trie    OK              0.02s
6/9 unit.module  OK              0.02s
7/9 unit.rplan   OK              0.02s
8/9 unit.utils   OK              0.01s
9/9 unit.zonecut OK              0.01s

Ok:                 9
Expected Fail:      0
Fail:               0
Unexpected Pass:    0
Skipped:            0
Timeout:            0
---

What TEST option do since its target is defined as:

---
pre-install-TEST-on: do-test
---
?

In my opinion, TEST could be removed from OPTIONS_DEFINE toguether with its target.

Could you check it?

Thanks

(In reply to Nuno Teixeira from comment #7)

When submitting mutation to the original patch, I've already tested that (using poudriere).
But just ago I did so manually. All fine.

(In reply to Nuno Teixeira from comment #7)

So although manual turns out fine, isn't the TEST / pre-install-TEST-on not useful when using portinstall?
(or is that pointless)?

Comment 10 Nuno Teixeira 2023-02-07 08:26:44 UTC

(In reply to Leo Vandewoestijne from comment #9)

I see it as pointless and it is the first time that I see it in pre-install phase.

Since this port have 3 kind of tests, I would keep the corresponding options "TESTUNIT TESTCONF TESTEXTRA" and remove TEST (and its pre-install target).

Other question: is there an open issue about broken tests TESTCONF and TESTEXTRA?

Created attachment 239966 [details]
knot-resolver 5.6.0 options revisited

(In reply to Nuno Teixeira from comment #10)

After having another look at the meson options file...

TEST:
Happens to actually be enabled by default, so I remove the option.

TESTUNIT:
Default is auto. Requires cmocka, otherwise skips it, so I keep that option.

TESTCONFIG:
Is enabled by default IF those impossible dependencies are there.
So I'll remove it.

TESTEXTRA:
I can't remember when the TESTEXTRA_BROKEN appeared / if the missing CMakeLists.txt ever was reported. Since it's not in high demand for ages I'll remove it.

Comment 12 Nuno Teixeira 2023-02-07 15:32:49 UTC

(In reply to Leo Vandewoestijne from comment #11)

Looks good. I will do some testing and commit with just an exception:

==> keep:

TESTUNIT_MESON_ON=     -Dunit_tests=enabled
TESTUNIT_MESON_OFF=    -Dunit_tests=disabled

so we have control on meson config: (auto, enable, disable) to never be set to 'auto' because it can be triggered to 'enable' by the presence of it's dependency mocka on systems that might be have it installed.

Comment 13 Nuno Teixeira 2023-02-07 17:12:54 UTC

(In reply to Nuno Teixeira from comment #12)
(...)

And changed TESTUNIT description to:
TESTUNIT_DESC=         Build unit tests

So user can do a `make test` after build.

Comment 14 commit-hook 2023-02-07 17:38:36 UTC

A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=0cd67390c61090ae6c3a213b06c891e6a9e76ddb

commit 0cd67390c61090ae6c3a213b06c891e6a9e76ddb
Author:     Leo Vandewoestijne <freebsd@dns.company>
AuthorDate: 2023-02-07 17:28:30 +0000
Commit:     Nuno Teixeira <eduardo@FreeBSD.org>
CommitDate: 2023-02-07 17:37:15 +0000

    dns/knot-resolver: Update to 5.6.0

     - Remove unused TEST option
     - Remove broken TESTCONFIG and TESTEXTRA options
     - Update TESTUNIT option description to reflect that it only builds
       tests so `make test` can be run after

    ChangeLog:      https://www.knot-resolver.cz/2023-01-26-knot-resolver-5.6.0.html
    PR:             269368

 dns/knot-resolver/Makefile | 31 +++++--------------------------
 dns/knot-resolver/distinfo |  6 +++---
 2 files changed, 8 insertions(+), 29 deletions(-)

Comment 15 Nuno Teixeira 2023-02-07 17:46:32 UTC

Please use this PR is something is missing or need to be adjusted/fixed.

Committed, thanks!