Thanks, I think this looks a *lot* better already! I'm excited about all the things we'll be able to do with this!
13 comments:
# Include Makefile.inc, clean test-specific variables;
# Add directory prefix to all test-specific variables and test name
# itself in order to allow the same test name in different directories
includemakefiles= \
$(foreach value,tests test-subdirs, \
$(eval $(value):=)) \
$(eval -include $(1)) \
$(foreach test, $(tests), \
$(foreach attribute, $(attributes), \
$(eval $(addprefix $(dir $(1)), $(test)-$(attribute)):= \
$($(test)-$(attribute))))) \
$(foreach test, $(tests), \
$(foreach attribute, $(attributes), \
$(eval $(test)-$(attribute):=))) \
$(eval alltests+=$(addprefix $(dir $(1)), $(tests))) \
$(eval subdirs+=$(subst $(CURDIR)/,, \
$(abspath $(addprefix $(dir $(1)),$(test-subdirs)))))
# Traverse through the unit-tests tree and invoke includemakefiles
evaluate_subdirs= \
$(eval cursubdirs:=$(subdirs)) \
$(eval subdirs:=) \
$(foreach dir,$(cursubdirs), \
$(eval $(call includemakefiles,$(dir)/Makefile.inc))) \
$(if $(subdirs),$(eval $(call evaluate_subdirs)))
this is copied from the toplevel Makefile. […]
Well, this is a different includemakefiles specific for tests. However, I think it might be possible to use the default version by making 'tests' a special class. Then you'd basically have this in Makefile.inc:
tests-y += i2c-test # can auto-detect that i2c-test.c is the runner source file, see below
i2c-test-srcs += src/device/i2c.c
i2c-test-mocks += platform_i2c_transfer
...and then you need to define a handler roughly like this:
define tests-handler
alltests += $(1)/$(2)
$(2)-runner := $(if $($(2)-runner)),$($(2)-runner),$(2).c)
$(2)-srcs += $(1)/$($(2)-runner))
$(foreach attribute,$(attributes),$(eval $(1)/$(2)-$(attribute) += $(2)-$(attribute)))
$(foreach attribute,$(attributes),$(eval $(2)-$(attribute):=))
endef
Patch Set #2, Line 86: $(eval $(call evaluate_subdirs))
Does this actually need the eval here?
Patch Set #2, Line 88: .SECONDEXPANSION:
IIRC you only need this once per 'make' invocation, so if this file is always included from toplevel you shouldn't need it here.
Patch Set #2, Line 93: $(eval WRAP_LD_FLAGS:= $(foreach mock,$($(1)-mocks),-Wl,--wrap=$(mock)))
Does this need to use eval?
Patch Set #2, Line 96: $$$$(filter tests/$$$$*.c, $($(1)-srcs)) $(TEST_KCONFIG_AUTOHEADER)
If you didn't cut the 'tests/' out of the name (see below) this could also just be way simpler as '$$$$*.c' instead of all the filtering.
Patch Set #2, Line 98: $(HOSTCC) $(HOSTCFLAGS) $(TEST_CFLAGS) $($(1)-cflags) -MMD \
Shouldn't there be an
-include $(TEST_KCONFIG_AUTOHEADER)
somewhere (either here or in $(TEST_CFLAGS))?
Patch Set #2, Line 99: $$(@)
nit: Let's try to be consistent about using $@ or $(@) (though I know other parts of coreboot aren't either). I think $@ is more common for automatic variables.
Patch Set #2, Line 102: $(HOSTCFLAGS) $(TEST_CFLAGS)
I think usually we don't pass CFLAGS for linking (or is there a good reason to here?).
Patch Set #2, Line 103: $(TEST_LDFLAGS) $(WRAP_LD_FLAGS) -o $$@
Please always add an extra indent for wrapped lines.
Patch Set #2, Line 107: # tests/ dir
Somewhat confused why you cut the "tests/" out, since you're not cutting the "src/" in supporting files out either. I feel like keeping them both would be more consistent.
Patch Set #2, Line 112: %-test.o
So is this the main reason you want to suffix all tests with '-test'? I think you could have this much easier by just calling the binary
$(eval $(test)-bin:=build/$(test)/run)
or something. There's no reason why the binary name needs to match the test file if you've sequestered it in its own directory already, after all (but if you want that you could have that too with $(notdir (test)) or whatever).
Patch Set #2, Line 119: $(patsubst %.c,%.o, $(subst tests/,,$($(test)-srcs))))))
Why not just use $($(test)-objs)?
In v2 I've added automatic generation of config for all tests. By default, qemu-x86 .config is used. […]
I'm not really sure I understand the dependency problem you're describing? You just need to make the rule to build the test dependent on the test-specific config.h (which could be stored under build/test/..path../file-test.config.h or something like that), and then you write a recipe to create that config.h file by creating a minimal .config with the desired options and running the equivalent of $(MAKE) oldconfig on it (or just running the 'conf' utility directly if that's easier).
But anyway, if that's too hard I think the poor-man's Kconfig by hacking up a header with #undefs as you describe would probably be good enough as well. I think it would be okay to start with that and see how far it gets us.
To view, visit change 39894. To unsubscribe, or for help writing mail filters, visit settings.