Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
    Derek Schrock
    @derekschrock
    So as ssl_get_error man page says errno 0 and SSL_ERROR_SYSCALL is an EOF (peer disconnected). So that just the client disconnecting. Guessing MS RDP macOS and mstsc are just getting hung up on some application level (caps) thing.
    rdesktop has some interesting warning and error message. But at least there's a little more info here. I still want to wait for an update from llvm-devel.
    Nexarian
    @Nexarian

    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.

    metalefty
    @metalefty
    Need to be released as v0.9.x to get the feature in CentOS. I'm happy to help making a release if you create backport PR to v0.9 branch.
    Nexarian
    @Nexarian
    They're fine with building from source I think.
    But I can definitely do that too once I have it working.
    Derek Schrock
    @derekschrock
    Looks like all my llvm16 issues are with this. https://reviews.llvm.org/D139582
    Reverted a day later. bisecting seems to tell this too.
    So no UDB issues here (yet).
    Derek Schrock
    @derekschrock
    Now a couple weeks later (last week) that same feature was tried again... see how that turns out.
    Nexarian
    @Nexarian
    I need some help with integrating cmocka into XRDP. All GitHub actions pass on this PR except one, and I cannot for the life of me understand why: neutrinolabs/xrdp#2524 -- For some reason cmocka isn't being linked into the distcheck, in spite of it being explicitly specified in the makefile.
    Derek Schrock
    @derekschrock
    You need an something like AM_LDFLAGS = @uniq@ for the -Lpath and -llibname ?
    Does that even resolve to anything? $CMOCKA_CFLAGS?
    Derek Schrock
    @derekschrock
    I guess you get it from PKG_CHECK_MODULES that woudl also add CMOCKA_LIBS.
    Or LDADD?
    oh wait.. you have in the in LDADD. nm.
    Derek Schrock
    @derekschrock
    er... Maybe it is -Lpath that's missing
    Nexarian
    @Nexarian
    -Lpath?
    Derek Schrock
    @derekschrock
    Add a path to find libs.
    I'm guessing it doesn't have -L/usr/local/lib since it's (default) FreeBSD
    Nexarian
    @Nexarian
    Where would I add that?
    Derek Schrock
    @derekschrock
    er wait.. this is just when building with c++?
    Derek Schrock
    @derekschrock
    Seems like it should all be there...
    Nexarian
    @Nexarian
    It's the distcheck
    Derek Schrock
    @derekschrock
    Does it work without the -j?
    Nexarian
    @Nexarian
    Yes
    that was me just trying to improve iteration time
    Nexarian
    @Nexarian
    Feel free to play with it...
    Nexarian
    @Nexarian
    We should keep the J
    I'll keep working on it of course. Getting cmocka to work at all was a little difficult as I didn't understand the framework yet.
    But if I crack this, we'll open a whole new ability for unit testing.
    Gyuhwan Park★
    @unstabler

    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?

    matt335672
    @matt335672

    @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, 100

    There 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.

    matt335672
    @matt335672

    @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

    Gyuhwan Park★
    @unstabler
    Thanks for your help!
    matt335672
    @matt335672
    @Nexarian - I've improved my patch slightly (arguably), so there's only one #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);
    Nexarian
    @Nexarian
    @matt335672 Wow! Thanks for the help, it would have taken me forever to figure out the C linkage error. Strangely, the tests are passing on my system but not in CI, similar to your system. My guess is I have a dependency installed that allows this to work whereas other systems don't.
    Nexarian
    @Nexarian

    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:

    alltilla/syslog-ng@ae70ed8

    Hmm we should also add names to all our steps.
    This cmocka integration is really helping us figure out how to improve our CI and unit testing infra!
    Nexarian
    @Nexarian
    Anyway I'm out of time for this tonight. I'll try again tomorrow1
    matt335672
    @matt335672

    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.

    Nexarian
    @Nexarian

    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 here

    I 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!

    matt335672
    @matt335672

    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.

    Nexarian
    @Nexarian
    Weirdly it doesn't fail on my box when I do CC=g++ make distcheck -- I now wonder if I have a newer version of g++
    Nexarian
    @Nexarian

    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.

    Nexarian
    @Nexarian
    Another struggle I had was figuring out how to upload the distcheck logs as an artifact in a generic way. The original PR uploaded this path from the github actions: ${{ 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.
    Anyway, added the wildcard to the PR for now.
    Nexarian
    @Nexarian
    Ok, please take a look at the following PRs. I've split them up into smaller granular chunks that should be easier to review individually. I haven't actually added the xrdp_region functions nor the unit tests yet, those PRs will come after this sequence of the initial 4 is checked in: