17 comments:
## 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
Patch Set #2, Line 18: tests
Yes, this is correct. […]
Done
Patch Set #2, Line 27: TEST_KCONFIG_FLAGS:= DOTCONFIG=$(TEST_DOTCONFIG) \
Makes sense.
Done
tab for consistency
Done
# 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.
Patch Set #2, 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.
Patch Set #2, Line 88: .SECONDEXPANSION:
Yeah, let's just move it up in the top Makefile if that's an issue.
Done
Patch Set #2, 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
Patch Set #2, Line 96: $$$$(filter tests/$$$$*.c, $($(1)-srcs)) $(TEST_KCONFIG_AUTOHEADER)
Indeed. I was so proud of creating such a complicated syntax.. […]
Done
Patch Set #2, 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
Patch Set #2, Line 99: $$(@)
nit: Let's try to be consistent about using $@ or $(@) (though I know other parts of coreboot aren't […]
Done
Patch Set #2, Line 102: $(HOSTCFLAGS) $(TEST_CFLAGS)
Sounds reasonable.
Done
Patch Set #2, Line 103: $(TEST_LDFLAGS) $(WRAP_LD_FLAGS) -o $$@
Please always add an extra indent for wrapped lines.
Done
Patch Set #2, Line 107: # tests/ dir
Frankly, at this point in time, I'm no longer sure what was my reasoning behind removing tests/ from […]
Done
Patch Set #2, Line 112: %-test.o
> So is this the main reason you want to suffix all tests with '-test'? […]
Done
Patch Set #2, Line 119: $(patsubst %.c,%.o, $(subst tests/,,$($(test)-srcs))))))
Good point.
Done
Patch Set #2, Line 128: cp $(DEFAULT_CONFIG) $(TEST_DOTCONFIG)
Indeed, there is an issue with wrong cp argument - will fix in v3. […]
Done
To view, visit change 39894. To unsubscribe, or for help writing mail filters, visit settings.