From: Garrett C. <yan...@gm...> - 2009-10-30 11:24:20
|
On Fri, Oct 30, 2009 at 4:21 AM, Garrett Cooper <yan...@gm...> wrote: > On Fri, Oct 30, 2009 at 3:56 AM, Jiří Paleček <jpa...@we...> wrote: >> >> Hello, >> >> >> yaneurabeya wrote: >>> >>> 2009/10/25 Jiří Paleček <jpa...@we...>: >>>> On Sun, 18 Oct 2009 20:22:00 +0200, Subrata Modak >>>> <su...@li...> wrote: >>>> >>>>> Jiri, >>>>> >>>>> Any further thoughts on this ? >>>> >>>> Yes, see the attachment. >>>> >>>> I created the directories for the different tests (rt_sigtimedwait, >>>> sigwaitinfo etc.), but to avoid copy-pasting the code, they share the >>>> same >>>> source. Please, tell me if that is OK. >>> >>> I'm a bit confused by let me help analyze the Makefile and provide >>> critiques: >>> >>> The basic template for any Makefile now, is as follows: >>> >>> # LICENSING TORT, e.g. GPLv2 License, etc. >>> >>> # <TOP-SRCDIR DEFINITION> >>> >>> # <PRE-INCLUDE> >>> >>> # <ADDITIONAL MAKE LOGIC GOES HERE> >>> >>> # <POST-INCLUDE> >>> >>> The description of these components is as follows: >>> >>> =========================================================== >>> TOP-SRCDIR DEFINITION >>> =========================================================== >>> top_srcdir ?= <relative path from current Makefile to the top of the >>> source tree> >>> >>> Example 1 (say, from testcases/realtime): >>> >>> top_srcdir ?= ../.. >>> >>> Example 2 (say, from lib): >>> >>> top_srcdir ?= .. >>> >>> The ?= is important, because this value can be redefined at ANY time >>> -- I've been considering making this assignment operator := though, >>> because it should be set just once per Makefile, and isn't being >>> exported anymore (it was a design mistake I made in the beginning that >>> Mike F. corrected me because I wasn't adhering to correct autotools >>> variable conventions). >>> >>> =========================================================== >>> PRE-INCLUDE >>> =========================================================== >>> This is important, and may lead to potential confusion. >>> >>> env_pre.mk is used for most includes outside of the testcases portion >>> of the tree. This has a minimal set of assumptions and dependent >>> targets, because it's designed to work with doc, lib, tools, and >>> utils. This can also be used in `trunk' situations (discussed below) >>> where no C-files need to be compiled, and/or linked in with libltp.a. >>> This also adds $(abs_top_srcdir)/include to the CPPFLAGS so you don't >>> need to add this to 300+ Makefiles (like it was before :)...]. >>> >>> testcases.mk is used for rest of the cases under testcases. It does >>> the following for you: >>> 1. Everything in env_pre.mk. >>> 2. Adds dependencies for libltp.a. >>> 3. Adds -lltp to LDLIBS. >>> 4. Adds dependencies for testcases/kernel/include/syscall_numbers.h >>> 5. Adds an optional hook for the tst_res apps in tools/apicmds so you >>> can run your tests out of the local directory before install [assuming >>> they're coded properly :)...]. >>> >>> All of the additional steps are done for developer convenience, so >>> that running make install from testcases/kernel/syscalls/open doesn't >>> require that I manually go into lib/ and testcases/kernel/include >>> beforehand and run a make all, and have to duplicate a lot of >>> unnecessary logic in the Makefiles. >>> >>> =========================================================== >>> ADDITIONAL MAKE LOGIC GOES HERE >>> =========================================================== >>> >>> Whatever you want to put in here that isn't provided with the defaults >>> in env_post.mk, generic_leaf_target.inc, or generic_trunk_target.inc, >>> so feel free to `use your imagination' as to whatever you may put in >>> here, but the only cases that you should have to deal with are: >>> >>> 1. Multiple C files / objects -> one or more executables. >>> 2. Items without implicit rule mappings, e.g. .c -> .o. >>> >>> =========================================================== >>> POST-INCLUDE: >>> =========================================================== >>> >>> - For all directory traversing Makefiles you include the `trunk >>> target' Makefile: >>> >>> include $(top_srcdir)/include/mk/generic_trunk_target.mk >>> >>> - For all non-directory traversing Makefiles, you include the `leaf >>> target' Makefile. >>> >>> include $(top_srcdir)/include/mk/generic_leaf_target.mk >>> >>> =========================================================== >>> >>> Now, to critique your proposed Makefiles... >>> >>> testcases/kernel/syscalls/rt_sigtimedwait/Makefile: >>> >>> +CFLAGS += -I../../../../include -Wall -O2 -DTEST_RT_SIGTIMEDWAIT >>> +LDLIBS += -L../../../../lib -lltp >>> + >>> +SRCS = $(wildcard *.c) >>> +TARGETS = $(patsubst %.c,%,$(SRCS)) rt_sigtimedwait01 >>> + >>> +all: $(TARGETS) >>> + >>> +rt_sigtimedwait01.o: ../sigwaitinfo/sigwaitinfo01.c >>> + $(CC) -c -o $@ $(CPPFLAGS) $(CFLAGS) $^ >>> + >>> +install: >>> + @set -e; for i in $(TARGETS); do ln -f $$i ../../../bin/$$i ; done >>> + >>> +clean: >>> + rm -f $(TARGETS) >>> >>> Given the above explanation, I would write this like the following >>> (minus the licensing tort, and with improper indented vars -- bloody >>> GMail :(...): >>> >>> ============================================= >>> top_srcdir ?= ../../../.. >>> >>> include $(top_srcdir)/include/mk/testcases.mk >>> >>> # NOTE: Placement doesn't matter, but for readability and consistency >>> it should go here. >>> CPPFLAGS += -DTEST_RT_SIGTIMEDWAIT >>> >>> # NOTE: Placement for this Make target doesn't matter... just so long >>> as it's defined in the Makefile. >>> rt_sigtimedwait01: $(abs_srcdir)/../sigwaitinfo/sigwaitinfo01.c >>> $(COMPILE.o) $(OUTPUT_OPTION) $^ >>> >>> # NOTE: Don't modify MAKE_TARGETS here because it isn't set yet, and >>> the default is to pick up all existing C files in the >>> # current directory and make them into single-source executables, e.g. >>> # sigwaitinfo01.c -> sigwaitinfo01.o -> sigwaitinfo01 ;)... >>> >>> include $(top_srcdir)/include/mk/generic_leaf_target.mk >>> >>> MAKE_TARGETS += rt_sigtimedwait01 >>> ============================================= >>> >>> Similar logic would be applied to your other Makefile, minus the >>> CPPFLAGS assignment. >>> >>> Please note that your binary is no longer linked into >>> $(abs_top_builddir)/testcases/bin -- it's instead install(1)'ed >>> directly into $(DESTDIR)/$(prefix)/testcases/bin . This was done to >>> facilitate proper installation by using the correct mode, and also to >>> avoid broken/hacky symlinking operations. >>> >>> Hope that helps describe things a bit better... let me know if I >>> should add this to a README or something... >>> >>> >> >> Thank you, it is definitely clearer now. I think it should be mentioned in >> the README, including some list of the ltp makefile variables (like >> $(abs_srcdir)). I'll wait with a new patch for the new build system to >> appear in a release, though. >> >> What I still don't get is where does the ZIPFILE variable get its value >> (this was the first place where I sought inspiration): >> >> jirka@debian:/usr/src/ltp-cvs$ fgrep -RI * -e ZIPFILE >> testcases/commands/unzip/Makefile:$(INSTALL_TARGETS): $(ZIPFILE) >> testcases/commands/unzip/Makefile:$(ZIPFILE): $(DIR) >> jirka@debian:/usr/src/ltp-cvs$ > > Ok, I'll add this information as an additional reference to README.mk-devel. > > ZIPFILE is most likely an ad-hoc variable defined in that Makefile > that needs to be added to the new Makefile (it's missing from install > and it's something that I forgot to add to TODO even though I noticed > it once or twice -- I need to note all of the issues clearly this > weekend after I go over the logs manually to ensure that I didn't miss > anything by accident). Assuming that ZIPFILE's generated at build time > (which it should be) you're fine getting away with ZIPFILE being a > `bareword' value relative to the current directory. The only possible > issue is that you'll have to ensure that the destination directory > exists in order for out-of-build-tree support to continue to function > properly (I don't expect people to get this correct all of the time, > but I'd like to keep breakage down to a minimum because it impacts > folks who cross-compile for multiple targets using the same src > directory like my group). You can do this similar to the following: > > foo: > mkdir -p $@ > foo/bar: foo > # fake tab; do something to create foo/bar > > If foo/bar isn't a variable, then it can be expressed as: > > foo: > mkdir -p $@ Sorry. I meant: .SECONDEXPANSION: foo/bar: $$(@D) # fake tab; do something to create foo/bar Most of the time this isn't the case though because specifying INSTALL_TARGETS, uses generate_install_rule, which uses a variable for the target value, and thus mucks up the `.SECONDEXPANSION:' call. > Specifying the directory as a dependency of the target has the two > following benefits: > 1. Ensures that the dependency remains constant and the requirement is > ensured from make (this is key). > 2. Speeds up the build because adding comparable logic to each build > and install rule (test -d foo || mkdir -p foo) I've discovered is > considerably slower than being dependent on the directory and having > make fill in the blanks as necessary. The difference was on the order > of at least 1~2 mins from what I saw on the Cisco build servers I use > and my personal machine. > > The dependency targets (e.g. foo) should be automatically available > via some default logic called up in .../include/mk/env_post.mk using > generate_install_rule_dir_dep .../include/mk/functions.mk, but I've > run into some corner cases where this isn't correct (some of the > INSTALL_TARGETS specified in Makefile.inc, IIRC), so I'll need to fix > something minor after I stare at the problem from a different angle > (it's most likely something really simple that I overlooked by > accident). > > I've made some bad assumptions too that I've had to fix after testing > in out-of-build-tree, so testing this new functionality is more > complicated than traditional in-build-tree support, but if I can get a > build-bot up and going for LTP which uses out-of-build-tree, this > would be easy to automate longterm. I don't forsee completing this > before the end of November though. > > So -- in summary: relative paths from current directory are ok -- just > be sure that the directory is a dependency of the target and is > created before the target is created or functional issues will occur > (best case consistent build errors -- worst case race conditions in > the source tree :[...). > > Cheers, > -Garrett > |