Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wizard recipe: msquic-v1.4.0 #3364

Closed
wants to merge 5 commits into from
Closed

Wizard recipe: msquic-v1.4.0 #3364

wants to merge 5 commits into from

Conversation

hros
Copy link
Contributor

@hros hros commented Jul 21, 2021

This pull request contains a new build recipe I built using the BinaryBuilder.jl wizard:

  • Package name: msquic
  • Version: v1.4.0

@staticfloat please review and merge.

Copy link
Member

@giordano giordano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, an overall comment: this needs to be greatly simplified. We want to have each library into a single package. We actually already have most of the libraries you're building here, so you can use them as dependencies. Also, no need to build CMake as we already have it

@hros
Copy link
Contributor Author

hros commented Jul 22, 2021

Ok, an overall comment: this needs to be greatly simplified. We want to have each library into a single package. We actually already have most of the libraries you're building here, so you can use them as dependencies. Also, no need to build CMake as we already have it

thanks for looking at the pr.

openssl was added as a dependency to build the latest cmake (3.21)
the compilation problem was also present with the default cmake (3.17?), so removing both (openssl, and cmake) should make a difference.

The required dependencies are: liblttng-ust-dev, lttng-tools (popt, libxml2, userspace, numactl are their sub-dependencies)
As far as I can tell lttng-* are not available as jll packages, however some of their sub-dependencies are available. namely, popt, xml2, numa.
It is definitely better to use the available jll packages, but since the packages were successfuly built with no errors, do you think switching to the jll dependencies would make a difference in the build problems?

btw, please remind me what is the proper way to update a pr on Yggdrasil? since the git commands were all executed by the BinaryBuilder wizard?

@giordano
Copy link
Member

It is definitely better to use the available jll packages, but since the packages were successfuly built with no errors, do you think switching to the jll dependencies would make a difference in the build problems?

No, it shouldn't

btw, please remind me what is the proper way to update a pr on Yggdrasil? since the git commands were all executed by the BinaryBuilder wizard?

You'll need to clone your fork, check out the branch wizard/msquic-v1.4.0_fc161a0e:

git clone --branch wizard/msquic-v1.4.0_fc161a0e https://github.com/hros/Yggdrasil

Then you can do the edits you want, stage and commit them, and then push the changes

git add <files edited>
git commit
git push -u origin

and this pull request will be automatically updated because you'd be pushing to the same branch

@hros
Copy link
Contributor Author

hros commented Jul 24, 2021

according to the suggestion, I've used binary jll packages as dependencies
as expected this did not fix the compilation issues
please let me know if you have an idea regarding the build errors

I am following MsQuic's build instructions which work flawlessly on my Ubuntu-20.04

M/msquic/build_tarballs.jl Outdated Show resolved Hide resolved
M/msquic/build_tarballs.jl Outdated Show resolved Hide resolved
M/msquic/build_tarballs.jl Outdated Show resolved Hide resolved
M/msquic/bundled/patches/msquic.patch Outdated Show resolved Hide resolved
@giordano
Copy link
Member

giordano commented Jul 28, 2021

Error on x86_64-linux-gnu:

[21:28:57] [ 44%] Building C object src/core/CMakeFiles/core.dir/version_neg.c.o
[21:28:57] In file included from /workspace/srcdir/msquic/src/inc/quic_platform.h:163:0,
[21:28:57]                  from /workspace/srcdir/msquic/src/core/precomp.h:25,
[21:28:57]                  from /workspace/srcdir/msquic/src/core/ack_tracker.c:40:
[21:28:57] /workspace/srcdir/msquic/src/inc/quic_crypt.h:229:5: error: expected declaration specifiers or ‘...’ before ‘sizeof’
[21:28:57]      sizeof(uint64_t) < CXPLAT_IV_LENGTH,
[21:28:57]      ^

Code:

CXPLAT_STATIC_ASSERT(
    sizeof(uint64_t) < CXPLAT_IV_LENGTH,
    "Packet Number Length is less than IV Length");

I'm not really sure what's going on there


Error on aarch64-linux-gnu:

