Martin Kletzander
2021-Jun-08 19:09 UTC
[Libguestfs] [libnbd PATCH 17/20] ci: Add support for test skipping
On Tue, Jun 08, 2021 at 05:22:06PM +0100, Richard W.M. Jones wrote:>On Tue, Jun 08, 2021 at 03:14:21PM +0200, Martin Kletzander wrote: >> On Tue, Jun 08, 2021 at 08:03:57AM -0500, Eric Blake wrote: >> >On Tue, Jun 08, 2021 at 09:53:58AM +0200, Martin Kletzander wrote: >> >>Some tests cannot be ran successfully and/or cannot properly probe for all their >> >>dependency configurations. This allows for skipping of individual test cases >> >>based on the OS and its version. >> >> >> >>Signed-off-by: Martin Kletzander <mkletzan at redhat.com> >> >>--- >> >> ci/build_script.sh | 60 +++++++++++++++++++++++++++++++++++++++++++++- >> >> 1 file changed, 59 insertions(+), 1 deletion(-) >> >> >> >>diff --git a/ci/build_script.sh b/ci/build_script.sh >> >>index cecbbba0bbc0..36bd51eb8522 100755 >> >>--- a/ci/build_script.sh >> >>+++ b/ci/build_script.sh >> >>@@ -32,7 +32,65 @@ main() { >> >> return 0 >> >> fi >> >> >> >>- $MAKE check >> >>+ # Add a way to run all the tests, even the skipped ones, with an environment >> >>+ # variable, so that it can be set fora branch or fork in GitLab. >> > >> >for a >> > >> >>+ if test "$SKIPPED_TESTS" != "force" >> >>+ then >> >>+ # Skip tests from ci/skipped_tests if this is the right OS version >> >>+ # The file >> >>+ local os_id >> > >> >ci/build_script.sh is currently listed as #!/bin/sh, but local is a bashism. >> > >> >> Oh, and it's not as easy as that either as I see: >> >> https://stackoverflow.com/questions/18597697/posix-compliant-way-to-scope-variables-to-a-function-in-a-shell-script >> >> Anyway, I'll just remove the `local`. I did not go through all the >> obstacles of making it POSIX-compatible to just throw it away for some >> variable scoping =) >> >> I see I made the same mistake in one previous patch as well, so I'll >> clean that too. > >We're allowed to assume /bin/bash exists in general libnbd tests, >although I don't know if that applies here since I'm not totally clear >if these CI scripts run in the environment that has all the deps >installed or not. >I wanted to keep the build script as portable as reasonably possible. Local variable scoping looks to be available even in the most limited of shells, but there is not need for this here. It was just one thing that I learned because I've seen someone else get bitten by not using it =) This is a linear script that does not even need a function let alone any scopes. Coincidentally it is yet another thing I learned in a very similar fashion. In the meantime I added a fix/workaround for CentOS-es and fixed the FreeBSD weirdness. The only thing missing now is tests on FreeBSD, which fail a lot, and (if there is some spare time) fix up some package name mapping for MacOS. I still have one more thing that bothers me, though. The symlinks in bash/ are set to be generated and packed into the distribution (using the `dist_` prefix for `dist_bashcompdir_DATA`), but they are defined conditionally based on whether bash-completion (and libxml2) are available on the system. This, of course, breaks `make dist` for systems where any of the packages is not available. I think that there are few ways how to get out of it: 1) Remove the `dist_` prefix -- this would generate the files during build time when the availability of the dependencies is more relevant. 2) Remove the conditionals -- creating the files only relies only on `ln` to create the symlink so there is no need for the dependency to be available. 3) Remove the rules and commit the symlinks to the repository -- this would keep the symlinks in git, they would get installed based on the conditionals, but would be always available for packing into the distribution, at least that's how I understand automake. I apologise for hijacking unrelated thread for this, but I would really like your opinion on that. Have a nice day, Martin>Rich. > >-- >Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones >Read my programming and virtualization blog: http://rwmj.wordpress.com >virt-top is 'top' for virtual machines. Tiny program with many >powerful monitoring features, net stats, disk stats, logging, etc. >http://people.redhat.com/~rjones/virt-top >-------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 833 bytes Desc: not available URL: <http://listman.redhat.com/archives/libguestfs/attachments/20210608/bb4b3f9a/attachment.sig>
Eric Blake
2021-Jun-09 16:00 UTC
[Libguestfs] distributing bash completion links [was: [libnbd PATCH 17/20] ci: Add support for test skipping]
On Tue, Jun 08, 2021 at 09:09:32PM +0200, Martin Kletzander wrote:> I still have one more thing that bothers me, though. The symlinks in > bash/ are set to be generated and packed into the distribution (using > the `dist_` prefix for `dist_bashcompdir_DATA`), but they are defined > conditionally based on whether bash-completion (and libxml2) are > available on the system. This, of course, breaks `make dist` for > systems where any of the packages is not available. I think that there > are few ways how to get out of it:Yeah, it's never a good idea to have the contents of a tarball differ according to what was present on the machine of the person running 'make dist'. Tarballs should be invariant, such that what gets installed should be determined solely by the results of ./configure on the machine unpacking the tarball, not on the machine that created the tarball. So you have indeed uncovered a bug that needs fixing.> > 1) Remove the `dist_` prefix -- this would generate the files during > build time when the availability of the dependencies is more relevant.That means the files only get installed on platforms where configure says it is useful to install them, rather than being part of the tarball. Seems reasonable.> > 2) Remove the conditionals -- creating the files only relies only on > `ln` to create the symlink so there is no need for the dependency to > be available.That says the files should always be part of the tarball, regardless of whether they will be installed. Also seems reasonable.> > 3) Remove the rules and commit the symlinks to the repository -- this > would keep the symlinks in git, they would get installed based on the > conditionals, but would be always available for packing into the > distribution, at least that's how I understand automake.The autotools frown on shipping a tarball with a symlink (because not all systems support symlinks, so extracting such a tarball is not portable). That recommendation still exists today, where the major culprits are Windows and FAT filesystems. Windows actually supports symlinks on NTFS file systems, but not necessarily out-of-the-box, but more importantly, anyone who unpacks the tarball on a flash drive (which are most-often formatted as FAT) suffer from the problem. So just as we shouldn't stick symlinks in the tarball (but instead should generate them during './config.status'), we probably shouldn't need to store symlinks in git. Of all of these options, I'm leaning the most towards 1. But it's not at the top of my priority list at the moment, so I suspect it will be easier for you to propose a patch and me review it than for me to try and write the patch. Thanks for working on CI, by the way! -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org