Name of user not set #1002873 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39894 )
Change subject: tests: Add build subsystem for unit testing coreboot ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc File tests/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@52 PS2, Line 52: # 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)))
I don't really see why you would need to change the top Makefile, can you explain? Or I guess I can wait for your next update and see...
First of all, I need to have these macros defined at the time of including tests/Makefile.inc: evaluate_subdir, includemakefiles and add-special-class.
Moreover, I want to be isolated from .DOTCONFIG presence (one used for building source) - I would like to run unit-tests even if the .config is not configured (I suppose it will be rather rare, but still someone may start TDD and build final coreboot image only after all tests passes). This brings another implication - I cannot include toolchain.inc when there is no .DOTCONFIG.
Last, there is no need to traverse through Makefile.incs under src/ when I'm building unit-tests.
(That libpayload is separate is somewhat intentional since it's using a separate license and should be possible to be built standalone. That doesn't apply to these tests.)
OK, I see.
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@93 PS2, Line 93: $(eval WRAP_LD_FLAGS:= $(foreach mock,$($(1)-mocks),-Wl,--wrap=$(mock)))
Oh... uhh... yes, I think that may be because you are using the variable inside a recipe. […]
Yes, this was exactly an issue with time of expand and time of use. However why $(eval) helped here? I thought output of $(eval) produces make construct which is parsed the same way (in the same Make phase) as the usual syntax.
Putting this aside - I come up with another solution, which looks better for me - let's change TEST_LDFLAGS per target. This way, I don't need any extra variables - I think it is cleaner. Below is the snippet (with extra CFLAGS for stage-related define).
$($(1)-objs): TEST_CFLAGS+= $$(if $$($(1)-stage), \ -D__$$(shell echo $$($(1)-stage) | tr '[:lower:]' '[:upper:]')__, \ -D__RAMSTAGE__) $($(1)-objs): build/$(1)/%.o: $$$$*.c $(TEST_KCONFIG_AUTOHEADER) mkdir -p $$(dir $$@) $(HOSTCC) $(HOSTCFLAGS) $$(TEST_CFLAGS) $($(1)-cflags) -MMD \ -MT $$@ -c $$< -o $$@
$($(1)-bin): TEST_LDFLAGS+= $$(foreach mock,$$($(1)-mocks),-Wl,--wrap=$$(mock)) $($(1)-bin): $($(1)-objs) $(HOSTCC) $$^ $($(1)-cflags) $$(TEST_LDFLAGS) -o $$
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@98 PS2, Line 98: $(HOSTCC) $(HOSTCFLAGS) $(TEST_CFLAGS) $($(1)-cflags) -MMD \
No, you need an -include, not just -I. […]
I already have -include for kconfig.h and commonlib/bsd/compiler.h. Thought that if kconfig.h is globally visible and it includes config.h I should be ok. But let me add these, so that to be compliant with top Makefile.