Julius Werner 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:
(6 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)))
Yes, as Julius pointed this is not the copy from top Makefile - however indeed I was using this as a […]
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...
(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.)
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@86 PS2, Line 86: $(eval $(call evaluate_subdirs))
No - I mean, at least functionality isn't altered by simply invoking $call. […]
Yeah, I know the top Makefile isn't perfect in some cases, a lot of stuff in there is pretty ancient and has grown warts over time. Let's just make sure things that get newly added make sense for now. (If you want, of course, you're welcome to submit cleanups for the top Makefile as well...)
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@88 PS2, Line 88: .SECONDEXPANSION:
This needs to be declared _before_ the first target which uses this feature. […]
Yeah, let's just move it up in the top Makefile if that's an issue.
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)))
You got me there - this is something tricky, which I must admit I'm not fully understand. […]
Oh... uhh... yes, I think that may be because you are using the variable inside a recipe. IIRC variables inside recipes only get resolved when that recipe runs (not when it is defined).
So I think you need to have a separate variable per target to make this work (e.g. WRAP_LD_FLAGS-$(1)).
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@98 PS2, Line 98: $(HOSTCC) $(HOSTCFLAGS) $(TEST_CFLAGS) $($(1)-cflags) -MMD \
I've added a "-Ibuild/tests" to TEST_CFLAGS earlier, with a comment that it is for config.h. […]
No, you need an -include, not just -I. That's what the main Makefile does too, and coreboot source files are written with the assumption that they don't need to include <config.h> explicitly. (Actually, the same is true for <rules.h> and <commonlib/bsd/compiler.h> so I think you'll need those too... see CPPFLAGS_common in the toplevel Makefile.inc.)
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@102 PS2, Line 102: $(HOSTCFLAGS) $(TEST_CFLAGS)
Yes, this doesn't make much sense, I will separate linker and compiler flags anyway by putting do di […]
Sounds reasonable.