| Message ID | 20231222135836.992841-6-bpoirier@nvidia.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | selftests: Add TEST_INCLUDES directive and adjust tests to use it | expand |
On Fri, Dec 22, 2023 at 08:58:31AM -0500, Benjamin Poirier wrote: > diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst > index ab376b316c36..8b79843ca514 100644 > --- a/Documentation/dev-tools/kselftest.rst > +++ b/Documentation/dev-tools/kselftest.rst > @@ -255,9 +255,15 @@ Contributing new tests (details) > > TEST_PROGS_EXTENDED, TEST_GEN_PROGS_EXTENDED mean it is the > executable which is not tested by default. > + > TEST_FILES, TEST_GEN_FILES mean it is the file which is used by > test. > > + TEST_INCLUDES lists files which are not in the current directory or one of > + its descendants but which should be included when exporting or installing > + the tests. The files are listed with a path relative to > + tools/testing/selftests/. > + > * First use the headers inside the kernel source and/or git repo, and then the > system headers. Headers for the kernel release as opposed to headers > installed by the distro on the system should be the primary focus to be able I've never had to touch this infrastructure, but the fact that TEST_INCLUDES is relative to tools/testing/selftests/ when all other TEST_* variables are relative to $PWD seems ... inconsistent?
On Wed, Dec 27, 2023 at 09:43:56PM +0200, Vladimir Oltean wrote: > On Fri, Dec 22, 2023 at 08:58:31AM -0500, Benjamin Poirier wrote: > > diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst > > index ab376b316c36..8b79843ca514 100644 > > --- a/Documentation/dev-tools/kselftest.rst > > +++ b/Documentation/dev-tools/kselftest.rst > > @@ -255,9 +255,15 @@ Contributing new tests (details) > > > > TEST_PROGS_EXTENDED, TEST_GEN_PROGS_EXTENDED mean it is the > > executable which is not tested by default. > > + > > TEST_FILES, TEST_GEN_FILES mean it is the file which is used by > > test. > > > > + TEST_INCLUDES lists files which are not in the current directory or one of > > + its descendants but which should be included when exporting or installing > > + the tests. The files are listed with a path relative to > > + tools/testing/selftests/. > > + > > * First use the headers inside the kernel source and/or git repo, and then the > > system headers. Headers for the kernel release as opposed to headers > > installed by the distro on the system should be the primary focus to be able > > I've never had to touch this infrastructure, but the fact that TEST_INCLUDES > is relative to tools/testing/selftests/ when all other TEST_* variables > are relative to $PWD seems ... inconsistent? To solve the inconsistency, can it be used like this everywhere? TEST_INCLUDES := \ $(SRC_PATH)/net/lib.sh (I haven't tried this)
On 2023-12-27 21:47 +0200, Vladimir Oltean wrote: > On Wed, Dec 27, 2023 at 09:43:56PM +0200, Vladimir Oltean wrote: > > On Fri, Dec 22, 2023 at 08:58:31AM -0500, Benjamin Poirier wrote: > > > diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst > > > index ab376b316c36..8b79843ca514 100644 > > > --- a/Documentation/dev-tools/kselftest.rst > > > +++ b/Documentation/dev-tools/kselftest.rst > > > @@ -255,9 +255,15 @@ Contributing new tests (details) > > > > > > TEST_PROGS_EXTENDED, TEST_GEN_PROGS_EXTENDED mean it is the > > > executable which is not tested by default. > > > + > > > TEST_FILES, TEST_GEN_FILES mean it is the file which is used by > > > test. > > > > > > + TEST_INCLUDES lists files which are not in the current directory or one of > > > + its descendants but which should be included when exporting or installing > > > + the tests. The files are listed with a path relative to > > > + tools/testing/selftests/. > > > + > > > * First use the headers inside the kernel source and/or git repo, and then the > > > system headers. Headers for the kernel release as opposed to headers > > > installed by the distro on the system should be the primary focus to be able > > > > I've never had to touch this infrastructure, but the fact that TEST_INCLUDES > > is relative to tools/testing/selftests/ when all other TEST_* variables > > are relative to $PWD seems ... inconsistent? I agree with your point about consistency. I have changed TEST_INCLUDES to take paths relative to PWD. The implementation is more complicated since the paths have to be converted to the old values anyways for `rsync -R` but it works. I pasted the overall diff below and it'll be part of the next version of the series. > > To solve the inconsistency, can it be used like this everywhere? > > TEST_INCLUDES := \ > $(SRC_PATH)/net/lib.sh After the changes, it's possible to list files using SRC_PATH but I didn't do it. Since the point is to make TEST_INCLUDES be more like TEST_PROGS, TEST_FILES, ..., I used relative paths. For example in net/forwarding/Makefile: TEST_INCLUDES := \ ../lib.sh diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst index 8b79843ca514..470cc7913647 100644 --- a/Documentation/dev-tools/kselftest.rst +++ b/Documentation/dev-tools/kselftest.rst @@ -259,10 +259,14 @@ Contributing new tests (details) TEST_FILES, TEST_GEN_FILES mean it is the file which is used by test. - TEST_INCLUDES lists files which are not in the current directory or one of - its descendants but which should be included when exporting or installing - the tests. The files are listed with a path relative to - tools/testing/selftests/. + TEST_INCLUDES is similar to TEST_FILES, it lists files which should be + included when exporting or installing the tests, with the following + differences: + * symlinks to files in other directories are preserved + * the part of paths below tools/testing/selftests/ is preserved when copying + the files to the output directory + TEST_INCLUDES is meant to list dependencies located in other directories of + the selftests hierarchy. * First use the headers inside the kernel source and/or git repo, and then the system headers. Headers for the kernel release as opposed to headers diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 3f494a31b479..c289505245f5 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -188,7 +188,7 @@ run_tests: all @for TARGET in $(TARGETS); do \ BUILD_TARGET=$$BUILD/$$TARGET; \ $(MAKE) OUTPUT=$$BUILD_TARGET -C $$TARGET run_tests \ - SRC_PATH=$(shell pwd) \ + SRC_PATH=$(shell readlink -e $$(pwd)) \ OBJ_PATH=$(BUILD) \ O=$(abs_objtree); \ done; @@ -242,7 +242,7 @@ ifdef INSTALL_PATH BUILD_TARGET=$$BUILD/$$TARGET; \ $(MAKE) install OUTPUT=$$BUILD_TARGET -C $$TARGET \ INSTALL_PATH=$(INSTALL_PATH)/$$TARGET \ - SRC_PATH=$(shell pwd) \ + SRC_PATH=$(shell readlink -e $$(pwd)) \ OBJ_PATH=$(INSTALL_PATH) \ O=$(abs_objtree) \ $(if $(FORCE_TARGETS),|| exit); \ diff --git a/tools/testing/selftests/drivers/net/bonding/Makefile b/tools/testing/selftests/drivers/net/bonding/Makefile index a61fe339b9be..03a089165d3f 100644 --- a/tools/testing/selftests/drivers/net/bonding/Makefile +++ b/tools/testing/selftests/drivers/net/bonding/Makefile @@ -18,7 +18,7 @@ TEST_FILES := \ bond_topo_3d1c.sh TEST_INCLUDES := \ - net/forwarding/lib.sh \ - net/lib.sh + ../../../net/forwarding/lib.sh \ + ../../../net/lib.sh include ../../../lib.mk diff --git a/tools/testing/selftests/drivers/net/dsa/Makefile b/tools/testing/selftests/drivers/net/dsa/Makefile index 8259eac80c3b..cd6817fe5be6 100644 --- a/tools/testing/selftests/drivers/net/dsa/Makefile +++ b/tools/testing/selftests/drivers/net/dsa/Makefile @@ -16,17 +16,17 @@ TEST_FILES := \ forwarding.config TEST_INCLUDES := \ - net/forwarding/bridge_locked_port.sh \ - net/forwarding/bridge_mdb.sh \ - net/forwarding/bridge_mld.sh \ - net/forwarding/bridge_vlan_aware.sh \ - net/forwarding/bridge_vlan_mcast.sh \ - net/forwarding/bridge_vlan_unaware.sh \ - net/forwarding/lib.sh \ - net/forwarding/local_termination.sh \ - net/forwarding/no_forwarding.sh \ - net/forwarding/tc_actions.sh \ - net/forwarding/tc_common.sh \ - net/lib.sh + ../../../net/forwarding/bridge_locked_port.sh \ + ../../../net/forwarding/bridge_mdb.sh \ + ../../../net/forwarding/bridge_mld.sh \ + ../../../net/forwarding/bridge_vlan_aware.sh \ + ../../../net/forwarding/bridge_vlan_mcast.sh \ + ../../../net/forwarding/bridge_vlan_unaware.sh \ + ../../../net/forwarding/lib.sh \ + ../../../net/forwarding/local_termination.sh \ + ../../../net/forwarding/no_forwarding.sh \ + ../../../net/forwarding/tc_actions.sh \ + ../../../net/forwarding/tc_common.sh \ + ../../../net/lib.sh include ../../../lib.mk diff --git a/tools/testing/selftests/drivers/net/team/Makefile b/tools/testing/selftests/drivers/net/team/Makefile index 8a9846b5a209..2d5a76d99181 100644 --- a/tools/testing/selftests/drivers/net/team/Makefile +++ b/tools/testing/selftests/drivers/net/team/Makefile @@ -4,8 +4,8 @@ TEST_PROGS := dev_addr_lists.sh TEST_INCLUDES := \ - drivers/net/bonding/lag_lib.sh \ - net/forwarding/lib.sh \ - net/lib.sh + ../bonding/lag_lib.sh \ + ../../../net/forwarding/lib.sh \ + ../../../net/lib.sh include ../../../lib.mk diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk index 2b6c2be4f356..087fee22dd53 100644 --- a/tools/testing/selftests/lib.mk +++ b/tools/testing/selftests/lib.mk @@ -69,14 +69,29 @@ define RUN_TESTS run_many $(1) endef +define INSTALL_INCLUDES + $(if $(TEST_INCLUDES), \ + relative_files=""; \ + for entry in $(TEST_INCLUDES); do \ + entry_dir=$$(readlink -e "$$(dirname "$$entry")"); \ + entry_name=$$(basename "$$entry"); \ + relative_dir=$${entry_dir#"$$SRC_PATH"/}; \ + if [ "$$relative_dir" = "$$entry_dir" ]; then \ + echo "Error: TEST_INCLUDES entry \"$$entry\" not located inside selftests directory ($$SRC_PATH)" >&2; \ + exit 1; \ + fi; \ + relative_files="$$relative_files $$relative_dir/$$entry_name"; \ + done; \ + cd $(SRC_PATH) && rsync -aR $$relative_files $(OBJ_PATH)/ \ + ) +endef + run_tests: all ifdef building_out_of_srctree @if [ "X$(TEST_PROGS)$(TEST_PROGS_EXTENDED)$(TEST_FILES)" != "X" ]; then \ rsync -aq --copy-unsafe-links $(TEST_PROGS) $(TEST_PROGS_EXTENDED) $(TEST_FILES) $(OUTPUT); \ fi - $(if $(TEST_INCLUDES), \ - cd $(SRC_PATH) && rsync -aR $(TEST_INCLUDES) $(OBJ_PATH)/ \ - ) + @$(INSTALL_INCLUDES) @if [ "X$(TEST_PROGS)" != "X" ]; then \ $(call RUN_TESTS, $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS) \ $(addprefix $(OUTPUT)/,$(TEST_PROGS))) ; \ @@ -106,9 +121,7 @@ endef install: all ifdef INSTALL_PATH $(INSTALL_RULE) - $(if $(TEST_INCLUDES), \ - cd $(SRC_PATH) && rsync -aR $(TEST_INCLUDES) $(OBJ_PATH)/ \ - ) + $(INSTALL_INCLUDES) else $(error Error: set INSTALL_PATH to use install) endif diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile index 5b55c0467eed..1fba2717738d 100644 --- a/tools/testing/selftests/net/forwarding/Makefile +++ b/tools/testing/selftests/net/forwarding/Makefile @@ -130,6 +130,6 @@ TEST_PROGS_EXTENDED := devlink_lib.sh \ tc_common.sh TEST_INCLUDES := \ - net/lib.sh + ../lib.sh include ../../lib.mk
On Thu, Dec 28, 2023 at 02:28:06PM -0500, Benjamin Poirier wrote: > I agree with your point about consistency. I have changed TEST_INCLUDES > to take paths relative to PWD. The implementation is more complicated > since the paths have to be converted to the old values anyways for > `rsync -R` but it works. I pasted the overall diff below and it'll be > part of the next version of the series. > > > > > To solve the inconsistency, can it be used like this everywhere? > > > > TEST_INCLUDES := \ > > $(SRC_PATH)/net/lib.sh > > After the changes, it's possible to list files using SRC_PATH but I > didn't do it. Since the point is to make TEST_INCLUDES be more like > TEST_PROGS, TEST_FILES, ..., I used relative paths. > For example in net/forwarding/Makefile: > TEST_INCLUDES := \ > ../lib.sh I thought you wanted to avoid the cascade of ../../../ which is confusing, so I recommended $(SRC_PATH) as a way to avoid that by using absolute paths instead - which makes it easier to track down the include file being sourced. My inconsistency issue was with TEST_INCLUDES being relative to a different directory than the rest. With that being addressed, I don't think that using absolute paths for some files would be inconsistent in a similar way. But I don't mind too much either way. Feel free to keep the ../../../ scheme.
diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst index ab376b316c36..8b79843ca514 100644 --- a/Documentation/dev-tools/kselftest.rst +++ b/Documentation/dev-tools/kselftest.rst @@ -255,9 +255,15 @@ Contributing new tests (details) TEST_PROGS_EXTENDED, TEST_GEN_PROGS_EXTENDED mean it is the executable which is not tested by default. + TEST_FILES, TEST_GEN_FILES mean it is the file which is used by test. + TEST_INCLUDES lists files which are not in the current directory or one of + its descendants but which should be included when exporting or installing + the tests. The files are listed with a path relative to + tools/testing/selftests/. + * First use the headers inside the kernel source and/or git repo, and then the system headers. Headers for the kernel release as opposed to headers installed by the distro on the system should be the primary focus to be able diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 81f094e7f0f7..3f494a31b479 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -188,6 +188,8 @@ run_tests: all @for TARGET in $(TARGETS); do \ BUILD_TARGET=$$BUILD/$$TARGET; \ $(MAKE) OUTPUT=$$BUILD_TARGET -C $$TARGET run_tests \ + SRC_PATH=$(shell pwd) \ + OBJ_PATH=$(BUILD) \ O=$(abs_objtree); \ done; @@ -238,7 +240,10 @@ ifdef INSTALL_PATH @ret=1; \ for TARGET in $(TARGETS); do \ BUILD_TARGET=$$BUILD/$$TARGET; \ - $(MAKE) OUTPUT=$$BUILD_TARGET -C $$TARGET INSTALL_PATH=$(INSTALL_PATH)/$$TARGET install \ + $(MAKE) install OUTPUT=$$BUILD_TARGET -C $$TARGET \ + INSTALL_PATH=$(INSTALL_PATH)/$$TARGET \ + SRC_PATH=$(shell pwd) \ + OBJ_PATH=$(INSTALL_PATH) \ O=$(abs_objtree) \ $(if $(FORCE_TARGETS),|| exit); \ ret=$$((ret * $$?)); \ diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk index aa646e0661f3..2b6c2be4f356 100644 --- a/tools/testing/selftests/lib.mk +++ b/tools/testing/selftests/lib.mk @@ -74,6 +74,9 @@ ifdef building_out_of_srctree @if [ "X$(TEST_PROGS)$(TEST_PROGS_EXTENDED)$(TEST_FILES)" != "X" ]; then \ rsync -aq --copy-unsafe-links $(TEST_PROGS) $(TEST_PROGS_EXTENDED) $(TEST_FILES) $(OUTPUT); \ fi + $(if $(TEST_INCLUDES), \ + cd $(SRC_PATH) && rsync -aR $(TEST_INCLUDES) $(OBJ_PATH)/ \ + ) @if [ "X$(TEST_PROGS)" != "X" ]; then \ $(call RUN_TESTS, $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS) \ $(addprefix $(OUTPUT)/,$(TEST_PROGS))) ; \ @@ -103,6 +106,9 @@ endef install: all ifdef INSTALL_PATH $(INSTALL_RULE) + $(if $(TEST_INCLUDES), \ + cd $(SRC_PATH) && rsync -aR $(TEST_INCLUDES) $(OBJ_PATH)/ \ + ) else $(error Error: set INSTALL_PATH to use install) endif
Some tests written in bash source other files in a parent directory. For example, drivers/net/bonding/dev_addr_lists.sh sources net/forwarding/lib.sh. If a subset of tests is exported and run outside the source tree (for example by using `make -C tools/testing/selftests gen_tar TARGETS="drivers/net/bonding"`), these other files must be made available as well. Commit ae108c48b5d2 ("selftests: net: Fix cross-tree inclusion of scripts") addressed this problem by symlinking and copying the sourced files but this only works for direct dependencies. Commit 25ae948b4478 ("selftests/net: add lib.sh") changed net/forwarding/lib.sh to source net/lib.sh. As a result, that latter file must be included as well when the former is exported. This is not handled currently. Therefore, add a mechanism to list dependent files in a new Makefile variable and export them. This allows sourcing those files using the same expression whether tests are run in-tree or exported. Dependencies are not resolved recursively so transitive dependencies must be listed in TEST_INCLUDES. For example, net/forwarding/lib.sh sources net/lib.sh; so a script that sources net/forwarding/lib.sh from a parent directory must list: TEST_INCLUDES := \ net/forwarding/lib.sh \ net/lib.sh Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com> --- Documentation/dev-tools/kselftest.rst | 6 ++++++ tools/testing/selftests/Makefile | 7 ++++++- tools/testing/selftests/lib.mk | 6 ++++++ 3 files changed, 18 insertions(+), 1 deletion(-)