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 3:
(17 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@4 PS2, Line 4: ## This program is free software; you can redistribute it and/or modify : ## it under the terms of the GNU General Public License as published by : ## the Free Software Foundation; version 2 of the License. : ## : ## This program is distributed in the hope that it will be useful, : ## but WITHOUT ANY WARRANTY; without even the implied warranty of : ## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : ## GNU General Public License for more details.
We're dropping these blocks now, so replace it with […]
Done
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@18 PS2, Line 18: tests
Yes, this is correct. […]
Done
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@27 PS2, Line 27: TEST_KCONFIG_FLAGS:= DOTCONFIG=$(TEST_DOTCONFIG) \
Makes sense.
Done
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@35 PS2, Line 35:
tab for consistency
Done
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 ca […]
One more comment which I have regarding your proposal above, namely about $(test)-runner. I think we may ask user to keep explicitly listing all sources he/she wants to include into the final binary. In other words - I don't see that much benefit, while things are less straightforward. After all it is only one extra line.
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@86 PS2, Line 86: $(eval $(call evaluate_subdirs))
Yeah, I know the top Makefile isn't perfect in some cases, a lot of stuff in there is pretty ancient […]
Let's start with removing this $(eval). I will think about cleanup in Makefile, but this is kind of a risky change. Let's see how it goes.
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@88 PS2, Line 88: .SECONDEXPANSION:
Yeah, let's just move it up in the top Makefile if that's an issue.
Done
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)))
Yes, this was exactly an issue with time of expand and time of use. […]
Done
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@96 PS2, Line 96: $$$$(filter tests/$$$$*.c, $($(1)-srcs)) $(TEST_KCONFIG_AUTOHEADER)
Indeed. I was so proud of creating such a complicated syntax.. […]
Done
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@98 PS2, Line 98: $(HOSTCC) $(HOSTCFLAGS) $(TEST_CFLAGS) $($(1)-cflags) -MMD \
I already have -include for kconfig.h and commonlib/bsd/compiler.h. Thought that if kconfig. […]
Done
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@99 PS2, Line 99: $$(@)
nit: Let's try to be consistent about using $@ or $(@) (though I know other parts of coreboot aren't […]
Done
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@102 PS2, Line 102: $(HOSTCFLAGS) $(TEST_CFLAGS)
Sounds reasonable.
Done
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@103 PS2, Line 103: $(TEST_LDFLAGS) $(WRAP_LD_FLAGS) -o $$@
Please always add an extra indent for wrapped lines.
Done
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@107 PS2, Line 107: # tests/ dir
Frankly, at this point in time, I'm no longer sure what was my reasoning behind removing tests/ from […]
Done
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@112 PS2, Line 112: %-test.o
So is this the main reason you want to suffix all tests with '-test'? […]
Done
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@119 PS2, Line 119: $(patsubst %.c,%.o, $(subst tests/,,$($(test)-srcs))))))
Good point.
Done
https://review.coreboot.org/c/coreboot/+/39894/2/tests/Makefile.inc@128 PS2, Line 128: cp $(DEFAULT_CONFIG) $(TEST_DOTCONFIG)
Indeed, there is an issue with wrong cp argument - will fix in v3. […]
Done