Attention is currently required from: laforge, neels, pespin.

View Change

5 comments:

• File configure.ac:

  • Patch Set #16, Line 259: AM_CONDITIONAL(ENABLE_TUN, test "x$embedded" != "xyes" && test "x$emscripten" != "xyes")

    This will still probably need fixing in a separate patch like the NETNS stuff you fixed

    Agreed, this likely needs an additional fix. This should follow the same
    approach as netns. I’ll address this in a follow-up patch.

• File src/core/Makefile.am:

  • Patch Set #16, Line 87: if ENABLE_TUN

    This will probably need some sort of other fix? Why can't you add it when building for emscripten?

    Agreed, this likely needs an additional fix. This should follow the same
    approach as netns. I’ll address this in a follow-up patch.

• File src/core/osmo_io_internal.h:

  • Patch Set #16, Line 5: #include "../config.h"

    This and the change below deserve a separate patch, feel free to submit previously in the branch to […]

    Done, split out and submitted as a separate patch.

• File src/core/stats_tcp.c:

  • Patch Set #16, Line 198: #if !defined(__EMSCRIPTEN__)

    This should be fixed in a more generic way. […]

    The build fails due to missing Linux-specific headers/types.

    First, without any guards:
      stats_tcp.c:33:10: fatal error: 'linux/tcp.h' file not found
        #include <linux/tcp.h>

    If I try to workaround this by guarding the include with `#if defined(__linux__)`,
    the build still fails because the code below relies on Linux-only types/macros:
      stats_tcp.c:103:18: error: variable has incomplete type 'struct tcp_info'
      stats_tcp.c:112:56: error: use of undeclared identifier 'TCP_INFO'

    So this can’t be fixed just by wrapping the include. We need a generic
    configure-time feature/header check (e.g. for linux/tcp.h and/or the presence
    of struct tcp_info / TCP_INFO) and conditionally build/enable the TCP_INFO
    stats code based on that.

    I’ll address this in a separate patch.

• File src/vty/logging_vty.c:

  • Patch Set #16, Line 937: #if !defined(__EMSCRIPTEN__)

    what about leaving this in and let it fail during runtime if user tries to use it?

    Agreed. I disabled it only to avoid creating files on the Web side, but since
    Emscripten provides an IndexedDB-backed filesystem, leaving it enabled and
    failing at runtime is fine. I’ll update the patch

To view, visit change 41813. To unsubscribe, or for help writing mail filters, visit settings.