here's xrdp development talk room, questions here: https://gitter.im/neutrinolabs/xrdp-questions
I have a friend whose organization will gladly adopt XRDP if we can get this implemented and working in a CentOS build of XRDP: neutrinolabs/xrdp#547
So I'm going to pause EGFX work temporarily and focus on this.
Hello, I need your help.
Currently Ulalaca has a problem with crashing randomly when lib_mod_event(...) is called by moving the mouse or typing with the keyboard, and it seems to be related around lib_mod_get_wait_objs(robjs, rcount, wobjs, wcount, timeout) and lib_mod_check_wait_objs() which used in flow control.
But I don't know how to implement these functions and what to return.
I checked the code of other module implementations, and it looks like wait_obj is passed from struct trans, but in Ulalaca, the part that communicates with the Unix Domain Socket is written independently.
I tried to create a wait_obj, which locks the logic that sends event data to projector, and passed it to wobjs, but that didn't work either.
Do I need to rewrite the part that communicates with UDS using struct trans to support flow control, instead of my own implementation?
@Nexarian - this patch fixes the linking issues:-
diff --git a/configure.ac b/configure.ac
index d6414bb6..9a973042 100644
--- a/configure.ac
+++ b/configure.ac
@@ -519,18 +519,23 @@ AC_CHECK_HEADER([X11/extensions/Xrandr.h], [],
CFLAGS="$save_CFLAGS"
-# perform unit tests if libcheck found
+# perform unit tests if libcheck and libmocka found
+perform_unit_tests=yes ;# Assume packages will be found
if test "x$ensure_tests_deps" == "xyes"; then
PKG_CHECK_MODULES([CHECK], [check >= 0.10.0],
- [perform_unit_tests=yes],
+ [],
[AC_MSG_ERROR([please install check, the unit test framework])])
# Check if cmocka is available - needed for unit testing
PKG_CHECK_MODULES([CMOCKA], [cmocka],
- [have_cmocka=yes],
- [AC_MSG_WARN([please install cmocka, the mocking framework])])
+ [],
+ [AC_MSG_ERROR([please install cmocka, the mocking framework])])
else
+ perform_unit_tests=yes
PKG_CHECK_MODULES([CHECK], [check >= 0.10.0],
- [perform_unit_tests=yes],
+ [],
+ [perform_unit_tests=no])
+ PKG_CHECK_MODULES([CMOCKA], [cmocka],
+ [],
[perform_unit_tests=no])
fi
diff --git a/tests/xrdp/Makefile.am b/tests/xrdp/Makefile.am
index 3dad7972..1f830753 100644
--- a/tests/xrdp/Makefile.am
+++ b/tests/xrdp/Makefile.am
@@ -33,7 +33,8 @@ test_xrdp_SOURCES = \
test_xrdp_CFLAGS = \
-D IMAGEDIR=\"$(srcdir)\" \
- @CHECK_CFLAGS@
+ @CHECK_CFLAGS@ \
+ @CMOCKA_CFLAGS@
test_xrdp_LDFLAGS = -Wl,--wrap=pixman_region_extents, \
-Wl,--wrap=pixman_region_not_empty
@@ -63,5 +64,5 @@ test_xrdp_LDADD = \
$(top_builddir)/xrdp/xrdp_main_utils.o \
$(PIXMAN_LIBS) \
$(IMLIB2_LIBS) \
- $(CMOCKA_LIBS) \
- @CHECK_LIBS@
+ @CHECK_LIBS@ \
+ @CMOCKA_LIBS@
diff --git a/tests/xrdp/test_xrdp_region.c b/tests/xrdp/test_xrdp_region.c
index 86aeabad..36e7a165 100644
--- a/tests/xrdp/test_xrdp_region.c
+++ b/tests/xrdp/test_xrdp_region.c
@@ -47,11 +47,27 @@
#include <stddef.h>
#include <stdint.h>
#include <setjmp.h>
+// cmocka.h lacks c++ guards for CI
+#ifdef __cplusplus
+extern "C" {
+#endif
#include <cmocka.h>
+#ifdef __cplusplus
+}
+#endif
#define UNUSED(x) (void)(x)
// Mock functions
+// C++ needs extern 'C' for wrapping to work correctly
+#ifdef __cplusplus
+extern "C"
+{
+pixman_box16_t *__wrap_pixman_region_extents(pixman_region16_t *region);
+pixman_bool_t __wrap_pixman_region_not_empty(pixman_region16_t *region);
+}
+
+#endif
static pixman_box16_t test_box =
{
0, 0, 100, 100There are two problems I encountered. The first was linking with cmocka, and the second was getting the wrapping to work with C++ name mangling.
Even with this, the test is failing on my rig, but I'm sure you can sort that out.
You might want to change what I've posted her, but hopefully it will give you a helping hand.
@unstabler - lib_mod_get_wait_objs() adds a list of file descriptors you are interest in reading or writing from to the passed-in arrays.
lib_mod_check_wait_objs is used to read and write to your file descriptors.
You shouldn't need to use struct trans. Have a look in neutrinordp/xrdp-neutrinordp.c where these functions are implemented for interfacing to a 3rd part library.
HTH
#ifdef __cplusplus block. This can relatively easily be wrapped I think for more uses of libmocka. I haven't looked into the actual test fail yet.diff --git a/configure.ac b/configure.ac
index d6414bb6..9a973042 100644
--- a/configure.ac
+++ b/configure.ac
@@ -519,18 +519,23 @@ AC_CHECK_HEADER([X11/extensions/Xrandr.h], [],
CFLAGS="$save_CFLAGS"
-# perform unit tests if libcheck found
+# perform unit tests if libcheck and libmocka found
+perform_unit_tests=yes ;# Assume packages will be found
if test "x$ensure_tests_deps" == "xyes"; then
PKG_CHECK_MODULES([CHECK], [check >= 0.10.0],
- [perform_unit_tests=yes],
+ [],
[AC_MSG_ERROR([please install check, the unit test framework])])
# Check if cmocka is available - needed for unit testing
PKG_CHECK_MODULES([CMOCKA], [cmocka],
- [have_cmocka=yes],
- [AC_MSG_WARN([please install cmocka, the mocking framework])])
+ [],
+ [AC_MSG_ERROR([please install cmocka, the mocking framework])])
else
+ perform_unit_tests=yes
PKG_CHECK_MODULES([CHECK], [check >= 0.10.0],
- [perform_unit_tests=yes],
+ [],
+ [perform_unit_tests=no])
+ PKG_CHECK_MODULES([CMOCKA], [cmocka],
+ [],
[perform_unit_tests=no])
fi
diff --git a/tests/xrdp/Makefile.am b/tests/xrdp/Makefile.am
index 3dad7972..1f830753 100644
--- a/tests/xrdp/Makefile.am
+++ b/tests/xrdp/Makefile.am
@@ -33,7 +33,8 @@ test_xrdp_SOURCES = \
test_xrdp_CFLAGS = \
-D IMAGEDIR=\"$(srcdir)\" \
- @CHECK_CFLAGS@
+ @CHECK_CFLAGS@ \
+ @CMOCKA_CFLAGS@
test_xrdp_LDFLAGS = -Wl,--wrap=pixman_region_extents, \
-Wl,--wrap=pixman_region_not_empty
@@ -63,5 +64,5 @@ test_xrdp_LDADD = \
$(top_builddir)/xrdp/xrdp_main_utils.o \
$(PIXMAN_LIBS) \
$(IMLIB2_LIBS) \
- $(CMOCKA_LIBS) \
- @CHECK_LIBS@
+ @CHECK_LIBS@ \
+ @CMOCKA_LIBS@
diff --git a/tests/xrdp/test_xrdp_region.c b/tests/xrdp/test_xrdp_region.c
index 86aeabad..4bd30d2e 100644
--- a/tests/xrdp/test_xrdp_region.c
+++ b/tests/xrdp/test_xrdp_region.c
@@ -47,7 +47,16 @@
#include <stddef.h>
#include <stdint.h>
#include <setjmp.h>
+#ifdef __cplusplus
+// cmocka.h lacks c++ guards for CI
+extern "C" {
#include <cmocka.h>
+}
+#define C_LINKAGE extern "C"
+#else
+#include <cmocka.h>
+#define C_LINKAGE
+#endif
#define UNUSED(x) (void)(x)
@@ -56,6 +65,7 @@ static pixman_box16_t test_box =
{
0, 0, 100, 100
};
+C_LINKAGE
pixman_box16_t *__wrap_pixman_region_extents(pixman_region16_t *region)
{
check_expected_ptr(region);
@@ -102,6 +112,7 @@ static void test_xrdp_region_get_bounds__happy_path(void **state)
g_free(rect);
}
+C_LINKAGE
pixman_bool_t __wrap_pixman_region_not_empty(pixman_region16_t *region)
{
check_expected_ptr(region);I think we need a good way to view "test-output.log" -- I could experiment with continually pushing my branch to get debug data, but that wouldn't be as good as seeing that log directly. I found this from another project that seems to do this:
Thanks for the feedback.
I can't comment on the test failing, except to say that it looks like _cmocka_run_group_tests() returns a failure count. I think you're right. Looking in the log file is probably the way forward here.
Figured out how to get the test log to be uploaded as an artifact. The error is here:
[ ERROR ] --- %s() has remaining non-returned values.
../../../../tests/xrdp/test_xrdp_region.c:84: note: remaining item was declared here
__wrap_pixman_region_extents: '%s' parameter still has values that haven't been checked.
../../../../tests/xrdp/test_xrdp_region.c:83: note: remaining item was declared hereI suspect the "extern C" is interfering with the "expect_any" and "will_return" directives. Might need to typedef the declaration of the function... Not sure. I'm in the twilight zone here.
Worse, if I add a "make check" step right before the distcheck, it passes!
I've just found that make distcheck on its own always uses the C compiler to build the check code, so that's why it's not failing on our rigs. I can make it fail with "CC=g++ make distcheck`
The problem seems to be that the wrapping isn't happening for the C++ compiler - the built-in functions are still being called, hence the craziness. I think we'll have to use the mangled names for the wrapped functions, so the extern "C" prefix for the wrapped functions won't work. I'm out of time today to look further.
After sleeping on it multiple times and trying everything I could think of, the fix was to add "----enable-pixman \" to AM_DISTCHECK_CONFIGURE_FLAGS. How can --wrap work if the original libraries aren't linked properly?
I'm not sure how this compiled or ran at all without pixman, when the tests tested a library without it.
But I think this needs to prompt a bigger discussion of what to do here -- If we're going to require unit tests for the distcheck, then that potentially means the build can't be "minimal" -- certain features will need to be flipped on in the distcheck that we may not desire. The better option might be to switch this out for an explicit "make check" step on every build workflow, and then not run unit tests as part of the distcheck (if that's even possible, my reading of what distcheck does also includes make check)
Regardless, this PR has become much more monstrous than I had intended. I'm going to split it up into smaller, more granular atomic pieces and post that list here later today.
${{ github.workspace }}/xrdp-0.9.80/_build/sub/tests/xrdp/test-suite.log which was fine for testing, but then I said "ok, how do we make that generic?" well in order to know that I'd need to be able to access the PACKAGE_VERSION number that's set in AC_INIT -- But I could not for the life of me figure out how to do that. The workaround I found that is "good enough" for now is to use a wildcard: xrdp-*, but there should be a way to pipe configure variables into github actions, right? This was the most promising github search I did to try to find out how: https://github.com/search?q=test-log+distcheck+extension%3A.yml&type=commits but most people either simply hard code the version and call it a day, or are using travis which seems to have more advanced abilities in this regard. A couple people are using the find command to do it, but that feels like overkill.