[PATCH] xfstests: fix install target using sudo

[PATCH] xfstests: fix install target using sudo

[Luis R. Rodriguez]

If you install with:

	sudo make install

Depending on the system, you may see /var/lib/xfstests/tests/ is empty.
This is because $(PWD) can expand to be empty on certain systems and so the
wildcard finds nothing.

PWD is only used on one target, the tests/*/ dir install target.

We can fix this by using $(CURDIR) however that does not suffice as we
are also using the $(wildcard) and that needs its own careful expansion.

This issue is observed on both Fedora and OpenSUSE, but not on Debian.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 tests/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/Makefile b/tests/Makefile
index 2611b3b845f5..084135da0487 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -5,7 +5,8 @@
 TOPDIR = ..
 include $(TOPDIR)/include/builddefs
 
-TESTS_SUBDIRS = $(sort $(dir $(wildcard $(PWD)/$(TESTS_DIR)/[a-z]*/)))
+TEST_DIR = $(dir $(CURDIR)/$(TESTS_DIR))
+TESTS_SUBDIRS = $(sort $(dir $(wildcard $(TEST_DIR)/[a-z]*/)))
 
 include $(BUILDRULES)
 
-- 
2.16.3

Re: [PATCH] xfstests: fix install target using sudo

[Dave Chinner]

On Tue, Jun 26, 2018 at 02:08:34PM -0700, Luis R. Rodriguez wrote:
> If you install with:
> 
> 	sudo make install
> 
> Depending on the system, you may see /var/lib/xfstests/tests/ is empty.
> This is because $(PWD) can expand to be empty on certain systems and so the
> wildcard finds nothing.

When and why?

I'm guessing it's because the environment variable PWD is not
exported by the shell that make is being run in?  /bin/sh, /bin/bash
and /bin/dash should always set PWD in the environment, so maybe
you're using a different shell that doesn't set PWD correctly?

> PWD is only used on one target, the tests/*/ dir install target.
> 
> We can fix this by using $(CURDIR) however that does not suffice as we
> are also using the $(wildcard) and that needs its own careful expansion.

$(CURDIR) is documented as an being the current absolute path, so
AFAICT there's nothing to be careful about in terms of expansion.
What are you trying to avoid here?

> This issue is observed on both Fedora and OpenSUSE, but not on Debian.

This really smells more of a shell/environment issue, not a distro
issue.

> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  tests/Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/Makefile b/tests/Makefile
> index 2611b3b845f5..084135da0487 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -5,7 +5,8 @@
>  TOPDIR = ..
>  include $(TOPDIR)/include/builddefs
>  
> -TESTS_SUBDIRS = $(sort $(dir $(wildcard $(PWD)/$(TESTS_DIR)/[a-z]*/)))
> +TEST_DIR = $(dir $(CURDIR)/$(TESTS_DIR))

I think this is wrong - you're creating an invalid path:

curdir/tests_dir is /home/dave/src/xfstests-dev/tests/tests
dir curdir/tests_dir is /home/dave/src/xfstests-dev/tests/

then using $(dir <path>) command to truncate away the invalid
path segment, resulting in the original path $(CURDIR) gave you.

In fact, $(CURDIR) is basically what the current code intends - I
didn't know this existed because it's not obviously searchable (i.e.
not a CWD or PWD variant) so I hacked around it with environment
variables.

So AFAICT, the change the needs to be made here is:

-TESTS_SUBDIRS = $(sort $(dir $(wildcard $(PWD)/$(TESTS_DIR)/[a-z]*/)))
+TESTS_SUBDIRS = $(sort $(dir $(wildcard $(CURDIR)/[a-z]*/)))

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfstests: fix install target using sudo

[Luis R. Rodriguez]

On Wed, Jun 27, 2018 at 05:42:19PM +1000, Dave Chinner wrote:
> On Tue, Jun 26, 2018 at 02:08:34PM -0700, Luis R. Rodriguez wrote:
> 
> In fact, $(CURDIR) is basically what the current code intends - I
> didn't know this existed because it's not obviously searchable (i.e.
> not a CWD or PWD variant) so I hacked around it with environment
> variables.
> 
> So AFAICT, the change the needs to be made here is:
> 
> -TESTS_SUBDIRS = $(sort $(dir $(wildcard $(PWD)/$(TESTS_DIR)/[a-z]*/)))
> +TESTS_SUBDIRS = $(sort $(dir $(wildcard $(CURDIR)/[a-z]*/)))

You are right, I botched my test with it and used CURDIR with TEST_DIR,
removing it as you did works. Will send a v2.

  Luis