Date: Tue, 29 Oct 2013 17:46:19 -0400 From: Julio Merino <julio@meroh.net> To: "Simon J. Gerraty" <sjg@juniper.net> Cc: freebsd-testing@freebsd.org, Rui Paulo <rpaulo@fnop.net> Subject: Re: Plugging ATF tests into the build and other cleanups Message-ID: <CADyfeQXm%2BDc1sxcJwgq3J=dnL0SUT1dSvxagXfUo6oUSTsro8g@mail.gmail.com> In-Reply-To: <20131029203658.75BF35807E@chaos.jnpr.net> References: <CADyfeQU7Y8APwTMDo9aTR2NUi2EBq0ytQ3QcF7Ct3xC7_BatBQ@mail.gmail.com> <20131029203658.75BF35807E@chaos.jnpr.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Oct 29, 2013 at 4:36 PM, Simon J. Gerraty <sjg@juniper.net> wrote: > > On Sun, 27 Oct 2013 18:12:12 -0400, Julio Merino writes: >>The one concern I have here is having to keep track of all tests in >>tools/build/mk/OptionalObsoleteFiles.inc so that setting >>WITHOUT_TESTS=no cleans up /usr/tests. This will be a pain to maintain > > Yes. > >>and a sure source of inconsistencies. If we could special-case this to >>make it more automatic, do you have any suggestions? > > Iff ATF/Kyua were the only thing populating /usr/tests/ you could just > rm -rf that dir, but that's not how delete-old-* works else > > KYUAFILES!= cd $DESTDIR/ && find usr/tests -name Kyuafile > OLD_DIRS+= ${KYUAFILES:H} > > would be all you'd need. > You can't even do OLD_FILES+= ${KYUAFILES:H}/* I'm confused... Is that "can't" supposed to be "can"? Or in other words, do you mean that this would or would not work if we assumed that ATF/Kyua are the only thing populating /usr/tests ? That's part of the rationale for the next patch: to make /usr/tests/ conditional on a single knob (WITH_TESTS) so that such assumptions can be made: either you install the full test suite or you don't. I don't think it makes sense to support in-between cases... or at least not until a real need for it appears). > It could be that a new delete-old- target might be useful, Would be nice if it weren't necessary though. I think delete-old-files should deal with this case just as it does for any other WITH_* knob so that there are no extra steps to the rebuild procedure. >>As usual: http://portal.meroh.net/~jmmv/freebsd-testing/ > > remove-without-atf.diff > > looks ok - assuming folk are ok with s,ATF,TESTS, > > freebsd-testing/recurse-subdir.diff > > I'm not so crazy about. > What's wrong with requiring folk to set TESTS_SUBDIR[S] ? Because SUBDIR and TESTS_SUBDIRS serve different purposes. With SUBDIR you are telling make (and only make) that it has to recurse into those directories. With TESTS_SUBDIRS you are telling both make and kyua to recurse into the subdirectories. This is useful to build helper binaries/libraries/etc that live in subdirectories without tests (which is necessary when bsd.*.mk don't play well with the test stuff); however, this use-case is not needed yet so we can ignore it for now. The real reason I tossed in this patch here is the following. In the current patchset, this is useful in, for example, src/lib/atf/Makefile (see last patch) : this file needs to generate a Kyuafile that registers its 3 subdirectories (libatf-c, libatf-c++ and test-programs) so that the recursion can be performed at runtime. There is no need to list those as TESTS_SUBDIRS because they are already in SUBDIR. There are some alternatives to this behavior: 1. Move the generation of the Kyuafile to src/lib/atf/tests/Makefile. But if you do that, you cannot get the automatic generation to work because that tests/ directory does not have any children (and thus the automatic generation code has no idea about what to do). 2. Manually set TESTS_SUBDIR in src/lib/atf/Makefile to SUBDIR... which would be suboptimal because this is something that could be done automatically (this patch). 3. Make src/lib/atf/tests/Makefile install a generic Kyuafile that discovers its directories, just like /usr/tests/Kyuafile and /usr/tests/lib/Kyuafile do. This is a reasonable optional and would follow your comments below of keeping stuff in tests/ > freebsd-testing/move-kyuafiles.diff > > I'm also not crazy about this one (the change to lib/Makefile) > What is the expected use? To populate /usr/tests/lib accordingly. You need: /usr/tests/Kyuafile - auto-recurse into directories /usr/tests/lib/Kyuafile - auto-recurse into directories /usr/tests/lib/libc/Kyuafile - list of tests to run and possibly other subdirectories and that /usr/tests/lib/Kyuafile has to come from somewhere. I think that doing this from src/tests/lib/, as I did it originally, is confusing and it seems to me that doing it from somewhere within src/lib/ is clearer. The rationale would be that everything required to create /usr/tests/lib/ would happen from a single place: src/lib/ . (More below.) > and why can't everything that is needed be > done within the context of the individual lib/*/tests/ dir? > That's how we are doing it. We could do this from src/lib/tests/ as easily; didn't think about it. Gave this a try and updated the patches accordingly; check them out. Still open is the question if we should do the same for src/lib/atf/Makefile et. al. I suspect the answer is yes, but then there is the issue mentioned above of the Kyuafile auto-generation. (Haven't done it yet as I don't have the time now and would appreciate your input.) > build-atf-tests.diff > > FWIW I prefer ${.CURDIR:H:H} etc rather than ${.CURDIR}/../.. > makes for much neater paths (also bit quicker on nfs) Done; didn't think about that. > It would be nice to be able to rely on SRCTOP being correctly set > (easy with bmake, have to think about fmake...) > > ATF_SRC = ${SRCTOP}/contrib/atf > > could then be set on one place > but otherwise ok. Indeed, having a SRCTOP would have made things easier... but I didn't find that available. Note: I have applied some of your suggestions to the existing patches but some things still remain open so don't bother doing an in-depth review again because they are broken. However, I gotta go now and I'd prefer to send this out for further comments while I have your attention ;) -- Julio Merino / @jmmv
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CADyfeQXm%2BDc1sxcJwgq3J=dnL0SUT1dSvxagXfUo6oUSTsro8g>