[PATCH] test-container: Add $complocaledir and mkdirp.

[PATCH] test-container: Add $complocaledir and mkdirp.

[Carlos O'Donell]

Despite me writing "necessary" in the commit message, it's obviously
not entirely required since you can just use:

+  xmkdirp (support_complocaledir_prefix, 0777);

in the test. The above does exactly the same as running the following from
the script:

mkdirp 0755 $complocaledir/

However, the patch below enables the test developer to choose how they
want to create the directory, via a script (patch enalbes this), or
inside the test.

One might argue that locales should just be installed by default
in the test chroot, but I would argue against it. I would say the framework
should do:

* Container tests by default should have no locales (as we have it today,
  fastest chroot creation).
* Container tests should be able to specify they want a specific locale
  installed or all locales (enhancement request, not supported today).

OK for master?

8< --- 8< --- 8<
From 925c47b47d597663606486402c38e2e729ed663c Mon Sep 17 00:00:00 2001
From: Carlos O'Donell <carlos@redhat.com>
Date: Thu, 23 Jan 2020 09:45:00 -0500
Subject: [PATCH] test-container: Add $complocaledir and mkdirp.

In order to test localedef in the test-container framework it
is necessary to add support for one new variable and one new
command. For localedef to install a compiled locale in the default
location the directory needs to be created
e.g. mkdir -p $complocaledir/dir.  This command requires both the
new command 'mkdirp' e.g. mkdir -p, and the new variable
$complocaledir. The only way to avoid this would be to install
locales into the pristine container root, but that would slow down
container testing (currently only has builtin C/POSIX available).

These new features will be used by the new tst-localedef-hardlinks
test.
---
 support/test-container.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/support/test-container.c b/support/test-container.c
index dff2ff379e..deb2cc3ff5 100644
--- a/support/test-container.c
+++ b/support/test-container.c
@@ -97,9 +97,13 @@ int verbose = 0;
 	 rm FILE
 	 cwd PATH
 	 exec FILE
-	 FILE must start with $B/, $S/, $I/, $L/, or /
-	  (expands to build dir, source dir, install dir, library dir
-	   (in container), or container's root)
+	 mkdirp MODE DIR
+
+	 FILE must start with $B/, $S/, $I/, $L/, $complocaledir/
+	 or / (expands to build dir, source dir, install dir,
+	 library dir (in container), compiled locale dir
+	 (in container), or container's root)
+
        details:
          - '#': A comment.
          - 'su': Enables running test as root in the container.
@@ -108,6 +112,8 @@ int verbose = 0;
          - 'rm': A minimal remove files command.
 	 - 'cwd': set test working directory
 	 - 'exec': change test binary location (may end in /)
+	 - 'mkdirp': A minimal "mkdir -p FILE" command.
+
    * mytest.root/postclean.req causes fresh rsync (with delete) after
      test if present
 
@@ -859,6 +865,7 @@ main (int argc, char **argv)
 	    int nt = tokenize (the_line, the_words, 3);
 	    int i;
 
+	    /* Expand variables.  */
 	    for (i = 1; i < nt; ++i)
 	      {
 		if (memcmp (the_words[i], "$B/", 3) == 0)
@@ -875,6 +882,10 @@ main (int argc, char **argv)
 		  the_words[i] = concat (new_root_path,
 					 support_libdir_prefix,
 					 the_words[i] + 2, NULL);
+		else if (memcmp (the_words[i], "$complocaledir/", 15) == 0)
+		  the_words[i] = concat (new_root_path,
+					 support_complocaledir_prefix,
+					 the_words[i] + 14, NULL);
 		/* "exec" and "cwd" use inside-root paths.  */
 		else if (strcmp (the_words[0], "exec") != 0
 			 && strcmp (the_words[0], "cwd") != 0
@@ -892,6 +903,8 @@ main (int argc, char **argv)
 		  the_words[2] = concat (the_words[2], the_words[1], NULL);
 	      }
 
+	    /* Run the command in the_words[0] with NT number of arguments
+	       (including the command).  */
 	    if (nt == 2 && strcmp (the_words[0], "so") == 0)
 	      {
 		the_words[2] = concat (new_root_path, support_libdir_prefix,
@@ -961,6 +974,12 @@ main (int argc, char **argv)
 	      {
 		be_su = 1;
 	      }
+	    else if (nt == 3 && strcmp (the_words[0], "mkdirp") == 0)
+	      {
+		long int m;
+		m = strtol (the_words[1], NULL, 0);
+		xmkdirp (the_words[2], m);
+	      }
 	    else if (nt > 0 && the_words[0][0] != '#')
 	      {
 		fprintf (stderr, "\033[31minvalid [%s]\033[0m\n", the_words[0]);
-- 
2.21.1

-- 
Cheers,
Carlos.

Re: [PATCH] test-container: Add $complocaledir and mkdirp.

[Florian Weimer]

* Carlos O'Donell via Libc-alpha:

> From 925c47b47d597663606486402c38e2e729ed663c Mon Sep 17 00:00:00 2001
> From: Carlos O'Donell <carlos@redhat.com>
> Date: Thu, 23 Jan 2020 09:45:00 -0500
> Subject: [PATCH] test-container: Add $complocaledir and mkdirp.
>
> In order to test localedef in the test-container framework it
> is necessary to add support for one new variable and one new
> command. For localedef to install a compiled locale in the default
> location the directory needs to be created
> e.g. mkdir -p $complocaledir/dir.  This command requires both the
> new command 'mkdirp' e.g. mkdir -p, and the new variable
> $complocaledir. The only way to avoid this would be to install
> locales into the pristine container root, but that would slow down
> container testing (currently only has builtin C/POSIX available).

You could create the directory unconditionally, without installing any
locales into it?

Thanks,
Florian

Re: [PATCH] test-container: Add $complocaledir and mkdirp.

[DJ Delorie]

"Carlos O'Donell" <carlos@redhat.com> writes:
> Despite me writing "necessary" in the commit message, it's obviously
> not entirely required since you can just use:
>
> +  xmkdirp (support_complocaledir_prefix, 0777);
>
> in the test.

Unless you need to set up a test scenario for the loader, or anything
else that might get touched before main() has a chance to run.  I'll
review as if that were the case, since I think that's the compelling
reason for it ;-)

> One might argue that locales should just be installed by default
> in the test chroot, but I would argue against it. I would say the framework
> should do:

The container currently has "make install" in it.  The framework doesn't
have any way of saying "make install-something-else", such as to install
locales.  I imagine some generic way of doing that, such as a "make"
command in the script, would provide an install-locale option too.

> OK for master?

I have a few minor issues with the patch (mostly comment nits), but OK
by me if you resolve them to your satisfaction.

Reviewed-by: DJ Delorie <dj@redhat.com>

> 8< --- 8< --- 8<
>From 925c47b47d597663606486402c38e2e729ed663c Mon Sep 17 00:00:00 2001
> From: Carlos O'Donell <carlos@redhat.com>
> Date: Thu, 23 Jan 2020 09:45:00 -0500
> Subject: [PATCH] test-container: Add $complocaledir and mkdirp.
>
> In order to test localedef in the test-container framework it
> is necessary to add support for one new variable and one new
> command.

I suspect we should have a generic table-driven way to import variables
from the Makefile, as our build system seems to have lots of them.  I
used shortcuts like $B for those cases that I thought would be common
enough to warrant special cases, but with something like $verylongword/
it might be time to consider the maintainability of that code instead of
just adding more cases to it.

I won't hold up this change for that reason - but I might for the next
variable we import ;-)

> $complocaledir. The only way to avoid this would be to install
> locales into the pristine container root, but that would slow down
> container testing (currently only has builtin C/POSIX available).

Once we have two tests using locales, though, it would be faster to
install it once and share the install, than to install twice.  That's if
we're installing *all* the locales.  I suspect it would be better to let
each test choose which locales it wants (to test presence, absence,
conflict between locales), which means more per-work test.

> -	 FILE must start with $B/, $S/, $I/, $L/, or /
> -	  (expands to build dir, source dir, install dir, library dir
> -	   (in container), or container's root)
> +	 mkdirp MODE DIR
> +
> +	 FILE must start with $B/, $S/, $I/, $L/, $complocaledir/
> +	 or / (expands to build dir, source dir, install dir,
> +	 library dir (in container), compiled locale dir
> +	 (in container), or container's root)
> +

Time to reformat this into a table-like comment I think?

  $B/ build dir
  $S/ source dir
  $I/ install dir
  . . .
  $complocaledir/
      compiled locale dir

>  	 - 'exec': change test binary location (may end in /)
> +	 - 'mkdirp': A minimal "mkdir -p FILE" command.
> +
>     * mytest.root/postclean.req causes fresh rsync (with delete) after

Ok.

> +	    /* Expand variables.  */

Ok.

>  					 the_words[i] + 2, NULL);
> +		else if (memcmp (the_words[i], "$complocaledir/", 15) == 0)
> +		  the_words[i] = concat (new_root_path,
> +					 support_complocaledir_prefix,
> +					 the_words[i] + 14, NULL);
>  		/* "exec" and "cwd" use inside-root paths.  */

Ok.

> +	    /* Run the command in the_words[0] with NT number of arguments
> +	       (including the command).  */
>  	    if (nt == 2 && strcmp (the_words[0], "so") == 0)

This is misleading - it reads like it applies to the "so" command only.
Please leave a blank between the comment and the if to "distance" the
comment a bit, or reword.

>  	      }
> +	    else if (nt == 3 && strcmp (the_words[0], "mkdirp") == 0)
> +	      {
> +		long int m;
> +		m = strtol (the_words[1], NULL, 0);

Error handling?

> +		xmkdirp (the_words[2], m);
> +	      }
>  	    else if (nt > 0 && the_words[0][0] != '#')

Re: [PATCH] test-container: Add $complocaledir and mkdirp.

[Carlos O'Donell]

On 4/27/20 3:49 AM, Florian Weimer wrote:
> * Carlos O'Donell via Libc-alpha:
> 
>> From 925c47b47d597663606486402c38e2e729ed663c Mon Sep 17 00:00:00 2001
>> From: Carlos O'Donell <carlos@redhat.com>
>> Date: Thu, 23 Jan 2020 09:45:00 -0500
>> Subject: [PATCH] test-container: Add $complocaledir and mkdirp.
>>
>> In order to test localedef in the test-container framework it
>> is necessary to add support for one new variable and one new
>> command. For localedef to install a compiled locale in the default
>> location the directory needs to be created
>> e.g. mkdir -p $complocaledir/dir.  This command requires both the
>> new command 'mkdirp' e.g. mkdir -p, and the new variable
>> $complocaledir. The only way to avoid this would be to install
>> locales into the pristine container root, but that would slow down
>> container testing (currently only has builtin C/POSIX available).
> 
> You could create the directory unconditionally, without installing any
> locales into it?

That's a very good point. I know of no reason we would want the directory
*not* to be present. 

My argument is about saving time during testing and avoiding needing to
run 'make localedata/install-locales' and build all the SUPPORTED locales.

I'm going to adjust the patches and break them out like this:
- Create $(complocaledir) when making testroot.pristine
- Add new test.
- Fixup old test which uses xmkdirp to create $(complocaledir) since
  it is no longer required.

-- 
Cheers,
Carlos.

Re: [PATCH] test-container: Add $complocaledir and mkdirp.

[DJ Delorie]

"Carlos O'Donell" <carlos@redhat.com> writes:
> That's a very good point. I know of no reason we would want the directory
> *not* to be present. 

The rule in $srcdir/Makefile that sets up the pristine testroot can do
whatever setup we want.  Or we could modify the "make install" rule to
make that directory, if we think the end user would benefit.

Re: [PATCH] test-container: Add $complocaledir and mkdirp.

[Carlos O'Donell]

On 4/28/20 6:03 PM, DJ Delorie wrote:
> "Carlos O'Donell" <carlos@redhat.com> writes:
>> That's a very good point. I know of no reason we would want the directory
>> *not* to be present. 
> 
> The rule in $srcdir/Makefile that sets up the pristine testroot can do
> whatever setup we want.  Or we could modify the "make install" rule to
> make that directory, if we think the end user would benefit.
> 

Traditionally locale building and installation was a distinct pass from
the installation. You may not need more than just C/POSIX. In fact many
of the container deployments have just C/POSIX and C.UTF-8 now. Thus
creating $(complocaledir) during "make install" seems like a directory
you might not use and would need to remove.

I think if we did it anywhere it would be in the Makefile rule to setup
testroot.pristine, and that's what I'll probably propose.

The question to you is:

I've already tested and created $complocaledir and mkdirp, should we just
add them as additional functionality others would find useful?

Basically my thought is "Yes." because at the end of the day the tests
will all want access to every configured directory we install things
into, and mkdirp seems useful for scripts.

What do you think?

-- 
Cheers,
Carlos.

Re: [PATCH] test-container: Add $complocaledir and mkdirp.

[DJ Delorie]

"Carlos O'Donell" <carlos@redhat.com> writes:
> I've already tested and created $complocaledir and mkdirp, should we just
> add them as additional functionality others would find useful?

Sure.

> Basically my thought is "Yes." because at the end of the day the tests
> will all want access to every configured directory we install things
> into, and mkdirp seems useful for scripts.

Yup.  The goal of a testing toolkit is to provide a set of primitive
features that enable the fastest development of tests, so that
developers have no excuse to not write tests.  Making makefile variables
available to those primitives, and expanding those primitives, adds to
that goal.

[PATCH v2] test-container: Support $(complocaledir) and mkdirp.

[Carlos O'Donell]

On 4/28/20 2:09 PM, DJ Delorie wrote:
> "Carlos O'Donell" <carlos@redhat.com> writes:
>> Despite me writing "necessary" in the commit message, it's obviously
>> not entirely required since you can just use:
>>
>> +  xmkdirp (support_complocaledir_prefix, 0777);
>>
>> in the test.
> 
> Unless you need to set up a test scenario for the loader, or anything
> else that might get touched before main() has a chance to run.  I'll
> review as if that were the case, since I think that's the compelling
> reason for it ;-)

Oh! Good point.

>> One might argue that locales should just be installed by default
>> in the test chroot, but I would argue against it. I would say the framework
>> should do:
> 
> The container currently has "make install" in it.  The framework doesn't
> have any way of saying "make install-something-else", such as to install
> locales.  I imagine some generic way of doing that, such as a "make"
> command in the script, would provide an install-locale option too.
> 
>> OK for master?
> 
> I have a few minor issues with the patch (mostly comment nits), but OK
> by me if you resolve them to your satisfaction.
> 
> Reviewed-by: DJ Delorie <dj@redhat.com>
> 
>> 8< --- 8< --- 8<
>> >From 925c47b47d597663606486402c38e2e729ed663c Mon Sep 17 00:00:00 2001
>> From: Carlos O'Donell <carlos@redhat.com>
>> Date: Thu, 23 Jan 2020 09:45:00 -0500
>> Subject: [PATCH] test-container: Add $complocaledir and mkdirp.
>>
>> In order to test localedef in the test-container framework it
>> is necessary to add support for one new variable and one new
>> command.
> 
> I suspect we should have a generic table-driven way to import variables
> from the Makefile, as our build system seems to have lots of them.  I
> used shortcuts like $B for those cases that I thought would be common
> enough to warrant special cases, but with something like $verylongword/
> it might be time to consider the maintainability of that code instead of
> just adding more cases to it.
> 
> I won't hold up this change for that reason - but I might for the next
> variable we import ;-)

My goal is this:

* Have the variables mirror the configure variables.
- If in configure we call it 'complocaledir' then internally we always
  use 'complocaledir'.

We have strayed from this somewhat with 'support_complocaledir_prefix',
and if anything I'd suggest we would just rename all of these to match
the configure names e.g. complocaledir instead of support_complocaledir_prefix.

>> $complocaledir. The only way to avoid this would be to install
>> locales into the pristine container root, but that would slow down
>> container testing (currently only has builtin C/POSIX available).
> 
> Once we have two tests using locales, though, it would be faster to
> install it once and share the install, than to install twice.  That's if
> we're installing *all* the locales.  I suspect it would be better to let
> each test choose which locales it wants (to test presence, absence,
> conflict between locales), which means more per-work test.

We don't need anything like this yet, but I'd suggest this:

(a) We already compile locales we need via localedata/Makefile (LOCALES).
- In fact we have 14 different Makefile's building locales for testing.

(b) When we generate these locales we should save them in the build
    directory in a special place.
- Refactor all 14 locales to one place, build the locale once, and share
  for all tests.

(c) When a container test needs a specific locale, we look for it in the
    special place, and copy it into place as required (without locale-archive
    support).
- Container tests just copy the locales required into the testroot.root so
  no additional compilation is required.

This means that we compile all required testing locales once, and for
containers we may need to copy them into the container multiple times,
but we assure clean containers for testing with just the locales requested.

(d) Setup additional tests to test locale-archive which would require some
    additional work.

This patch doesn't make this worse because I'm only compiling test locales
not the real locales for testing.

For now, as you say, lets pretend this is additional support for newer tests.

>> -	 FILE must start with $B/, $S/, $I/, $L/, or /
>> -	  (expands to build dir, source dir, install dir, library dir
>> -	   (in container), or container's root)
>> +	 mkdirp MODE DIR
>> +
>> +	 FILE must start with $B/, $S/, $I/, $L/, $complocaledir/
>> +	 or / (expands to build dir, source dir, install dir,
>> +	 library dir (in container), compiled locale dir
>> +	 (in container), or container's root)
>> +
> 
> Time to reformat this into a table-like comment I think?
> 
>   $B/ build dir
>   $S/ source dir
>   $I/ install dir
>   . . .
>   $complocaledir/
>       compiled locale dir
> 

Fixed.

>>  	 - 'exec': change test binary location (may end in /)
>> +	 - 'mkdirp': A minimal "mkdir -p FILE" command.
>> +
>>     * mytest.root/postclean.req causes fresh rsync (with delete) after
> 
> Ok.
> 
>> +	    /* Expand variables.  */
> 
> Ok.
> 
>>  					 the_words[i] + 2, NULL);
>> +		else if (memcmp (the_words[i], "$complocaledir/", 15) == 0)
>> +		  the_words[i] = concat (new_root_path,
>> +					 support_complocaledir_prefix,
>> +					 the_words[i] + 14, NULL);
>>  		/* "exec" and "cwd" use inside-root paths.  */
> 
> Ok.
> 
>> +	    /* Run the command in the_words[0] with NT number of arguments
>> +	       (including the command).  */
>>  	    if (nt == 2 && strcmp (the_words[0], "so") == 0)
> 
> This is misleading - it reads like it applies to the "so" command only.
> Please leave a blank between the comment and the if to "distance" the
> comment a bit, or reword.
> 

Fixed. Added a blank space.

>>  	      }
>> +	    else if (nt == 3 && strcmp (the_words[0], "mkdirp") == 0)
>> +	      {
>> +		long int m;
>> +		m = strtol (the_words[1], NULL, 0);
> 
> Error handling?

It's in xmkdirp, where we do this:

 61   rv = mkdir (path, mode);
 62   if (rv != 0)
 63     FAIL_EXIT1 ("mkdir_p (\"%s\", 0%o): %m", path, mode);

Either the mode is valid and this works, or it's invalid and we
FAIL_EXIT1.

Is that sufficient?
 
>> +		xmkdirp (the_words[2], m);
>> +	      }
>>  	    else if (nt > 0 && the_words[0][0] != '#')
> 

v2
- Rewrote variables section in comments.
- Added blank space to leading comment in command execution block.

8< --- 8< --- 8<
From 7497bc7808cdd3b0a4c02b9e13876b5a93517db6 Mon Sep 17 00:00:00 2001
From: Carlos O'Donell <carlos@redhat.com>
Date: Thu, 23 Jan 2020 09:45:00 -0500
Subject: [PATCH] test-container: Support $(complocaledir) and mkdirp.

Expand the support infrastructure:
- Create $(complocaledir) in the testroot.pristine to support localedef.
- Add the variable $complocaledir to script support.
- Add the script command 'mkdirp'.

All localedef tests which run with default paths need to have the
$(complocaledir) created in testroot.pristine. The localedef binary
will not by itself create the default path, but it will write into
the path. By adding this we can simplify the localedef tests.

The variable $complocaledir is the value of the configured
$(complocaledir) which is the location of the compiled locales that
will be searched by the runtime by default.

The command mkdirp will be available in script setup and will
be equivalent to running `mkdir -p`.

The variable and command can be used to write more complex tests.

Reviewed-by: DJ Delorie <dj@redhat.com>
---
 Makefile                 |  3 +++
 support/test-container.c | 40 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 8f0a93aceb..6dcfe40c25 100644
--- a/Makefile
+++ b/Makefile
@@ -588,6 +588,9 @@ $(objpfx)testroot.pristine/install.stamp :
 	# We need a working /bin/sh for some of the tests.
 	test -d $(objpfx)testroot.pristine/bin || \
 	  mkdir $(objpfx)testroot.pristine/bin
+	# We need the compiled locale dir for localedef tests.
+	test -d $(objpfx)testroot.pristine/$(complocaledir) || \
+	  mkdir -p $(objpfx)testroot.pristine/$(complocaledir)
 	cp $(objpfx)support/shell-container $(objpfx)testroot.pristine/bin/sh
 	cp $(objpfx)support/echo-container $(objpfx)testroot.pristine/bin/echo
 	cp $(objpfx)support/true-container $(objpfx)testroot.pristine/bin/true
diff --git a/support/test-container.c b/support/test-container.c
index dff2ff379e..08aeeae8cc 100644
--- a/support/test-container.c
+++ b/support/test-container.c
@@ -72,6 +72,10 @@ int verbose = 0;
 
    * mkdir $buildroot/testroot.pristine/
    * install into it
+     * default glibc install
+     * create /bin for /bin/sh
+     * create $(complocaledir) so localedef tests work with default paths.
+     * install /bin/sh, /bin/echo, and /bin/true.
    * rsync to $buildroot/testroot.root/
 
    "Per-test" actions:
@@ -97,9 +101,23 @@ int verbose = 0;
 	 rm FILE
 	 cwd PATH
 	 exec FILE
-	 FILE must start with $B/, $S/, $I/, $L/, or /
-	  (expands to build dir, source dir, install dir, library dir
-	   (in container), or container's root)
+	 mkdirp MODE DIR
+
+       variables:
+	 $B/ build dir, equivalent to $(common-objpfx)
+	 $S/ source dir, equivalent to $(srcdir)
+	 $I/ install dir, equivalent to $(prefix)
+	 $L/ library dir (in container), equivalent to $(libdir)
+	 $complocaledir/ compiled locale dir, equivalent to $(complocaledir)
+	 / container's root
+
+	 If FILE begins with any of these variables then they will be
+	 substituted for the described value.
+
+	 The goal is to expose as many of the runtime's configured paths
+	 via variables so they can be used to setup the container environment
+	 before execution reaches the test.
+
        details:
          - '#': A comment.
          - 'su': Enables running test as root in the container.
@@ -108,6 +126,8 @@ int verbose = 0;
          - 'rm': A minimal remove files command.
 	 - 'cwd': set test working directory
 	 - 'exec': change test binary location (may end in /)
+	 - 'mkdirp': A minimal "mkdir -p FILE" command.
+
    * mytest.root/postclean.req causes fresh rsync (with delete) after
      test if present
 
@@ -859,6 +879,7 @@ main (int argc, char **argv)
 	    int nt = tokenize (the_line, the_words, 3);
 	    int i;
 
+	    /* Expand variables.  */
 	    for (i = 1; i < nt; ++i)
 	      {
 		if (memcmp (the_words[i], "$B/", 3) == 0)
@@ -875,6 +896,10 @@ main (int argc, char **argv)
 		  the_words[i] = concat (new_root_path,
 					 support_libdir_prefix,
 					 the_words[i] + 2, NULL);
+		else if (memcmp (the_words[i], "$complocaledir/", 15) == 0)
+		  the_words[i] = concat (new_root_path,
+					 support_complocaledir_prefix,
+					 the_words[i] + 14, NULL);
 		/* "exec" and "cwd" use inside-root paths.  */
 		else if (strcmp (the_words[0], "exec") != 0
 			 && strcmp (the_words[0], "cwd") != 0
@@ -892,6 +917,9 @@ main (int argc, char **argv)
 		  the_words[2] = concat (the_words[2], the_words[1], NULL);
 	      }
 
+	    /* Run the following commands in the_words[0] with NT number of
+	       arguments (including the command).  */
+
 	    if (nt == 2 && strcmp (the_words[0], "so") == 0)
 	      {
 		the_words[2] = concat (new_root_path, support_libdir_prefix,
@@ -961,6 +989,12 @@ main (int argc, char **argv)
 	      {
 		be_su = 1;
 	      }
+	    else if (nt == 3 && strcmp (the_words[0], "mkdirp") == 0)
+	      {
+		long int m;
+		m = strtol (the_words[1], NULL, 0);
+		xmkdirp (the_words[2], m);
+	      }
 	    else if (nt > 0 && the_words[0][0] != '#')
 	      {
 		fprintf (stderr, "\033[31minvalid [%s]\033[0m\n", the_words[0]);
-- 
2.21.1

-- 
Cheers,
Carlos.

Re: [PATCH v2] test-container: Support $(complocaledir) and mkdirp.

[DJ Delorie]

"Carlos O'Donell" <carlos@redhat.com> writes:
> My goal is this:
>
> * Have the variables mirror the configure variables.
> - If in configure we call it 'complocaledir' then internally we always
>   use 'complocaledir'.
>
> We have strayed from this somewhat with 'support_complocaledir_prefix',
> and if anything I'd suggest we would just rename all of these to match
> the configure names e.g. complocaledir instead of support_complocaledir_prefix.

I think the only part that's important is that the Makefile variables
match the $variables in the script.  Internally, since they're global
scope, adding some decoration like support_{variable}_value (or _prefix
or _path, whatever) makes sense to avoid namespace collision.

> (c) When a container test needs a specific locale, we look for it in the
>     special place, and copy it into place as required (without locale-archive
>     support).
> - Container tests just copy the locales required into the testroot.root so
>   no additional compilation is required.

As much as we're complaining about the speed of compiling these, I think
doing it any way other than how the user would install them should be a
non-starter, unless we CLEARLY outline how to test the user-path and
cache-path code seperately.

Although some hackery that *does* the user-path into the testroot, then
caches the results (rsync?) would meet both needs.

> This patch doesn't make this worse because I'm only compiling test locales
> not the real locales for testing.

Yup :-)

>>> +		m = strtol (the_words[1], NULL, 0);
>> 
>> Error handling?
>
> It's in xmkdirp, where we do this:

strtol("Carlos", ...) returns 0.  You'll silently create an unusable
directory.  You need to check errno here, with the usual clear/call/test
code.

>  	test -d $(objpfx)testroot.pristine/bin || \
>  	  mkdir $(objpfx)testroot.pristine/bin
> +	# We need the compiled locale dir for localedef tests.
> +	test -d $(objpfx)testroot.pristine/$(complocaledir) || \
> +	  mkdir -p $(objpfx)testroot.pristine/$(complocaledir)
>  	cp $(objpfx)support/shell-container $(objpfx)testroot.pristine/bin/sh

Ok.

>     * mkdir $buildroot/testroot.pristine/
>     * install into it
> +     * default glibc install
> +     * create /bin for /bin/sh
> +     * create $(complocaledir) so localedef tests work with default paths.
> +     * install /bin/sh, /bin/echo, and /bin/true.
>     * rsync to $buildroot/testroot.root/

Ok.

>  	 exec FILE
> -	 FILE must start with $B/, $S/, $I/, $L/, or /
> -	  (expands to build dir, source dir, install dir, library dir
> -	   (in container), or container's root)
> +	 mkdirp MODE DIR
> +
> +       variables:
> +	 $B/ build dir, equivalent to $(common-objpfx)
> +	 $S/ source dir, equivalent to $(srcdir)
> +	 $I/ install dir, equivalent to $(prefix)
> +	 $L/ library dir (in container), equivalent to $(libdir)
> +	 $complocaledir/ compiled locale dir, equivalent to $(complocaledir)
> +	 / container's root
> +
> +	 If FILE begins with any of these variables then they will be
> +	 substituted for the described value.
> +
> +	 The goal is to expose as many of the runtime's configured paths
> +	 via variables so they can be used to setup the container environment
> +	 before execution reaches the test.
> +
>         details:

Ok.

>  	 - 'exec': change test binary location (may end in /)
> +	 - 'mkdirp': A minimal "mkdir -p FILE" command.
> +

Ok.

>  
> +	    /* Expand variables.  */
>  	    for (i = 1; i < nt; ++i)

Ok.

>  					 the_words[i] + 2, NULL);
> +		else if (memcmp (the_words[i], "$complocaledir/", 15) == 0)
> +		  the_words[i] = concat (new_root_path,
> +					 support_complocaledir_prefix,
> +					 the_words[i] + 14, NULL);
>  		/* "exec" and "cwd" use inside-root paths.  */

Ok.

>  	      }
>  
> +	    /* Run the following commands in the_words[0] with NT number of
> +	       arguments (including the command).  */
> +
>  	    if (nt == 2 && strcmp (the_words[0], "so") == 0)

Ok.

>  	      }
> +	    else if (nt == 3 && strcmp (the_words[0], "mkdirp") == 0)
> +	      {
> +		long int m;
> +		m = strtol (the_words[1], NULL, 0);
> +		xmkdirp (the_words[2], m);
> +	      }
>  	    else if (nt > 0 && the_words[0][0] != '#')

Needs errno check on strtol().

[PATCH v3] test-container: Support $(complocaledir) and mkdirp.

[Carlos O'Donell]

On 4/29/20 1:57 PM, DJ Delorie wrote:
> "Carlos O'Donell" <carlos@redhat.com> writes:
>> My goal is this:
>>
>> * Have the variables mirror the configure variables.
>> - If in configure we call it 'complocaledir' then internally we always
>>   use 'complocaledir'.
>>
>> We have strayed from this somewhat with 'support_complocaledir_prefix',
>> and if anything I'd suggest we would just rename all of these to match
>> the configure names e.g. complocaledir instead of support_complocaledir_prefix.
> 
> I think the only part that's important is that the Makefile variables
> match the $variables in the script.  Internally, since they're global
> scope, adding some decoration like support_{variable}_value (or _prefix
> or _path, whatever) makes sense to avoid namespace collision.
> 
>> (c) When a container test needs a specific locale, we look for it in the
>>     special place, and copy it into place as required (without locale-archive
>>     support).
>> - Container tests just copy the locales required into the testroot.root so
>>   no additional compilation is required.
> 
> As much as we're complaining about the speed of compiling these, I think
> doing it any way other than how the user would install them should be a
> non-starter, unless we CLEARLY outline how to test the user-path and
> cache-path code seperately.
> 
> Although some hackery that *does* the user-path into the testroot, then
> caches the results (rsync?) would meet both needs.
> 
>> This patch doesn't make this worse because I'm only compiling test locales
>> not the real locales for testing.
> 
> Yup :-)
> 
>>>> +		m = strtol (the_words[1], NULL, 0);
>>>
>>> Error handling?
>>
>> It's in xmkdirp, where we do this:
> 
> strtol("Carlos", ...) returns 0.  You'll silently create an unusable
> directory.  You need to check errno here, with the usual clear/call/test
> code.
v3
- Check errno for strtol.

OK for master?

From fb6e2305ae421e42566acf7008367f45f4035d30 Mon Sep 17 00:00:00 2001
From: Carlos O'Donell <carlos@redhat.com>
Date: Thu, 23 Jan 2020 09:45:00 -0500
Subject: [PATCH] test-container: Support $(complocaledir) and mkdirp.

Expand the support infrastructure:
- Create $(complocaledir) in the testroot.pristine to support localedef.
- Add the variable $complocaledir to script support.
- Add the script command 'mkdirp'.

All localedef tests which run with default paths need to have the
$(complocaledir) created in testroot.pristine. The localedef binary
will not by itself create the default path, but it will write into
the path. By adding this we can simplify the localedef tests.

The variable $complocaledir is the value of the configured
$(complocaledir) which is the location of the compiled locales that
will be searched by the runtime by default.

The command mkdirp will be available in script setup and will
be equivalent to running `mkdir -p`.

The variable and command can be used to write more complex tests.

Reviewed-by: DJ Delorie <dj@redhat.com>
---
 Makefile                 |  3 +++
 support/test-container.c | 42 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 8f0a93aceb..6dcfe40c25 100644
--- a/Makefile
+++ b/Makefile
@@ -588,6 +588,9 @@ $(objpfx)testroot.pristine/install.stamp :
 	# We need a working /bin/sh for some of the tests.
 	test -d $(objpfx)testroot.pristine/bin || \
 	  mkdir $(objpfx)testroot.pristine/bin
+	# We need the compiled locale dir for localedef tests.
+	test -d $(objpfx)testroot.pristine/$(complocaledir) || \
+	  mkdir -p $(objpfx)testroot.pristine/$(complocaledir)
 	cp $(objpfx)support/shell-container $(objpfx)testroot.pristine/bin/sh
 	cp $(objpfx)support/echo-container $(objpfx)testroot.pristine/bin/echo
 	cp $(objpfx)support/true-container $(objpfx)testroot.pristine/bin/true
diff --git a/support/test-container.c b/support/test-container.c
index dff2ff379e..08d5195b7e 100644
--- a/support/test-container.c
+++ b/support/test-container.c
@@ -72,6 +72,10 @@ int verbose = 0;
 
    * mkdir $buildroot/testroot.pristine/
    * install into it
+     * default glibc install
+     * create /bin for /bin/sh
+     * create $(complocaledir) so localedef tests work with default paths.
+     * install /bin/sh, /bin/echo, and /bin/true.
    * rsync to $buildroot/testroot.root/
 
    "Per-test" actions:
@@ -97,9 +101,23 @@ int verbose = 0;
 	 rm FILE
 	 cwd PATH
 	 exec FILE
-	 FILE must start with $B/, $S/, $I/, $L/, or /
-	  (expands to build dir, source dir, install dir, library dir
-	   (in container), or container's root)
+	 mkdirp MODE DIR
+
+       variables:
+	 $B/ build dir, equivalent to $(common-objpfx)
+	 $S/ source dir, equivalent to $(srcdir)
+	 $I/ install dir, equivalent to $(prefix)
+	 $L/ library dir (in container), equivalent to $(libdir)
+	 $complocaledir/ compiled locale dir, equivalent to $(complocaledir)
+	 / container's root
+
+	 If FILE begins with any of these variables then they will be
+	 substituted for the described value.
+
+	 The goal is to expose as many of the runtime's configured paths
+	 via variables so they can be used to setup the container environment
+	 before execution reaches the test.
+
        details:
          - '#': A comment.
          - 'su': Enables running test as root in the container.
@@ -108,6 +126,8 @@ int verbose = 0;
          - 'rm': A minimal remove files command.
 	 - 'cwd': set test working directory
 	 - 'exec': change test binary location (may end in /)
+	 - 'mkdirp': A minimal "mkdir -p FILE" command.
+
    * mytest.root/postclean.req causes fresh rsync (with delete) after
      test if present
 
@@ -859,6 +879,7 @@ main (int argc, char **argv)
 	    int nt = tokenize (the_line, the_words, 3);
 	    int i;
 
+	    /* Expand variables.  */
 	    for (i = 1; i < nt; ++i)
 	      {
 		if (memcmp (the_words[i], "$B/", 3) == 0)
@@ -875,6 +896,10 @@ main (int argc, char **argv)
 		  the_words[i] = concat (new_root_path,
 					 support_libdir_prefix,
 					 the_words[i] + 2, NULL);
+		else if (memcmp (the_words[i], "$complocaledir/", 15) == 0)
+		  the_words[i] = concat (new_root_path,
+					 support_complocaledir_prefix,
+					 the_words[i] + 14, NULL);
 		/* "exec" and "cwd" use inside-root paths.  */
 		else if (strcmp (the_words[0], "exec") != 0
 			 && strcmp (the_words[0], "cwd") != 0
@@ -892,6 +917,9 @@ main (int argc, char **argv)
 		  the_words[2] = concat (the_words[2], the_words[1], NULL);
 	      }
 
+	    /* Run the following commands in the_words[0] with NT number of
+	       arguments (including the command).  */
+
 	    if (nt == 2 && strcmp (the_words[0], "so") == 0)
 	      {
 		the_words[2] = concat (new_root_path, support_libdir_prefix,
@@ -961,6 +989,14 @@ main (int argc, char **argv)
 	      {
 		be_su = 1;
 	      }
+	    else if (nt == 3 && strcmp (the_words[0], "mkdirp") == 0)
+	      {
+		long int m;
+		errno = 0;
+		m = strtol (the_words[1], NULL, 0);
+		TEST_COMPARE (errno, 0);
+		xmkdirp (the_words[2], m);
+	      }
 	    else if (nt > 0 && the_words[0][0] != '#')
 	      {
 		fprintf (stderr, "\033[31minvalid [%s]\033[0m\n", the_words[0]);
-- 
2.21.1




-- 
Cheers,
Carlos.

Re: [PATCH v3] test-container: Support $(complocaledir) and mkdirp.

[DJ Delorie]

"Carlos O'Donell" <carlos@redhat.com> writes:
> v3
> - Check errno for strtol.
>
> OK for master?

LGTM
Reviewed-by: DJ Delorie <dj@redhat.com>

> +	    else if (nt == 3 && strcmp (the_words[0], "mkdirp") == 0)
> +	      {
> +		long int m;
> +		errno = 0;
> +		m = strtol (the_words[1], NULL, 0);
> +		TEST_COMPARE (errno, 0);
> +		xmkdirp (the_words[2], m);
> +	      }
>  	    else if (nt > 0 && the_words[0][0] != '#')

Ok.

Re: [PATCH v3] test-container: Support $(complocaledir) and mkdirp.

[Carlos O'Donell]

On 4/29/20 6:57 PM, DJ Delorie wrote:
> "Carlos O'Donell" <carlos@redhat.com> writes:
>> v3
>> - Check errno for strtol.
>>
>> OK for master?
> 
> LGTM
> Reviewed-by: DJ Delorie <dj@redhat.com>
> 
>> +	    else if (nt == 3 && strcmp (the_words[0], "mkdirp") == 0)
>> +	      {
>> +		long int m;
>> +		errno = 0;
>> +		m = strtol (the_words[1], NULL, 0);
>> +		TEST_COMPARE (errno, 0);
>> +		xmkdirp (the_words[2], m);
>> +	      }
>>  	    else if (nt > 0 && the_words[0][0] != '#')
> 
> Ok.
> 

Thanks!

Pushed as #1 in sqequence.

-- 
Cheers,
Carlos.