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

[DJ Delorie <dj@redhat.com>]

"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] != '#')