diff mbox series

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

Message ID 643f2f94-6797-874e-04aa-79e4599b4e3c@redhat.com
State New
Headers show
Series [v2] test-container: Support $(complocaledir) and mkdirp. | expand

Commit Message

Carlos O'Donell April 29, 2020, 3:55 p.m. UTC 
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 mbox series

Patch

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]);