10:24 p.m.
The set of changes fixes qemuxml2argvtest and closes issue #58.
Note, qemucapsprobe hasn't been fixed yet but it has no impact on the
current test suite.
Pipeline results: https://gitlab.com/roolebo/libvirt/-/pipelines/213377159
Roman Bolshakov (4):
tests: Fix opendir mocks on macOS
tests: Fix mock chaining on macOS
qemuxml2argvtest: Increase timeout on macOS
ci: Run test suite on macOS
ci/cirrus/build.yml | 3 +--
tests/meson.build | 8 +++++++-
tests/virfilewrapper.c | 4 ++++
tests/virmock.h | 37 +++++++++++++++++++++++++++++++++++--
tests/virpcimock.c | 5 +++++
5 files changed, 52 insertions(+), 5 deletions(-)
--
2.29.2
10:24 p.m.
New subject: [PATCH 1/4] tests: Fix opendir mocks on macOS
opendir() mocks need to search for decorated function with $INODE64
suffix, like stat mocks.
Signed-off-by: Roman Bolshakov <r.bolshakov(a)yadro.com>
---
tests/virfilewrapper.c | 4 ++++
tests/virpcimock.c | 4 ++++
2 files changed, 8 insertions(+)
diff --git a/tests/virfilewrapper.c b/tests/virfilewrapper.c
index 0500a3617e..ca2356b5c9 100644
--- a/tests/virfilewrapper.c
+++ b/tests/virfilewrapper.c
@@ -56,7 +56,11 @@ static void init_syms(void)
VIR_MOCK_REAL_INIT(access);
VIR_MOCK_REAL_INIT(mkdir);
VIR_MOCK_REAL_INIT(open);
+# ifdef __APPLE__
+ VIR_MOCK_REAL_INIT_ALIASED(opendir, "opendir$INODE64");
+# else
VIR_MOCK_REAL_INIT(opendir);
+# endif
VIR_MOCK_REAL_INIT(execv);
VIR_MOCK_REAL_INIT(execve);
}
diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index 5cd688c825..686f894e99 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -935,7 +935,11 @@ init_syms(void)
VIR_MOCK_REAL_INIT(__open_2);
# endif /* ! __GLIBC__ */
VIR_MOCK_REAL_INIT(close);
+# ifdef __APPLE__
+ VIR_MOCK_REAL_INIT_ALIASED(opendir, "opendir$INODE64");
+# else
VIR_MOCK_REAL_INIT(opendir);
+# endif
VIR_MOCK_REAL_INIT(virFileCanonicalizePath);
}
--
2.29.2
4:58 p.m.
New subject: [PATCH 1/4] tests: Fix opendir mocks on macOS
On 11/8/20 10:24 PM, Roman Bolshakov wrote:
opendir() mocks need to search for decorated function with $INODE64
suffix, like stat mocks.
Signed-off-by: Roman Bolshakov <r.bolshakov(a)yadro.com>
---
tests/virfilewrapper.c | 4 ++++
tests/virpcimock.c | 4 ++++
2 files changed, 8 insertions(+)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
10:24 p.m.
New subject: [PATCH 2/4] tests: Fix mock chaining on macOS
Some tests in qemuxml2argvtest need opendir() from virpcimock, others
need opendir() from virfilewrapper.
But as of now, only opendir() from virpcimock has an effect.
real_opendir in virpcimock has a pointer to opendir$INODE64 in
libsystem_kernel.dylib instead of pointing to opendir$INODE64 in
qemuxml2argvtest (from virfilewrapper). And because the second one is
never used, tests that rely on prefixes added by virFileWrapperAddPrefix
fail.
That can be fixed if dlsym(3) is asked explicitly to search symbols in
main executable with RTLD_MAIN_ONLY before going to other dylibs.
Existing RTLD_NEXT handle results into libsystem_kernel.dylib being
searched before main executable.
Signed-off-by: Roman Bolshakov <r.bolshakov(a)yadro.com>
---
tests/virmock.h | 37 +++++++++++++++++++++++++++++++++++--
tests/virpcimock.c | 1 +
2 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/tests/virmock.h b/tests/virmock.h
index dea5feb80f..db051f3636 100644
--- a/tests/virmock.h
+++ b/tests/virmock.h
@@ -284,8 +284,19 @@
static void (*real_##name)(void); \
void wrap_##name(void)
+#ifdef __APPLE__
+# define VIR_MOCK_REAL_INIT_MAIN(name, alias) \
+ do { \
+ if (real_##name == NULL) { \
+ real_##name = dlsym(RTLD_MAIN_ONLY, alias); \
+ } \
+ } while (0)
+#else
+# define VIR_MOCK_REAL_INIT_MAIN(name, alias) \
+ do {} while (0)
+#endif
-#define VIR_MOCK_REAL_INIT(name) \
+#define VIR_MOCK_REAL_INIT_NEXT(name) \
do { \
if (real_##name == NULL && \
!(real_##name = dlsym(RTLD_NEXT, \
@@ -295,7 +306,7 @@
} \
} while (0)
-#define VIR_MOCK_REAL_INIT_ALIASED(name, alias) \
+#define VIR_MOCK_REAL_INIT_ALIASED_NEXT(name, alias) \
do { \
if (real_##name == NULL && \
!(real_##name = dlsym(RTLD_NEXT, \
@@ -304,3 +315,25 @@
abort(); \
} \
} while (0)
+
+#ifdef VIR_MOCK_LOOKUP_MAIN
+# define VIR_MOCK_REAL_INIT(name) \
+ do { \
+ VIR_MOCK_REAL_INIT_MAIN(name, #name); \
+ VIR_MOCK_REAL_INIT_NEXT(name); \
+ } while (0)
+# define VIR_MOCK_REAL_INIT_ALIASED(name, alias) \
+ do { \
+ VIR_MOCK_REAL_INIT_MAIN(name, alias); \
+ VIR_MOCK_REAL_INIT_ALIASED_NEXT(name, alias); \
+ } while (0)
+#else
+# define VIR_MOCK_REAL_INIT(name) \
+ do { \
+ VIR_MOCK_REAL_INIT_NEXT(name); \
+ } while (0)
+# define VIR_MOCK_REAL_INIT_ALIASED(name, alias) \
+ do { \
+ VIR_MOCK_REAL_INIT_ALIASED_NEXT(name, alias); \
+ } while (0)
+#endif
diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index 686f894e99..4aa96cae08 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -19,6 +19,7 @@
#include <config.h>
#if defined(__linux__) || defined(__FreeBSD__) || defined(__APPLE__)
+# define VIR_MOCK_LOOKUP_MAIN
# include "virmock.h"
# include <unistd.h>
# include <fcntl.h>
--
2.29.2
4:58 p.m.
New subject: [PATCH 2/4] tests: Fix mock chaining on macOS
On 11/8/20 10:24 PM, Roman Bolshakov wrote:
Some tests in qemuxml2argvtest need opendir() from virpcimock,
others
need opendir() from virfilewrapper.
But as of now, only opendir() from virpcimock has an effect.
real_opendir in virpcimock has a pointer to opendir$INODE64 in
libsystem_kernel.dylib instead of pointing to opendir$INODE64 in
qemuxml2argvtest (from virfilewrapper). And because the second one is
never used, tests that rely on prefixes added by virFileWrapperAddPrefix
fail.
That can be fixed if dlsym(3) is asked explicitly to search symbols in
main executable with RTLD_MAIN_ONLY before going to other dylibs.
Existing RTLD_NEXT handle results into libsystem_kernel.dylib being
searched before main executable.
Signed-off-by: Roman Bolshakov <r.bolshakov(a)yadro.com>
---
tests/virmock.h | 37 +++++++++++++++++++++++++++++++++++--
tests/virpcimock.c | 1 +
2 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/tests/virmock.h b/tests/virmock.h
index dea5feb80f..db051f3636 100644
--- a/tests/virmock.h
+++ b/tests/virmock.h
@@ -284,8 +284,19 @@
static void (*real_##name)(void); \
void wrap_##name(void)
+#ifdef __APPLE__
+# define VIR_MOCK_REAL_INIT_MAIN(name, alias) \
+ do { \
+ if (real_##name == NULL) { \
+ real_##name = dlsym(RTLD_MAIN_ONLY, alias); \
+ } \
+ } while (0)
+#else
+# define VIR_MOCK_REAL_INIT_MAIN(name, alias) \
+ do {} while (0)
+#endif
-#define VIR_MOCK_REAL_INIT(name) \
+#define VIR_MOCK_REAL_INIT_NEXT(name) \
do { \
if (real_##name == NULL && \
!(real_##name = dlsym(RTLD_NEXT, \
@@ -295,7 +306,7 @@
} \
} while (0)
-#define VIR_MOCK_REAL_INIT_ALIASED(name, alias) \
+#define VIR_MOCK_REAL_INIT_ALIASED_NEXT(name, alias) \
do { \
if (real_##name == NULL && \
!(real_##name = dlsym(RTLD_NEXT, \
@@ -304,3 +315,25 @@
abort(); \
} \
} while (0)
+
+#ifdef VIR_MOCK_LOOKUP_MAIN
I think we can do this even without this macro. If the
VIR_MOCK_LOOKUP_MAIN macro is not defined then we are still guarded by
VIR_MOCK_REAL_INIT_MAIN() being a NOP on !__APPLE__. Also ..
+# define VIR_MOCK_REAL_INIT(name) \
+ do { \
+ VIR_MOCK_REAL_INIT_MAIN(name, #name); \
+ VIR_MOCK_REAL_INIT_NEXT(name); \
+ } while (0)
+# define VIR_MOCK_REAL_INIT_ALIASED(name, alias) \
+ do { \
+ VIR_MOCK_REAL_INIT_MAIN(name, alias); \
+ VIR_MOCK_REAL_INIT_ALIASED_NEXT(name, alias); \
+ } while (0)
+#else
+# define VIR_MOCK_REAL_INIT(name) \
+ do { \
+ VIR_MOCK_REAL_INIT_NEXT(name); \
+ } while (0)
+# define VIR_MOCK_REAL_INIT_ALIASED(name, alias) \
+ do { \
+ VIR_MOCK_REAL_INIT_ALIASED_NEXT(name, alias); \
+ } while (0)
+#endif
diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index 686f894e99..4aa96cae08 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -19,6 +19,7 @@
#include <config.h>
#if defined(__linux__) || defined(__FreeBSD__) || defined(__APPLE__)
+# define VIR_MOCK_LOOKUP_MAIN
.. it feels a bit odd that this is defined for virpcimock only.
Michal
8:05 p.m.
New subject: [PATCH 2/4] tests: Fix mock chaining on macOS
On Fri, Nov 13, 2020 at 04:58:39PM +0100, Michal Privoznik wrote:
On 11/8/20 10:24 PM, Roman Bolshakov wrote:
> Some tests in qemuxml2argvtest need opendir() from virpcimock, others
> need opendir() from virfilewrapper.
>
> But as of now, only opendir() from virpcimock has an effect.
> real_opendir in virpcimock has a pointer to opendir$INODE64 in
> libsystem_kernel.dylib instead of pointing to opendir$INODE64 in
> qemuxml2argvtest (from virfilewrapper). And because the second one is
> never used, tests that rely on prefixes added by virFileWrapperAddPrefix
> fail.
>
> That can be fixed if dlsym(3) is asked explicitly to search symbols in
> main executable with RTLD_MAIN_ONLY before going to other dylibs.
> Existing RTLD_NEXT handle results into libsystem_kernel.dylib being
> searched before main executable.
>
> Signed-off-by: Roman Bolshakov <r.bolshakov(a)yadro.com>
> ---
> tests/virmock.h | 37 +++++++++++++++++++++++++++++++++++--
> tests/virpcimock.c | 1 +
> 2 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/tests/virmock.h b/tests/virmock.h
> index dea5feb80f..db051f3636 100644
> --- a/tests/virmock.h
> +++ b/tests/virmock.h
> @@ -284,8 +284,19 @@
> static void (*real_##name)(void); \
> void wrap_##name(void)
> +#ifdef __APPLE__
> +# define VIR_MOCK_REAL_INIT_MAIN(name, alias) \
> + do { \
> + if (real_##name == NULL) { \
> + real_##name = dlsym(RTLD_MAIN_ONLY, alias); \
> + } \
> + } while (0)
> +#else
> +# define VIR_MOCK_REAL_INIT_MAIN(name, alias) \
> + do {} while (0)
> +#endif
> -#define VIR_MOCK_REAL_INIT(name) \
> +#define VIR_MOCK_REAL_INIT_NEXT(name) \
> do { \
> if (real_##name == NULL && \
> !(real_##name = dlsym(RTLD_NEXT, \
> @@ -295,7 +306,7 @@
> } \
> } while (0)
> -#define VIR_MOCK_REAL_INIT_ALIASED(name, alias) \
> +#define VIR_MOCK_REAL_INIT_ALIASED_NEXT(name, alias) \
> do { \
> if (real_##name == NULL && \
> !(real_##name = dlsym(RTLD_NEXT, \
> @@ -304,3 +315,25 @@
> abort(); \
> } \
> } while (0)
> +
> +#ifdef VIR_MOCK_LOOKUP_MAIN
I think we can do this even without this macro. If the VIR_MOCK_LOOKUP_MAIN
macro is not defined then we are still guarded by VIR_MOCK_REAL_INIT_MAIN()
being a NOP on !__APPLE__. Also ..
Yeah, that works for me. I thought of it after I sent the patch :)
I'll improve it, thanks!
> +# define VIR_MOCK_REAL_INIT(name) \
> + do { \
> + VIR_MOCK_REAL_INIT_MAIN(name, #name); \
> + VIR_MOCK_REAL_INIT_NEXT(name); \
> + } while (0)
> +# define VIR_MOCK_REAL_INIT_ALIASED(name, alias) \
> + do { \
> + VIR_MOCK_REAL_INIT_MAIN(name, alias); \
> + VIR_MOCK_REAL_INIT_ALIASED_NEXT(name, alias); \
> + } while (0)
> +#else
> +# define VIR_MOCK_REAL_INIT(name) \
> + do { \
> + VIR_MOCK_REAL_INIT_NEXT(name); \
> + } while (0)
> +# define VIR_MOCK_REAL_INIT_ALIASED(name, alias) \
> + do { \
> + VIR_MOCK_REAL_INIT_ALIASED_NEXT(name, alias); \
> + } while (0)
> +#endif
> diff --git a/tests/virpcimock.c b/tests/virpcimock.c
> index 686f894e99..4aa96cae08 100644
> --- a/tests/virpcimock.c
> +++ b/tests/virpcimock.c
> @@ -19,6 +19,7 @@
> #include <config.h>
> #if defined(__linux__) || defined(__FreeBSD__) || defined(__APPLE__)
> +# define VIR_MOCK_LOOKUP_MAIN
.. it feels a bit odd that this is defined for virpcimock only.
That's the only place where exact lookup order is needed.
Regards,
Roman
10:24 p.m.
New subject: [PATCH 3/4] qemuxml2argvtest: Increase timeout on macOS
The test takes 100+ seconds if all test suite is run with meson and 45+
seconds if it's run directly. According to the output of sample tool,
most of the time (~72 seconds) is spent in poll():
Sort by top of stack, same collapsed (when >= 5):
poll (in libsystem_kernel.dylib) 72396
tiny_free_no_lock (in libsystem_malloc.dylib) 1726
tiny_malloc_should_clear (in libsystem_malloc.dylib) 1671
free_tiny (in libsystem_malloc.dylib) 1287
tiny_free_list_add_ptr (in libsystem_malloc.dylib) 1258
tiny_malloc_from_free_list (in libsystem_malloc.dylib) 1256
Closes https://gitlab.com/libvirt/libvirt/-/issues/58
Signed-off-by: Roman Bolshakov <r.bolshakov(a)yadro.com>
---
tests/meson.build | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tests/meson.build b/tests/meson.build
index 8decdfa20c..3b77a211bb 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -571,7 +571,13 @@ foreach data : tests
],
export_dynamic: true,
)
- test(data['name'], test_bin, env: tests_env)
+ if data['name'] == 'qemuxml2argvtest' and host_machine.system() ==
'darwin'
+ timeout = 180
+ else
+ # default meson timeout
+ timeout = 30
+ endif
+ test(data['name'], test_bin, env: tests_env, timeout: timeout)
endforeach
--
2.29.2
4:58 p.m.
New subject: [PATCH 3/4] qemuxml2argvtest: Increase timeout on macOS
On 11/8/20 10:24 PM, Roman Bolshakov wrote:
The test takes 100+ seconds if all test suite is run with meson and
45+
seconds if it's run directly. According to the output of sample tool,
most of the time (~72 seconds) is spent in poll():
Sort by top of stack, same collapsed (when >= 5):
poll (in libsystem_kernel.dylib) 72396
tiny_free_no_lock (in libsystem_malloc.dylib) 1726
tiny_malloc_should_clear (in libsystem_malloc.dylib) 1671
free_tiny (in libsystem_malloc.dylib) 1287
tiny_free_list_add_ptr (in libsystem_malloc.dylib) 1258
tiny_malloc_from_free_list (in libsystem_malloc.dylib) 1256
Closes https://gitlab.com/libvirt/libvirt/-/issues/58
Signed-off-by: Roman Bolshakov <r.bolshakov(a)yadro.com>
---
tests/meson.build | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tests/meson.build b/tests/meson.build
index 8decdfa20c..3b77a211bb 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -571,7 +571,13 @@ foreach data : tests
],
export_dynamic: true,
)
- test(data['name'], test_bin, env: tests_env)
+ if data['name'] == 'qemuxml2argvtest' and host_machine.system() ==
'darwin'
+ timeout = 180
+ else
+ # default meson timeout
+ timeout = 30
+ endif
+ test(data['name'], test_bin, env: tests_env, timeout: timeout)
endforeach
I think the last time I wanted to increase the timeout I was told that
it is machine specific and since I know my machine the best I should
use: meson test --timeout-multiplier=X
Do you need this for the next cirrus patch? If so I think we should just
add --timeout-multiplier= there.
Michal
8 p.m.
New subject: [PATCH 3/4] qemuxml2argvtest: Increase timeout on macOS
On Fri, 2020-11-13 at 16:58 +0100, Michal Privoznik wrote:
On 11/8/20 10:24 PM, Roman Bolshakov wrote:
> + if data['name'] == 'qemuxml2argvtest' and host_machine.system()
== 'darwin'
> + timeout = 180
> + else
> + # default meson timeout
> + timeout = 30
> + endif
> + test(data['name'], test_bin, env: tests_env, timeout: timeout)
I think the last time I wanted to increase the timeout I was told that
it is machine specific and since I know my machine the best I should
use: meson test --timeout-multiplier=X
Agreed: in my local test environment, for example, the test suite
completes successfully without tweaking the timeout at all. If the
Cirrus CI builders are slower and require a larger timeout that's
perfectly fine, but we should apply the tweak for that environment
only using the command line argument.
--
Andrea Bolognani / Red Hat / Virtualization
11:34 p.m.
New subject: [PATCH 3/4] qemuxml2argvtest: Increase timeout on macOS
On Fri, Nov 13, 2020 at 04:58:36PM +0100, Michal Privoznik wrote:
On 11/8/20 10:24 PM, Roman Bolshakov wrote:
> The test takes 100+ seconds if all test suite is run with meson and 45+
> seconds if it's run directly. According to the output of sample tool,
> most of the time (~72 seconds) is spent in poll():
>
> Sort by top of stack, same collapsed (when >= 5):
> poll (in libsystem_kernel.dylib) 72396
> tiny_free_no_lock (in libsystem_malloc.dylib) 1726
> tiny_malloc_should_clear (in libsystem_malloc.dylib) 1671
> free_tiny (in libsystem_malloc.dylib) 1287
> tiny_free_list_add_ptr (in libsystem_malloc.dylib) 1258
> tiny_malloc_from_free_list (in libsystem_malloc.dylib) 1256
>
> Closes https://gitlab.com/libvirt/libvirt/-/issues/58
> Signed-off-by: Roman Bolshakov <r.bolshakov(a)yadro.com>
> ---
> tests/meson.build | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/tests/meson.build b/tests/meson.build
> index 8decdfa20c..3b77a211bb 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -571,7 +571,13 @@ foreach data : tests
> ],
> export_dynamic: true,
> )
> - test(data['name'], test_bin, env: tests_env)
> + if data['name'] == 'qemuxml2argvtest' and host_machine.system()
== 'darwin'
> + timeout = 180
> + else
> + # default meson timeout
> + timeout = 30
> + endif
> + test(data['name'], test_bin, env: tests_env, timeout: timeout)
> endforeach
>
I think the last time I wanted to increase the timeout I was told that it is
machine specific and since I know my machine the best I should use: meson
test --timeout-multiplier=X
But qumexml2argvtest has 1000+ test cases. And prior to the series the
test never fully passed. In some sense the series raises a baseline (but
with a higher timeout than on Linux).
Do you need this for the next cirrus patch? If so I think we should
just add
--timeout-multiplier= there.
No, I needed it for local environment. And I profiled it a bit to find
where the time is spent [1]. I can also build a flamegraph if it helps.
FWIW, it takes less time in Cirrus CI [2] than on MBP Pro 2012 but
timeout margin is quite narrow for default timeout:
90/127 qemuxml2argvtest OK 29.4992618560791 s
So, I've measured test execution time on a few apple laptops at hand.
On 16" MBP 2019 I get:
90/285 libvirt / qemuxml2argvtest OK 22.11s
On 15" MBP 2012:
80/117 qemuxml2argvtest OK 48.31s
On 13" MBA 2015 running on battery:
80/117 qemuxml2argvtest OK 50.20s
On 13" MBA 2015 running on A/C power:
80/117 qemuxml2argvtest OK 48.71s
Without the explicit timeout everyone has to remember that at least "-t 2"
has to be always passed to "meson test" on darwin. In my opinion,
project defaults should be reasonable without tweaking, to allow nice
"meson test -C build" to run all tests and be sure that they run without
failures if no issues are actually introduced, i.e. there should be no
false positives. This is especially important for "meson dist".
What do you think about other timeout values for qemuxml2argvtest on
darwin: 60s, 90s, 120s, 150s? Is there a chance that one of them would
be ok?
1.
https://www.dropbox.com/s/0rxid8me0976m69/qemuxml2argvtest_2020-11-08_220...
2. https://gitlab.com/roolebo/libvirt/-/jobs/837071634
Thanks,
Roman
11:01 a.m.
New subject: [PATCH 3/4] qemuxml2argvtest: Increase timeout on macOS
On Tue, 2020-11-17 at 01:34 +0300, Roman Bolshakov wrote:
On Fri, Nov 13, 2020 at 04:58:36PM +0100, Michal Privoznik wrote:
> On 11/8/20 10:24 PM, Roman Bolshakov wrote:
> > + if data['name'] == 'qemuxml2argvtest' and
host_machine.system() == 'darwin'
> > + timeout = 180
> > + else
> > + # default meson timeout
> > + timeout = 30
> > + endif
> > + test(data['name'], test_bin, env: tests_env, timeout: timeout)
> > endforeach
>
> I think the last time I wanted to increase the timeout I was told that it is
> machine specific and since I know my machine the best I should use: meson
> test --timeout-multiplier=X
But qumexml2argvtest has 1000+ test cases. And prior to the series the
test never fully passed. In some sense the series raises a baseline (but
with a higher timeout than on Linux).
> Do you need this for the next cirrus patch? If so I think we should just add
> --timeout-multiplier= there.
No, I needed it for local environment. And I profiled it a bit to find
where the time is spent [1]. I can also build a flamegraph if it helps.
FWIW, it takes less time in Cirrus CI [2] than on MBP Pro 2012 but
timeout margin is quite narrow for default timeout:
90/127 qemuxml2argvtest OK 29.4992618560791 s
So, I've measured test execution time on a few apple laptops at hand.
On 16" MBP 2019 I get:
90/285 libvirt / qemuxml2argvtest OK 22.11s
On 15" MBP 2012:
80/117 qemuxml2argvtest OK 48.31s
On 13" MBA 2015 running on battery:
80/117 qemuxml2argvtest OK 50.20s
On 13" MBA 2015 running on A/C power:
80/117 qemuxml2argvtest OK 48.71s
Without the explicit timeout everyone has to remember that at least "-t 2"
has to be always passed to "meson test" on darwin. In my opinion,
project defaults should be reasonable without tweaking, to allow nice
"meson test -C build" to run all tests and be sure that they run without
failures if no issues are actually introduced, i.e. there should be no
false positives. This is especially important for "meson dist".
What do you think about other timeout values for qemuxml2argvtest on
darwin: 60s, 90s, 120s, 150s? Is there a chance that one of them would
be ok?
It's true that we make accomodations so that things work out of the
box as much as possible, and this would fall in line with that trend.
You've almost convinced we want this, but yeah we should make this
maybe 90s - 180s seems quite excessive, given your measurements.
I wonder if we should just bump the timeout for this test
unconditionally: we know it's generally the one that takes the
longest, so even on non-Apple hardware we might very well find that
all other tests finish before the default timeout kicks in but this
one can't quite make it.
That's pretty much hwhat happened not too long ago in Debian: then
6.7.0 was imported, it FTBFS on a bunch of slow architectures due to
this specific test timing out:
https://buildd.debian.org/status/fetch.php?pkg=libvirt&arch=armel&...
The solution was to pass --timeout-multiplier
https://salsa.debian.org/libvirt-team/libvirt/-/commit/b327f9a356c81657ae...
https://salsa.debian.org/libvirt-team/libvirt/-/commit/2a7b4f4ad9285bfc9e...
Even if you bumped the timeout unconditionally, we wouldn't be able
to drop that patch: if you look at the latest build for mipsel
https://buildd.debian.org/status/fetch.php?pkg=libvirt&arch=mipsel&am...
for example, you'll see that *several* tests take longer than 30s to
finish, even those that take a fraction of qemuxml2argv on x86_64, so
I wouldn't be too keen on keeping track of them and singling them out
in meson.build.
But, one thing is old, slow architectures that people almost
certainly don't do day-to-day development on, and another thing is
relatively recent MacBooks :)
--
Andrea Bolognani / Red Hat / Virtualization
6:14 p.m.
New subject: [PATCH 3/4] qemuxml2argvtest: Increase timeout on macOS
On Tue, Nov 17, 2020 at 11:01:21AM +0100, Andrea Bolognani wrote:
On Tue, 2020-11-17 at 01:34 +0300, Roman Bolshakov wrote:
> On Fri, Nov 13, 2020 at 04:58:36PM +0100, Michal Privoznik wrote:
> > On 11/8/20 10:24 PM, Roman Bolshakov wrote:
> > > + if data['name'] == 'qemuxml2argvtest' and
host_machine.system() == 'darwin'
> > > + timeout = 180
> > > + else
> > > + # default meson timeout
> > > + timeout = 30
> > > + endif
> > > + test(data['name'], test_bin, env: tests_env, timeout: timeout)
> > > endforeach
> >
> > I think the last time I wanted to increase the timeout I was told that it is
> > machine specific and since I know my machine the best I should use: meson
> > test --timeout-multiplier=X
>
> But qumexml2argvtest has 1000+ test cases. And prior to the series the
> test never fully passed. In some sense the series raises a baseline (but
> with a higher timeout than on Linux).
>
> > Do you need this for the next cirrus patch? If so I think we should just add
> > --timeout-multiplier= there.
>
> No, I needed it for local environment. And I profiled it a bit to find
> where the time is spent [1]. I can also build a flamegraph if it helps.
>
> FWIW, it takes less time in Cirrus CI [2] than on MBP Pro 2012 but
> timeout margin is quite narrow for default timeout:
>
> 90/127 qemuxml2argvtest OK 29.4992618560791 s
>
> So, I've measured test execution time on a few apple laptops at hand.
>
> On 16" MBP 2019 I get:
> 90/285 libvirt / qemuxml2argvtest OK 22.11s
>
> On 15" MBP 2012:
> 80/117 qemuxml2argvtest OK 48.31s
>
> On 13" MBA 2015 running on battery:
> 80/117 qemuxml2argvtest OK 50.20s
>
> On 13" MBA 2015 running on A/C power:
> 80/117 qemuxml2argvtest OK 48.71s
>
> Without the explicit timeout everyone has to remember that at least "-t
2"
> has to be always passed to "meson test" on darwin. In my opinion,
> project defaults should be reasonable without tweaking, to allow nice
> "meson test -C build" to run all tests and be sure that they run without
> failures if no issues are actually introduced, i.e. there should be no
> false positives. This is especially important for "meson dist".
>
> What do you think about other timeout values for qemuxml2argvtest on
> darwin: 60s, 90s, 120s, 150s? Is there a chance that one of them would
> be ok?
It's true that we make accomodations so that things work out of the
box as much as possible, and this would fall in line with that trend.
You've almost convinced we want this, but yeah we should make this
maybe 90s - 180s seems quite excessive, given your measurements.
Ok, I can change it to 90s.
I wonder if we should just bump the timeout for this test
unconditionally: we know it's generally the one that takes the
longest, so even on non-Apple hardware we might very well find that
all other tests finish before the default timeout kicks in but this
one can't quite make it.
I'm fine if it's unconditionally increased on all platforms for
qemuxml2argvtest. I'll send the change in v2 if nobody has objections.
That's pretty much what happened not too long ago in Debian:
then
6.7.0 was imported, it FTBFS on a bunch of slow architectures due to
this specific test timing out:
https://buildd.debian.org/status/fetch.php?pkg=libvirt&arch=armel&...
The solution was to pass --timeout-multiplier
https://salsa.debian.org/libvirt-team/libvirt/-/commit/b327f9a356c81657ae...
https://salsa.debian.org/libvirt-team/libvirt/-/commit/2a7b4f4ad9285bfc9e...
Even if you bumped the timeout unconditionally, we wouldn't be able
to drop that patch: if you look at the latest build for mipsel
https://buildd.debian.org/status/fetch.php?pkg=libvirt&arch=mipsel&am...
for example, you'll see that *several* tests take longer than 30s to
finish, even those that take a fraction of qemuxml2argv on x86_64, so
I wouldn't be too keen on keeping track of them and singling them out
in meson.build.
But, one thing is old, slow architectures that people almost
certainly don't do day-to-day development on, and another thing is
relatively recent MacBooks :)
I doubt there are many users/developers of libvirtd on macOS right now
because there's no upstream Hypervisor.framework and
Virtualization.framework support. libvirt is used as a standalone client
tool [1] and for virt-manager [2]. The only usable qemu domain in
libvirtd is TCG.
With regards to HVF, I'm ressurecting my HVF patchset [3] as all
outstanding issues (syntax check, the tests, qemucapsprobe) are mostly
fixed or about to be fixed. There's also an outstanding patch series to
QEMU that improves reporting of non KVM accelerators [4]. Once we add
HVF and may be HAXM domain support, there will be much more interest in
libvirt, qemu on macOS. So, convenient development environment would be
important to ease further contributions.
1. https://formulae.brew.sh/formula/libvirt
2. https://github.com/jeffreywildman/homebrew-virt-manager
3. https://www.redhat.com/archives/libvir-list/2018-November/msg00802.html
4. https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg03944.html
Thanks,
Roman
10:24 p.m.
New subject: [PATCH 4/4] ci: Run test suite on macOS
There's no need to have different CI process between macOS and FreeBSD
as test suite has been fixed on macOS.
Signed-off-by: Roman Bolshakov <r.bolshakov(a)yadro.com>
---
ci/cirrus/build.yml | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/ci/cirrus/build.yml b/ci/cirrus/build.yml
index 912284b906..dc1cccd135 100644
--- a/ci/cirrus/build.yml
+++ b/ci/cirrus/build.yml
@@ -20,5 +20,4 @@ build_task:
- git reset --hard "$CI_COMMIT_SHA"
build_script:
- meson build --prefix=$(pwd)/install-root
- - if test "$(uname)" = "FreeBSD"; then ninja -C build dist; fi
- - if test "$(uname)" = "Darwin"; then ninja -C build &&
ninja -C build install; fi
+ - ninja -C build dist
--
2.29.2
4:58 p.m.
New subject: [PATCH 4/4] ci: Run test suite on macOS
On 11/8/20 10:24 PM, Roman Bolshakov wrote:
There's no need to have different CI process between macOS and
FreeBSD
as test suite has been fixed on macOS.
Signed-off-by: Roman Bolshakov <r.bolshakov(a)yadro.com>
---
ci/cirrus/build.yml | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/ci/cirrus/build.yml b/ci/cirrus/build.yml
index 912284b906..dc1cccd135 100644
--- a/ci/cirrus/build.yml
+++ b/ci/cirrus/build.yml
@@ -20,5 +20,4 @@ build_task:
- git reset --hard "$CI_COMMIT_SHA"
build_script:
- meson build --prefix=$(pwd)/install-root
- - if test "$(uname)" = "FreeBSD"; then ninja -C build dist; fi
- - if test "$(uname)" = "Darwin"; then ninja -C build &&
ninja -C build install; fi
+ - ninja -C build dist
If we go with --timeout-multiplier= as I'm suggesting in 3/4 then this
can't be ninja, but meson. I'm not sure what the whole point of 'ninja'
is at this point, sorry.
Michal
8:08 p.m.
New subject: [PATCH 4/4] ci: Run test suite on macOS
On Fri, 2020-11-13 at 16:58 +0100, Michal Privoznik wrote:
On 11/8/20 10:24 PM, Roman Bolshakov wrote:
> - - if test "$(uname)" = "FreeBSD"; then ninja -C build dist;
fi
> - - if test "$(uname)" = "Darwin"; then ninja -C build
&& ninja -C build install; fi
> + - ninja -C build dist
If we go with --timeout-multiplier= as I'm suggesting in 3/4 then this
can't be ninja, but meson. I'm not sure what the whole point of 'ninja'
is at this point, sorry.
ninja is the low-level tool, so in some cases (notably running the
test suite) having meson call ninja instead of invoking the latter
directly can enable additional features.
In this case, I assume 'ninja dist' will use 'ninja test' rather than
'meson test', so we might have to do something like
- if test "$(uname)" = "FreeBSD"; then cd build && ninja
dist; fi
- if test "$(uname)" = "Darwin"; then cd build && ninja
&& meson test --timeout-multiplier=X && ninja install; fi
to run the test suite on macOS without hitting the timeout.
--
Andrea Bolognani / Red Hat / Virtualization
8:10 a.m.
New subject: [PATCH 4/4] ci: Run test suite on macOS
On 11/13/20 8:08 PM, Andrea Bolognani wrote:
On Fri, 2020-11-13 at 16:58 +0100, Michal Privoznik wrote:
> On 11/8/20 10:24 PM, Roman Bolshakov wrote:
>> - - if test "$(uname)" = "FreeBSD"; then ninja -C build
dist; fi
>> - - if test "$(uname)" = "Darwin"; then ninja -C build
&& ninja -C build install; fi
>> + - ninja -C build dist
>
> If we go with --timeout-multiplier= as I'm suggesting in 3/4 then this
> can't be ninja, but meson. I'm not sure what the whole point of
'ninja'
> is at this point, sorry.
ninja is the low-level tool, so in some cases (notably running the
test suite) having meson call ninja instead of invoking the latter
directly can enable additional features.
And I guess that's my problem with it. The --timeout-multiplier= is
argument of meson, but if I want to run a single threaded build (useful
if I mess up something in a header file that's included from everywhere)
there's no way to pass "-j1" to meson and I have to use ninja. But guess
what, ninja doesn't accept --timeout-multiplier (nor does its -h output
suggest something of that kind). I'd understand if one was "user
friendly" interface of the other, but that doesn't seem to be the case.
I guess I'm annoyed with having to use different tool each time.
Michal
2:46 p.m.
New subject: [PATCH 4/4] ci: Run test suite on macOS
On Fri, Nov 20, 2020 at 08:10:57AM +0100, Michal Privoznik wrote:
On 11/13/20 8:08 PM, Andrea Bolognani wrote:
> On Fri, 2020-11-13 at 16:58 +0100, Michal Privoznik wrote:
> > On 11/8/20 10:24 PM, Roman Bolshakov wrote:
> > > - - if test "$(uname)" = "FreeBSD"; then ninja -C
build dist; fi
> > > - - if test "$(uname)" = "Darwin"; then ninja -C
build && ninja -C build install; fi
> > > + - ninja -C build dist
> >
> > If we go with --timeout-multiplier= as I'm suggesting in 3/4 then this
> > can't be ninja, but meson. I'm not sure what the whole point of
'ninja'
> > is at this point, sorry.
>
> ninja is the low-level tool, so in some cases (notably running the
> test suite) having meson call ninja instead of invoking the latter
> directly can enable additional features.
And I guess that's my problem with it. The --timeout-multiplier= is argument
of meson, but if I want to run a single threaded build (useful if I mess up
something in a header file that's included from everywhere) there's no way
to pass "-j1" to meson and I have to use ninja. But guess what, ninja
doesn't accept --timeout-multiplier (nor does its -h output suggest
something of that kind). I'd understand if one was "user friendly"
interface
of the other, but that doesn't seem to be the case.
I guess I'm annoyed with having to use different tool each time.
Michal
Hi Michal,
My intention with the patch was to remove the difference (skipping tests
on macOS) that is no longer needed. FreeBSD had the most concise
invocation, so I kept it.
It's not trouble for me to update it to "meson dist" [1] if you think
it's more flexible (available since 0.52.0).
1. https://mesonbuild.com/Creating-releases.html
Thanks,
Roman
4:46 p.m.
New subject: [PATCH 4/4] ci: Run test suite on macOS
On Mon, 2020-11-23 at 16:46 +0300, Roman Bolshakov wrote:
My intention with the patch was to remove the difference (skipping
tests
on macOS) that is no longer needed. FreeBSD had the most concise
invocation, so I kept it.
It's not trouble for me to update it to "meson dist" [1] if you think
it's more flexible (available since 0.52.0).
The GitLab CI configuration runs 'ninja dist' (as well as 'ninja
test'), so let's stick with that one. We might want to change those
across the board, but that would be a completely separate patch :)
--
Andrea Bolognani / Red Hat / Virtualization
10:17 a.m.
New subject: [PATCH 4/4] ci: Run test suite on macOS
On 11/23/20 2:46 PM, Roman Bolshakov wrote:
On Fri, Nov 20, 2020 at 08:10:57AM +0100, Michal Privoznik wrote:
> On 11/13/20 8:08 PM, Andrea Bolognani wrote:
>> On Fri, 2020-11-13 at 16:58 +0100, Michal Privoznik wrote:
>>> On 11/8/20 10:24 PM, Roman Bolshakov wrote:
>>>> - - if test "$(uname)" = "FreeBSD"; then ninja -C
build dist; fi
>>>> - - if test "$(uname)" = "Darwin"; then ninja -C
build && ninja -C build install; fi
>>>> + - ninja -C build dist
>>>
>>> If we go with --timeout-multiplier= as I'm suggesting in 3/4 then this
>>> can't be ninja, but meson. I'm not sure what the whole point of
'ninja'
>>> is at this point, sorry.
>>
>> ninja is the low-level tool, so in some cases (notably running the
>> test suite) having meson call ninja instead of invoking the latter
>> directly can enable additional features.
>
> And I guess that's my problem with it. The --timeout-multiplier= is argument
> of meson, but if I want to run a single threaded build (useful if I mess up
> something in a header file that's included from everywhere) there's no way
> to pass "-j1" to meson and I have to use ninja. But guess what, ninja
> doesn't accept --timeout-multiplier (nor does its -h output suggest
> something of that kind). I'd understand if one was "user friendly"
interface
> of the other, but that doesn't seem to be the case.
> I guess I'm annoyed with having to use different tool each time.
>
> Michal
>
Hi Michal,
My intention with the patch was to remove the difference (skipping tests
on macOS) that is no longer needed. FreeBSD had the most concise
invocation, so I kept it.
It's not trouble for me to update it to "meson dist" [1] if you think
it's more flexible (available since 0.52.0).
My e-mail was more of a rant than suggestion for you to change anything.
Well, I guess every build system is all sunshine and roses until you
start using it.
Michal
1395
days inactive
1411
days old
18 comments
4 participants
participants (4)
-
Andrea Bolognani -
Michal Privoznik -
Michal Prívozník
-
Roman Bolshakov