[RFC] Rename `make test` in suite.test.mk with `make regress`

[Garrett Cooper]

> On Oct 17, 2015, at 21:27, NGie Cooper <yaneurabeya at gmail.com> wrote:
> 
>>> On Oct 17, 2015, at 19:21, Julio Merino <jmmv at freebsd.org> wrote:
>>> 
>>>> On Oct 17, 2015, at 21:56, NGie Cooper <yaneurabeya at gmail.com> wrote:
>>>> 
>>>> 
>>>> On Oct 17, 2015, at 18:50, Julio Merino <jmmv at freebsd.org> wrote:
>>>> 
>>>>> On Oct 17, 2015, at 18:03, NGie Cooper <yaneurabeya at gmail.com> wrote:
>>>>> 
>>>>> Hi all,
>>>>> 	There’s a lesser known target in suite.test.mk that runs `kyua test` in a similar manner to how Jenkins and other groups have integrated kyua into their test infrastructures.
>>>>> 	The legacy target on FreeBSD was `regress`, but the target created with the bsd.test.mk creation back a few years ago was `test`. Why change from `test` to `regress`? There are places in the tree (bin/test for example) that have targets named test, so in order to avoid clashing with a common target (name), it’s best to use the legacy target name.
>>>>> 	Would anyone have any serious heartburn over the change?
>>>> 
>>>> Is this only because of bin/test?  Seems like renaming a target to avoid that one collision is just moving the problem around.  It is possible that some other directory could later grow a target that conflicts with your new name.
>>> 
>>> Yes, I realize that. The goal though is I want to be able to call `make <something>` from the top and it would iterate down each and every subdirectory and run tests.
>> 
>> Other for the bin/test naming collision, that should work already, shouldn't it?
> 
> make test isn’t currently hooked in to any subdirectories. So this is what happens today:
> 
> $ make -VRELDIR
> bin
> $ make test
> ===> tests (buildconfig)
> ===> tests (all)
> $ cd test
> $ make test
> `test' is up to date.
> $
> 
> If I fake hooking it up, things get a bit weird because it’s now going to evaluate all targets named `test`, which currently causes a build break with bin/sh and the builds test binary when I do it from bin/ :
> 
> $ env SUBDIR_TARGETS=test make test
> ===> tests (test)
> *** Using this test does not preclude you from running the tests
> *** installed in /usr/tests.  This test run may raise false
> *** positives and/or false negatives.
> 
> *** WARNING: make test is experimental
> ***
> *** Using this test does not preclude you from running the tests
> *** installed in /usr/tests.  This test run may raise false
> *** positives and/or false negatives.
> 
> legacy_test:main  ->  passed  [0.056s]
> 
> Results file id is usr_tests_bin_test.20151018-023326-624852
> Results saved to /home/ngie/.kyua/store/results.usr_tests_bin_test.20151018-023326-624852.db
> 
> 1/1 passed (0 failed)
> 
> *** Once again, note that make test is unsupported.
> cc -O2 -pipe -fno-strict-aliasing -O2 -pipe   -std=gnu99 -fstack-protector-strong -Wsystem-headers -Werror -Wall -Wno-format-y2k -W -Wno-unused-parameter -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wreturn-type -Wcast-qual -Wwrite-strings -Wswitch -Wshadow -Wunused-parameter -Wcast-align -Wchar-subscripts -Winline -Wnested-externs -Wredundant-decls -Wold-style-definition -Wno-pointer-sign -Wmissing-variable-declarations -Wthread-safety -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable -Qunused-arguments  -o test test.o
> 
> Neither case works, so this is why test is currently out of the picture.
> 
>> Not sure how ugly this would be, but you'd also define an internal "run-tests" target or similar that is used exclusively by the recursion code.  Then, in the top-level Makefile, you define a bare "test" target that starts the recursion so that "make test" works.  And then, in bsd.test.mk, you define a "test" target if - and only if - there is no "test" target yet.  bin/test would behave differently than the rest if you ever went in there and ran "make test", but I think that's a minor problem.
> 
> I’m pretty sure make testworld would be ok for a top-level target. Honestly though, I don’t think people really care as long as they can execute one command and it would do everything for them (run_tests, etc).
> 
>> This is offtopic, but do you have a plan anywhere on how to make the tests invocation this way reliable?  Without installed binaries, the tests might not do the right thing, and I'm not sure if the object tree is enough for this to work properly.  That's the reason why "make test" from the top-level directory is currently disallowed.
> 
> I can’t make it 100% reliable and it’s not a replacement for really running the tests or doing other CI testing as the tests might rely on tools on the base system. As a base set of requirements:
> - Compile the tests
> - Install the tests and supporting files to a temporary directory under MAKEOBJDIRPREFIX.
> - Run the tests
> 
> Once this has been resolved, we can start looking at other items, like:
> - How 
> 
>> I think I see what you mean: that because "regress" already existed in the FreeBSD tree, no other targets were introduced to collide with it?  If yes, I wouldn't expect that to be the case.  Tests were not interleaved with the source tree in the past, so the "regress" target only existed in a subset of the tree, didn't it?
> 
> Correct. It’s been established as a “hands-off” target for some time.
> 
> You’re correct about regress only existing in part of the tree but in reality it was hooked in to other places in the build system, even if it was unused outside tools/regression:
> 
> $ grep -r regress share/mk Makefile*
> share/mk/bsd.subdir.mk:             realinstall regress tags \
> share/mk/bsd.sys.mk:            realdepend realinstall regress subdir-all subdir-depend \
> Makefile:       obj objlink regress rerelease showconfig tags toolchain update \
> $
> 
> I admit that it’s a poorly named target, but would probably raise less eyebrows than telling people to invoke “kyua test” has in the past/would...
> 
>>>> Have you considered "check"?  That'd be in line with what automake does, for example, which would homogenize the target name with a ton of other projects out there.
>>> 
>>> … `make check` would be ok too. I just have to comb the tree for binaries that don’t match `check`.
> 
> This doesn’t seem like a doable option right now, but I’ll need to do more investigation as this checkout doesn’t have the rest of my changes:
> 
> $ find . -name check.\*
> ./sbin/fsck_msdosfs/check.c
> ./tools/build/make_check/check.mk
> ./contrib/xz/src/liblzma/api/lzma/check.h
> ./contrib/xz/src/liblzma/check/check.c
> ./contrib/xz/src/liblzma/check/check.h
> ./contrib/atf/atf-c/check.c
> ./contrib/atf/atf-c/check.h
> ./contrib/atf/atf-c++/check.cpp
> ./contrib/atf/atf-c++/check.hpp
> ./contrib/serf/build/check.py
> ./contrib/ntp/sntp/libopts/check.c
> ./crypto/heimdal/kadmin/check.c
> $ cd sbin/fsck_msdosfs; make check
> cc -O2 -pipe -fno-strict-aliasing -O2 -pipe   -I/usr/src/svn/sbin/fsck_msdosfs/../fsck -std=gnu99 -fstack-protector-strong -Wsystem-headers -Werror -Wall -Wno-format-y2k -W -Wno-unused-parameter -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wreturn-type -Wcast-qual -Wwrite-strings -Wswitch -Wshadow -Wunused-parameter -Wcast-align -Wchar-subscripts -Winline -Wnested-externs -Wredundant-decls -Wold-style-definition -Wno-pointer-sign -Wmissing-variable-declarations -Wthread-safety -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable -Qunused-arguments  /usr/src/svn/sbin/fsck_msdosfs/check.c  -o check
> /usr/lib/crt1.o: In function `_start':
> /usr/src/git/lib/csu/amd64/crt1.c:(.text+0x17b): undefined reference to `main'
> /tmp/check-014c0d.o: In function `checkfilesys':
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x2f): undefined reference to `alwaysno'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x35): undefined reference to `rdonly'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x3b): undefined reference to `preen'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x53): undefined reference to `rdonly'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x77): undefined reference to `rdonly'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x9d): undefined reference to `pwarn'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0xa3): undefined reference to `rdonly'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0xaf): undefined reference to `preen'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0xc4): undefined reference to `preen'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0xd7): undefined reference to `rdonly'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0xef): undefined reference to `readboot'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x10c): undefined reference to `perr'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x148): undefined reference to `preen'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x14e): undefined reference to `skipclean'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x164): undefined reference to `checkdirty'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x191): undefined reference to `preen'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x1cc): undefined reference to `readfat'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x235): undefined reference to `readfat'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x25c): undefined reference to `comparefat'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x28a): undefined reference to `preen'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x2aa): undefined reference to `checkfat'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x2cb): undefined reference to `preen'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x2eb): undefined reference to `resetDosDirSection'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x317): undefined reference to `handleDirTree'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x32a): undefined reference to `preen'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x34d): undefined reference to `checklost'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x371): undefined reference to `ask'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x390): undefined reference to `writefat'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x3f3): undefined reference to `pwarn'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x401): undefined reference to `pwarn'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x420): undefined reference to `ask'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x43f): undefined reference to `pwarn'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x454): undefined reference to `pwarn'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x46f): undefined reference to `writefat'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x48a): undefined reference to `finishDosDirSection'
> /usr/src/svn/sbin/fsck_msdosfs/check.c:(.text+0x4b6): undefined reference to `pwarn'
> cc: error: linker command failed with exit code 1 (use -v to see invocation)
> *** Error code 1
> 
> Stop.
> make: stopped in /usr/src/svn/sbin/fsck_msdosfs
> 
> Sometimes FreeBSD needs to bite the bullet and conform to something that’s been widely accepted for the last 15 years, even if there will be teeth gnashing.

I've mulled this over for a while and the best path forward is "make check". I'll go fix all non-conforming areas of the tree and send out a note to arch as well.
Thanks!
-NGie