lorder: skip tests if cc not available

[olivier]

Summary

On system without compilere installed the tests are failing:

 $ kyua test usr.bin/lorder/
usr.bin/lorder/lorder_test:noargs  ->  passed  [0.019s]
usr.bin/lorder/lorder_test:dashdash  ->  failed: atf-check failed; see the output of the test for details  [0.029s]
usr.bin/lorder/lorder_test:invalid  ->  passed  [0.029s]
usr.bin/lorder/lorder_test:nonexistent  ->  passed  [0.028s]
usr.bin/lorder/lorder_test:onearg  ->  failed: atf-check failed; see the output of the test for details  [0.028s]
usr.bin/lorder/lorder_test:objects  ->  failed: atf-check failed; see the output of the test for details  [0.030s]
usr.bin/lorder/lorder_test:archives  ->  failed: atf-check failed; see the output of the test for details  [0.045s]

3/7 passed (4 failed)

Add a check for cc on tests using it.

Test Plan

Skip if cc is missing:

# mv /usr/bin/cc /usr/bin/cc.bak
# kyua test usr.bin/lorder/
usr.bin/lorder/lorder_test:archives  ->  skipped: The required program cc could not be found in the PATH  [0.009s]
usr.bin/lorder/lorder_test:dashdash  ->  skipped: The required program cc could not be found in the PATH  [0.010s]
usr.bin/lorder/lorder_test:objects  ->  skipped: The required program cc could not be found in the PATH  [0.010s]
usr.bin/lorder/lorder_test:onearg  ->  skipped: The required program cc could not be found in the PATH  [0.010s]
usr.bin/lorder/lorder_test:noargs  ->  passed  [0.014s]
usr.bin/lorder/lorder_test:invalid  ->  passed  [0.020s]
usr.bin/lorder/lorder_test:nonexistent  ->  passed  [0.019s]

7/7 passed (0 failed)

[des]

Why are you testing lorder, which is part of the toolchain, on a system without a toolchain?

[olivier]

In D44548#1015876, @des wrote:

Why are you testing lorder, which is part of the toolchain, on a system without a toolchain?

I’m just running the regression tests from a FreeBSD with a make.conf full of WITHOUT_
And since the introduction of those tests, I’m noticing those new tests are failing.
So one idea is too declare the requiere_prog to avoid that.

[des]

I think D44558 is a better approach.

[des]

Now that I think of it, this isn't enough, because lorder itself uses nm, which is also conditional on MK_TOOLCHAIN. Also, the canonical way to do this is to add atf_set require.progs to the test case head, which allows Kyua to skip the test entirely, instead of trying to run it and then ignoring the failure.

[olivier]

In D44548#1016031, @des wrote:

I think D44558 is a better approach.

Yes: Thanks

[olivier]

Update test to latest sync, and add nm in requiere list too.

[ngie]

Specifying this in the Makefile is significantly less code than doing it in the atf tests:

TEST_METADATA.lorder_test+=  required_programs="ar cc lorder nm"

This metadata ends up in the Kyuafile.

[olivier]

In D44548#1068628, @ngie wrote:

Specifying this in the Makefile is significantly less code than doing it in the atf tests:

TEST_METADATA.lorder_test+=  required_programs="ar cc lorder nm"

This metadata ends up in the Kyuafile.

I didn’t know we could do that !
Thanks! I will fix it.

[des]

In D44548#1068628, @ngie wrote:

Specifying this in the Makefile is significantly less code than doing it in the atf tests:

TEST_METADATA.lorder_test+=  required_programs="ar cc lorder nm"

This metadata ends up in the Kyuafile.

Yes, it's less code, but I happen to know that lorder _and these tests_ are also used by downstream projects, and separating the requirements from the tests themselves will cause difficulties for those downstream consumers. Olivier's patch is fine as it is, although I'm not sure it is necessary to require lorder in a test meant to exercise lorder since if lorder is not installed, then neither is the test.

[des]

usr.bin/lorder/tests/lorder_test.sh

12

This is redundant since the test is only installed if lorder is installed.

[des]

This is still relevant, but needs changes:

• I think it's fair to assume that lorder is present if its test is present, so we shouldn't test for it.
• Since lorder requires nm, it's also fair to assume that nm is present if lorder is present (or rather: it's not the test's responsibility, and if lorder is present but nm is missing, I would rather see the test fail than skip).
• This leaves ar and cc which I agree that the test should explicitly require.
• In pkgbase, lorder is in toolchain while ar, cc, and nm are in clang; this needs to be addresses but is outside the scope of this review.

[ivy]

In D44548#1177316, @des wrote:

• I think it's fair to assume that lorder is present if its test is present,

i don't think this is true with pkgbase because all the tests are lumped into a single FreeBSD-tests package, so it's quite easy to have a test installed without the thing that's being tested. sometimes FreeBSD-tests has the correct dependency, but this is only by accident if a compiled test has a shlib requirement.

so we shouldn't test for it.

... however, i tend to agree with this: it doesn't make sense for a test to have to test that the test subject is installed. instead it shouldn't be possible to end up in a situation where this is false to begin with. i've been vaguely aware of this issue for a while but i'm not sure what the right fix is; the nicest solution might be to move tests to their own packages (e.g., FreeBSD-toolchain-tests) which would depend on the base package. this may not be possible for all tests but looking at how /usr/tests/usr.bin/Kyuafile works, i think a lot could be moved.

• Since lorder requires nm, it's also fair to assume that nm is present if lorder is present (or rather: it's not the test's responsibility, and if lorder is present but nm is missing, I would rather see the test fail than skip).
• This leaves ar and cc which I agree that the test should explicitly require.

i don't have a strong opinion here but this seems reasonable to me.

• In pkgbase, lorder is in toolchain while ar, cc, and nm are in clang; this needs to be addresses but is outside the scope of this review.

as mentioned on Slack, we might want to move ar and nm from clang to toolchain; i remember there was a reason i didn't do this when i created the toolchain package but i think it was simply that moving them wasn't obviously correct, so i left it until later.