* [PATCH] test-container: Add $complocaledir and mkdirp. @ 2020-04-26 19:17 Carlos O'Donell 2020-04-27 7:49 ` Florian Weimer 2020-04-28 18:09 ` DJ Delorie 0 siblings, 2 replies; 12+ messages in thread From: Carlos O'Donell @ 2020-04-26 19:17 UTC (permalink / raw) To: libc-alpha, DJ Delorie 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] test-container: Add $complocaledir and mkdirp. 2020-04-26 19:17 [PATCH] test-container: Add $complocaledir and mkdirp Carlos O'Donell @ 2020-04-27 7:49 ` Florian Weimer 2020-04-28 19:59 ` Carlos O'Donell 2020-04-28 18:09 ` DJ Delorie 1 sibling, 1 reply; 12+ messages in thread From: Florian Weimer @ 2020-04-27 7:49 UTC (permalink / raw) To: Carlos O'Donell via Libc-alpha * 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] test-container: Add $complocaledir and mkdirp. 2020-04-27 7:49 ` Florian Weimer @ 2020-04-28 19:59 ` Carlos O'Donell 2020-04-28 22:03 ` DJ Delorie 0 siblings, 1 reply; 12+ messages in thread From: Carlos O'Donell @ 2020-04-28 19:59 UTC (permalink / raw) To: Florian Weimer, Carlos O'Donell via Libc-alpha 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] test-container: Add $complocaledir and mkdirp. 2020-04-28 19:59 ` Carlos O'Donell @ 2020-04-28 22:03 ` DJ Delorie 2020-04-28 23:44 ` Carlos O'Donell 0 siblings, 1 reply; 12+ messages in thread From: DJ Delorie @ 2020-04-28 22:03 UTC (permalink / raw) To: Carlos O'Donell; +Cc: fweimer, libc-alpha "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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] test-container: Add $complocaledir and mkdirp. 2020-04-28 22:03 ` DJ Delorie @ 2020-04-28 23:44 ` Carlos O'Donell 2020-04-29 0:12 ` DJ Delorie 0 siblings, 1 reply; 12+ messages in thread From: Carlos O'Donell @ 2020-04-28 23:44 UTC (permalink / raw) To: DJ Delorie; +Cc: fweimer, libc-alpha 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] test-container: Add $complocaledir and mkdirp. 2020-04-28 23:44 ` Carlos O'Donell @ 2020-04-29 0:12 ` DJ Delorie 0 siblings, 0 replies; 12+ messages in thread From: DJ Delorie @ 2020-04-29 0:12 UTC (permalink / raw) To: Carlos O'Donell; +Cc: fweimer, libc-alpha "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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] test-container: Add $complocaledir and mkdirp. 2020-04-26 19:17 [PATCH] test-container: Add $complocaledir and mkdirp Carlos O'Donell 2020-04-27 7:49 ` Florian Weimer @ 2020-04-28 18:09 ` DJ Delorie 2020-04-29 15:55 ` [PATCH v2] test-container: Support $(complocaledir) " Carlos O'Donell 1 sibling, 1 reply; 12+ messages in thread From: DJ Delorie @ 2020-04-28 18:09 UTC (permalink / raw) To: Carlos O'Donell; +Cc: libc-alpha "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] != '#') ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] test-container: Support $(complocaledir) and mkdirp. 2020-04-28 18:09 ` DJ Delorie @ 2020-04-29 15:55 ` Carlos O'Donell 2020-04-29 17:57 ` DJ Delorie 0 siblings, 1 reply; 12+ messages in thread From: Carlos O'Donell @ 2020-04-29 15:55 UTC (permalink / raw) To: DJ Delorie; +Cc: libc-alpha 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] test-container: Support $(complocaledir) and mkdirp. 2020-04-29 15:55 ` [PATCH v2] test-container: Support $(complocaledir) " Carlos O'Donell @ 2020-04-29 17:57 ` DJ Delorie 2020-04-29 21:31 ` [PATCH v3] " Carlos O'Donell 0 siblings, 1 reply; 12+ messages in thread From: DJ Delorie @ 2020-04-29 17:57 UTC (permalink / raw) To: Carlos O'Donell; +Cc: libc-alpha "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(). ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3] test-container: Support $(complocaledir) and mkdirp. 2020-04-29 17:57 ` DJ Delorie @ 2020-04-29 21:31 ` Carlos O'Donell 2020-04-29 22:57 ` DJ Delorie 0 siblings, 1 reply; 12+ messages in thread From: Carlos O'Donell @ 2020-04-29 21:31 UTC (permalink / raw) To: DJ Delorie; +Cc: libc-alpha 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] test-container: Support $(complocaledir) and mkdirp. 2020-04-29 21:31 ` [PATCH v3] " Carlos O'Donell @ 2020-04-29 22:57 ` DJ Delorie 2020-04-30 20:29 ` Carlos O'Donell 0 siblings, 1 reply; 12+ messages in thread From: DJ Delorie @ 2020-04-29 22:57 UTC (permalink / raw) To: Carlos O'Donell; +Cc: libc-alpha "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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] test-container: Support $(complocaledir) and mkdirp. 2020-04-29 22:57 ` DJ Delorie @ 2020-04-30 20:29 ` Carlos O'Donell 0 siblings, 0 replies; 12+ messages in thread From: Carlos O'Donell @ 2020-04-30 20:29 UTC (permalink / raw) To: DJ Delorie; +Cc: libc-alpha 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-04-30 20:29 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-26 19:17 [PATCH] test-container: Add $complocaledir and mkdirp Carlos O'Donell 2020-04-27 7:49 ` Florian Weimer 2020-04-28 19:59 ` Carlos O'Donell 2020-04-28 22:03 ` DJ Delorie 2020-04-28 23:44 ` Carlos O'Donell 2020-04-29 0:12 ` DJ Delorie 2020-04-28 18:09 ` DJ Delorie 2020-04-29 15:55 ` [PATCH v2] test-container: Support $(complocaledir) " Carlos O'Donell 2020-04-29 17:57 ` DJ Delorie 2020-04-29 21:31 ` [PATCH v3] " Carlos O'Donell 2020-04-29 22:57 ` DJ Delorie 2020-04-30 20:29 ` Carlos O'Donell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).