Date: Thu, 28 Dec 2023 14:28:06 -0500
From: Benjamin Poirier <bpoirier@...dia.com>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: netdev@...r.kernel.org, Shuah Khan <shuah@...nel.org>,
	Petr Machata <petrm@...dia.com>, Hangbin Liu <liuhangbin@...il.com>
Subject: Re: [RFC PATCH net-next 05/10] selftests: Introduce Makefile
 variable to list shared bash scripts

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

Powered by blists - more mailing lists