[21:28:57] [ 45%] Built target perflib
[21:28:57] /workspace/srcdir/msquic/src/core/frame.c: In function ‘QuicFrameLog’:
[21:28:57] /workspace/srcdir/msquic/src/core/frame.c:1742:9: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
[21:28:57]          QuicTraceLogVerbose(
[21:28:57]          ^~~~~~~~~~~~~~~~~~~
[21:28:57] /workspace/srcdir/msquic/src/core/frame.c:1764:9: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
[21:28:57]          QuicTraceLogVerbose(
[21:28:57]          ^~~~~~~~~~~~~~~~~~~
[21:28:57] cc1: all warnings being treated as errors

@hros
Copy link
Contributor Author

hros commented Jul 31, 2021

Error on x86_64-linux-gnu:

[21:28:57] [ 44%] Building C object src/core/CMakeFiles/core.dir/version_neg.c.o
[21:28:57] In file included from /workspace/srcdir/msquic/src/inc/quic_platform.h:163:0,
[21:28:57]                  from /workspace/srcdir/msquic/src/core/precomp.h:25,
[21:28:57]                  from /workspace/srcdir/msquic/src/core/ack_tracker.c:40:
[21:28:57] /workspace/srcdir/msquic/src/inc/quic_crypt.h:229:5: error: expected declaration specifiers or ‘...’ before ‘sizeof’
[21:28:57]      sizeof(uint64_t) < CXPLAT_IV_LENGTH,
[21:28:57]      ^

Code:

CXPLAT_STATIC_ASSERT(
    sizeof(uint64_t) < CXPLAT_IV_LENGTH,
    "Packet Number Length is less than IV Length");

I'm not really sure what's going on there

The offending line is static_assert(sizeof(uint64_t)<12);

I believe I have found a problem, the compiler being used is: /opt/bin/x86_64-linux-gnu-libgfortran4-cxx11/x86_64-linux-gnu-gcc, and if I change it to /opt/bin/x86_64-linux-gnu-libgfortran4-cxx11/x86_64-linux-gnu-g++ then I can compile without errors (at least the first few files)

however if I try and add -DCMAKE_C_COMPILER=/opt/x86_64-linux-gnu/bin/x86_64-linux-gnu-g++ to the cmake generation command, I get ther following error: # error "The CMAKE_C_COMPILER is set to a C++ compiler"

the files use a .c suffix instead of .cc, .cpp which are usually used for C++ which could be the cause of the problem
however in the "native" mode (not in the BinaryBuilder sandbox) the compiler being used is /usr/bin/cc which is symlinked to gcc and not g++ and this compiler works (note it is gcc version 9, whereas in the sandbox version 7 is being used)

after further investigation, the project uses an #ifdef clang to declare the macro as _Static assert when compiling for windows, but does not perform the same check in the posix platforms

@giordano
Copy link
Member

giordano commented Aug 1, 2021

Using the C++ compiler to compile a C source code doesn't sound like a good idea

@giordano
Copy link
Member

giordano commented Aug 1, 2021

Also, I get the same errors with GCC 9.1.0, so the version of the compiler is unlikely to be the main issue here.

@hros
Copy link
Contributor Author

hros commented Aug 5, 2021

Also, I get the same errors with GCC 9.1.0, so the version of the compiler is unlikely to be the main issue here.

Thank you so much for making the effort and testing different compilers
I have also tried that, and it did not help
As I said, the problem seems to be that an assertion macro is defined as static_assert while for .c files one should use _Static_assert instead.
Indeed the msquic project redefines that macro for the windows platform in quic_platform_winuser.h as _Static_assert, but does not redefine it in the linux include file quic_platform_posix.h.
I compiled the offending file (ack_tracker.c) in Ubuntu-20.04 with -E and I saw that the macro was being expanded to Static_assert.
I finally figured out that the problem is in the system include file: assert.h. Ubuntu (and I guess all modern distributions) have a file which is copyrighted (c) 1991-2020 by the FSF and the file ends with the following:

#if defined __USE_ISOC11 && !defined __cplusplus
# undef static_assert
# define static_assert _Static_assert
#endif

The assert.h included in the BinaryBuilder sandbox (/opt/x86_64-linux-gnu/x86_64-linux-gnu/sys-root/usr/include/assert.h) is copyrighted "Copyright (C) 1991,1992,1994-2001,2003,2004,2007", and does not contain the lines to enable using static_assert in C files.

In order to compile the file, I could either:

  1. override the assert.h file
  2. change the macro definition for linux/posix (hopefully this would not require revision in future versions of the msquic)
  3. wait for the maintainers of BinaryBuiler to update the include file

@giordano
Copy link
Member

giordano commented Aug 6, 2021

Now compilation fails for me at

[  1%] Building C object src/core/CMakeFiles/core.dir/frame.c.o
cd /workspace/srcdir/msquic/build/src/core && /opt/bin/x86_64-linux-gnu-libgfortran4-cxx11/x86_64-linux-gnu-gcc --sysroot=/opt/x86_64-linux-gnu/x86_64-linux-gnu/sys-root/ -DCX_PLATFORM_LINUX -DQUIC_DISABLE_CLIENT_CERT_TESTS -DQUIC_DISABLE_DEFERRED_CERT_TESTS -DQUIC_EVENTS_STUB -DQUIC_LOGS_STUB -DQUIC_SHARED_EPHEMERAL_WORKAROUND -DQUIC_TEST_OPENSSL_FLAGS=1 -DVER_BUILD_ID=0 -DVER_SUFFIX=-private -D_GNU_SOURCE -I/workspace/srcdir/msquic/src/inc  -Dstatic_assert=_Static_assert -O3  -DNDEBUG   -fms-extensions -fPIC -pthread -Werror -Wall -Wextra -Wformat=2 -Wno-type-limits -Wno-unknown-pragmas -Wno-multichar -Wno-missing-field-initializers -o CMakeFiles/core.dir/frame.c.o   -c /workspace/srcdir/msquic/src/core/frame.c
/workspace/srcdir/msquic/src/core/frame.c: In function ‘QuicFrameLog’:
/workspace/srcdir/msquic/src/core/frame.c:1742:9: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
         QuicTraceLogVerbose(
         ^~~~~~~~~~~~~~~~~~~
/workspace/srcdir/msquic/src/core/frame.c:1764:9: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
         QuicTraceLogVerbose(
         ^~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[2]: *** [src/core/CMakeFiles/core.dir/build.make:200: src/core/CMakeFiles/core.dir/frame.c.o] Error 1
make[2]: Leaving directory '/workspace/srcdir/msquic/build'
make[1]: *** [CMakeFiles/Makefile2:692: src/core/CMakeFiles/core.dir/all] Error 2
make[1]: Leaving directory '/workspace/srcdir/msquic/build'
make: *** [Makefile:161: all] Error 2

@hros
Copy link
Contributor Author

hros commented Aug 7, 2021

After applying export CFLAGS="-Dstatic_assert=_Static_assert" the build compiles a large portion of the the project but fails with errors about a missing function: sendmmsg
the build script is currently:

cd msquic
git submodule update --init --recursive
mkdir build && cd build
export CFLAGS="-Dstatic_assert=_Static_assert"
cmake -DCMAKE_INSTALL_PREFIX=$prefix -DCMAKE_TOOLCHAIN_FILE=${CMAKE_TARGET_TOOLCHAIN} -DCMAKE_BUILD_TYPE=Release -G 'Unix Makefiles' ..
cmake --build .

The build fails when compiling src/platform/datapath_epoll.c. Line 2554 is:

sendmmsg(
                SocketContext->SocketFd,
                Mhdrs + SendData->SentMessagesCount,
                (unsigned int)(TotalMessagesCount - SendData->SentMessagesCount),
                0);

and sendmmsg is not defined.

Like the case of static_assert there is a difference between the headers in the BinaryBuilder sandbox and the Linux (Ubuntu 20.04) headers. This time, the header is sys/socket.h.
In the Linux version, the header contains the lines:

#ifdef __USE_GNU
/* For `recvmmsg' and `sendmmsg'.  */
struct mmsghdr
  {
    struct msghdr msg_hdr;      /* Actual message header.  */
    unsigned int msg_len;       /* Number of received or sent bytes for the
                                   entry.  */
  };
#endif


#ifdef __USE_GNU
/* Send a VLEN messages as described by VMESSAGES to socket FD.
   Returns the number of datagrams successfully written or -1 for errors.

   This function is a cancellation point and therefore not marked with
   __THROW.  */
extern int sendmmsg (int __fd, struct mmsghdr *__vmessages,
                     unsigned int __vlen, int __flags);
#endif

which are missing from the sandbox version of sys/socket.h

Moreover sendmmsg is also missing from the libc library in the sandbox. it is present in Ububntu's libc as well as the libc sources (link).

@giordano
Copy link
Member

giordano commented Aug 8, 2021

For context, we use quite old versions of glibc (the specific versions depend on the architectures): https://github.com/JuliaPackaging/Yggdrasil/blob/f3fdd76e3fcb3081d40d46ef79536b294217af0a/RootFS.md#glibc-versions. This is because these are the minimum versions of glibc required to run Julia itself. I guess this package implicitly requires a much newer version of glibc

@hros
Copy link
Contributor Author

hros commented Aug 8, 2021

For context, we use quite old versions of glibc (the specific versions depend on the architectures): https://github.com/JuliaPackaging/Yggdrasil/blob/f3fdd76e3fcb3081d40d46ef79536b294217af0a/RootFS.md#glibc-versions. This is because these are the minimum versions of glibc required to run Julia itself. I guess this package implicitly requires a much newer version of glibc

What are the options going forward regarding the glibc version gap?
Is it possible to base this library on a newer version of glibc?
Otherwise, is it possible to add the missing feature (i.e. starting with sendmmsg.c and hopefully getting all dependencies) as an additional external library for this project

@giordano
Copy link
Member

giordano commented Aug 9, 2021

Is it possible to base this library on a newer version of glibc?

Not really: the compilers we provide need to be built against a specific libc version, it isn't that easy to ask for a different libc later. You're basically facing JuliaPackaging/BinaryBuilder.jl#863.

@hros
Copy link
Contributor Author

hros commented Aug 9, 2021

Is it possible to base this library on a newer version of glibc?

Not really: the compilers we provide need to be built against a specific libc version, it isn't that easy to ask for a different libc later. You're basically facing JuliaPackaging/BinaryBuilder.jl#863.

I see....

I contemplated using the definition in sysdeps/unix/sysv/linux/sendmmsg.c of glibc and call the kernel implementation directly with syscall. But that seems rather risky considering I do not know if that function is implemented in the target kernel machine.

I opened an issue with the msquic project asking if sendmmsg could be removed/replaced
note the discussion:

Right now now sendmmsg is a requirement. Not something that couldn't be fixed, but glibc 2.14 (which was the first release to have sendmmsg) is 10 years old at this point, so we would need to prioritize it appropriately.
One other thing to add is that with older glibc versions you won't have GSO support either. So both features combined missing will result in the largest performance drop, as you only will be able to send a single UDP packet at a time.

perhaps it is time to upgrade to 2.14?

@hros
Copy link
Contributor Author

hros commented Aug 11, 2021

The msquic team have added a patch that enables compilation without sendmmsg.

However, that would create a huge performance degradation as described in the comment

So, while the project might compile in BinaryBuilder, I see no reason to do so if the end result would have such poor performance. I am looking to add a Julia package that would use the msquic jll package and deliver high throughput QUIC protocol communication with an interface similar to Julia's TCP communication

Please let me know if

  1. I can use a recent glib version (>2.14) with BinaryBuilder
  2. Build a "jll" package without BinaryBuilder with a higher glib version (will that work with the rest of Julia)?

@giordano
Copy link
Member

  1. not really possible at the moment, maybe in the future but we have no timeline
  2. you're free to make libraries available to the users however you want. JLLs are just a convenient way which is well integrated with the rest of the ecosystem: they're regular Julia packages (really, they are no special) which automatically download the needed library upon installation, other packages can depend on them, so they also appear in Project.toml/Manifest.toml, since they're plain Julia packages they're also precompilation-friendly, etc... However, if this system doesn't work for you (for example because building requirements are higher than what provided by BinaryBuilder) you can look for other options.

@imciner2
Copy link
Member

imciner2 commented Apr 3, 2024

Closing due to inactivity. Please submit a new PR on top of the current master branch if you still want to include this recipe.

@imciner2 imciner2 closed this Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants