mas-bandwidth / yojimbo

A network library for client/server games written in C++
BSD 3-Clause "New" or "Revised" License
2.48k stars 244 forks source link

Working on improving the vcpkg integration: libsodium and mbedtls versions? #177

Closed McKillroy closed 10 months ago

McKillroy commented 2 years ago

This is a question for @gafferongames concerning the use of libsodium and mbedtls: I am currently working on improving the vcpkg and CMake integration and would like to allow the use of vcpkg's manifest mode together with a custom vcpkg registry. What I need to know, which are the latest versions of the two packages that are known to work with yojimbo. Thanks ~Chris

gafferongames commented 2 years ago

I don't remember. :)

kingoftheconnors commented 2 years ago

Hey! I've actually been working on the same thing for the past week! I got it to work with just the most current versions of the packages: Libsodium 1.0.18 and mbedtls 2.24.0!

McKillroy commented 2 years ago

@kingoftheconnors @gafferongames mbedtls is at version 3.1.0 already. Is this supposed to work? It has better CMake support. https://github.com/Mbed-TLS/mbedtls - see tags. Also see: https://github.com/Mbed-TLS/mbedtls/blob/v3.1.0/docs/3.0-migration-guide.md

kingoftheconnors commented 2 years ago

@McKillroy Thanks for the great question! I'm still learning all this myself.

I'm pretty sure vcpkg gets its package version and port info its registered package list at https://vcpkg.io/en/packages.html, which is still at mbedtls version 2.24.0. I'm not sure how to update it to the most recent version outside of installing mbedtls normally.

So, I haven't tested yojimbo with 3.1.0. I have no idea whether it would still work or not.

McKillroy commented 2 years ago

I'm currently using a modified port for mbedtls which uses 2.24.0 and creates an mbedtlsConfig.cmake file. But 2.24.0 is kinda outdated. I'm also struggleing with getting transitive dependencies working with vcpkg manifest mode and custom registries (I'm using a custom vcpkg registry, since otherwise it's all even more of a royal pain). Learning a lot here too.

If we could use the latest mbedtls (3.1.0) I'd be motivated to make a custom port for that - shouldn't be too hard because of the improved cmake support.

yojimbo_user -> yojimbo (manifest) -> unofficial-sodium, mbedtls (custom port with manifest)

for mbedtls I'm using this Config in a custom mbedtls port in a custom registry:


message("****** mbedtlsConfig - START - ******")

############################################################
### Compute the installation prefix relative to this file.

    get_filename_component(_IMPORT_PREFIX "${CMAKE_CURRENT_LIST_FILE}" PATH)
    get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
    get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
    if(_IMPORT_PREFIX STREQUAL "/")
      set(_IMPORT_PREFIX "")
    endif()
    MESSAGE("_IMPORT_PREFIX = ${_IMPORT_PREFIX}")

  ## External libraries and headers

  add_library( mbedtls STATIC IMPORTED )  
  target_include_directories( mbedtls INTERFACE "${_IMPORT_PREFIX}/include"  )

  add_library( mbedx509 STATIC IMPORTED )  
  target_include_directories( mbedx509 INTERFACE "${_IMPORT_PREFIX}/include"  )

  add_library( mbedcrypto STATIC IMPORTED )  
  target_include_directories( mbedcrypto INTERFACE "${_IMPORT_PREFIX}/include"  )

  ## External library binaries
  if(WIN32)
    if(${CMAKE_BUILD_TYPE}_ STREQUAL "Debug_")
        set_target_properties(mbedtls    PROPERTIES IMPORTED_LOCATION ${_IMPORT_PREFIX}/debug/lib/mbedtls.lib     )
        set_target_properties(mbedx509   PROPERTIES IMPORTED_LOCATION ${_IMPORT_PREFIX}/debug/lib/mbedx509.lib    )
        set_target_properties(mbedcrypto PROPERTIES IMPORTED_LOCATION ${_IMPORT_PREFIX}/debug/lib/mbedcrypto.lib  )
    else()
        set_target_properties(mbedtls    PROPERTIES IMPORTED_LOCATION ${_IMPORT_PREFIX}/lib/mbedtls.lib     )
        set_target_properties(mbedx509   PROPERTIES IMPORTED_LOCATION ${_IMPORT_PREFIX}/lib/mbedx509.lib    )
        set_target_properties(mbedcrypto PROPERTIES IMPORTED_LOCATION ${_IMPORT_PREFIX}/lib/mbedcrypto.lib  )
    endif()
  ELSEIF(UNIX)
    if(${CMAKE_BUILD_TYPE}_ STREQUAL "Debug_")
        set_target_properties(mbedtls    PROPERTIES IMPORTED_LOCATION ${_IMPORT_PREFIX}/debug/lib/libmbedtls.a    )
        set_target_properties(mbedx509   PROPERTIES IMPORTED_LOCATION ${_IMPORT_PREFIX}/debug/lib/libmbedx509.a   )
        set_target_properties(mbedcrypto PROPERTIES IMPORTED_LOCATION ${_IMPORT_PREFIX}/debug/lib/libmbedcrypto.a )
    else()
        set_target_properties(mbedtls    PROPERTIES IMPORTED_LOCATION ${_IMPORT_PREFIX}/lib/libmbedtls.a    )
        set_target_properties(mbedx509   PROPERTIES IMPORTED_LOCATION ${_IMPORT_PREFIX}/lib/libmbedx509.a   )
        set_target_properties(mbedcrypto PROPERTIES IMPORTED_LOCATION ${_IMPORT_PREFIX}/lib/libmbedcrypto.a )
    endif()
  ENDIF()

message("****** mbedtlsConfig - DONE- ******")```
kingoftheconnors commented 2 years ago

I'm also using a custom manifest; specifically the one found in this repo's vcpkg file. The only difference I can see is the version I used defined the VCPKG_MBEDTLS_BASE variable:

if(NOT DEFINED VCPKG_ROOT_DIR)
    message(FATAL_ERROR "VCPKG_ROOT_DIR is not defined. Please set it to the root folder of your vcpkg installation.")
endif()

############################################################
## External imports: mbedtls
############################################################
##
    ## TODO: 
    ##      -> 32 bit ???
    ##      -> Dynamic linking setup ???

    # External library paths
  if(WIN32)
    SET(VCPKG_MBEDTLS_BASE   "${VCPKG_ROOT_DIR}/packages/mbedtls_x64-windows-static" )
  elseif(UNIX)
    SET(VCPKG_MBEDTLS_BASE   "${VCPKG_ROOT_DIR}/installed/x64-linux")
  else()
    MESSAGE(FATAL "Unrecognized Operating System. I understand either WIN32 or UNIX to be set to true.")
  endif()

  ## External libraries and headers

  add_library( mbedtls STATIC IMPORTED )  
  target_include_directories( mbedtls INTERFACE "${VCPKG_MBEDTLS_BASE}/include"  )

  add_library( mbedx509 STATIC IMPORTED )  
  target_include_directories( mbedx509 INTERFACE "${VCPKG_MBEDTLS_BASE}/include"  )

  add_library( mbedcrypto STATIC IMPORTED )  
  target_include_directories( mbedcrypto INTERFACE "${VCPKG_MBEDTLS_BASE}/include"  )

  ## External library binaries
  if(WIN32)
    if(${CMAKE_BUILD_TYPE}_ STREQUAL "Debug_")
        set_target_properties(mbedtls    PROPERTIES IMPORTED_LOCATION ${VCPKG_MBEDTLS_BASE}/debug/lib/mbedtls.lib     )
        set_target_properties(mbedx509   PROPERTIES IMPORTED_LOCATION ${VCPKG_MBEDTLS_BASE}/debug/lib/mbedx509.lib    )
        set_target_properties(mbedcrypto PROPERTIES IMPORTED_LOCATION ${VCPKG_MBEDTLS_BASE}/debug/lib/mbedcrypto.lib  )
    else()
        set_target_properties(mbedtls    PROPERTIES IMPORTED_LOCATION ${VCPKG_MBEDTLS_BASE}/lib/mbedtls.lib     )
        set_target_properties(mbedx509   PROPERTIES IMPORTED_LOCATION ${VCPKG_MBEDTLS_BASE}/lib/mbedx509.lib    )
        set_target_properties(mbedcrypto PROPERTIES IMPORTED_LOCATION ${VCPKG_MBEDTLS_BASE}/lib/mbedcrypto.lib  )
    endif()
  ELSEIF(UNIX)
    if(${CMAKE_BUILD_TYPE}_ STREQUAL "Debug_")
        set_target_properties(mbedtls    PROPERTIES IMPORTED_LOCATION ${VCPKG_MBEDTLS_BASE}/debug/lib/libmbedtls.a    )
        set_target_properties(mbedx509   PROPERTIES IMPORTED_LOCATION ${VCPKG_MBEDTLS_BASE}/debug/lib/libmbedx509.a   )
        set_target_properties(mbedcrypto PROPERTIES IMPORTED_LOCATION ${VCPKG_MBEDTLS_BASE}/debug/lib/libmbedcrypto.a )
    else()
        set_target_properties(mbedtls    PROPERTIES IMPORTED_LOCATION ${VCPKG_MBEDTLS_BASE}/lib/libmbedtls.a    )
        set_target_properties(mbedx509   PROPERTIES IMPORTED_LOCATION ${VCPKG_MBEDTLS_BASE}/lib/libmbedx509.a   )
        set_target_properties(mbedcrypto PROPERTIES IMPORTED_LOCATION ${VCPKG_MBEDTLS_BASE}/lib/libmbedcrypto.a )
    endif()
  ENDIF()

Do you think you could just subtitute the values for the properties and directories to a local installation to update mbedtls?

McKillroy commented 2 years ago

@kingoftheconnors Its my PR - that's why it's basically the same. An imported target like above can point anywhere, so - yes- you could use any local installation, assuming the triplet/version of it work.

I'm in the moment fighting the lack of transitivity. I made a vcpkg/CMake project using yojimbo and still have to declare libsodium and mbedtls explicitly at the top level. In the moment I believe it has to do with mbedtls and libsodium not fully supporting clients via their cmake setup.

mbedtls 3.1.0 at least defines public dependencies and an install interface, which is not the case in 2.28.0. Just compare these two CMakeLists.txt for 2.28.0 and 3.1.0 to see the difference: Old 2.28.0: https://github.com/Mbed-TLS/mbedtls/blob/v2.28.0/library/CMakeLists.txt New 3.1.0: https://github.com/Mbed-TLS/mbedtls/blob/v3.1.0/library/CMakeLists.txt

Depending on which version works with Yojimbo the build of a convenient cmake/vcpkg based build system for Yojimbo will be different.

For libsodium it's even worse: There is no CMake support at all - the vcpkg port provides one: https://github.com/microsoft/vcpkg/blob/master/ports/libsodium/CMakeLists.txt And that's outdated CMake style as well.

So - as a result we have to fix it ourselves (See my suggestion below).

Independently from the build issues, I think Yojimbo should be updated to mbedtls 3.1.0 for modernization and safety reasons See mbedtls notes on the step up to 3.1.0 here: https://github.com/Mbed-TLS/mbedtls/blob/v3.1.0/docs/3.0-migration-guide.md

So my suggestion is: Update Yojimbo to use mbedtls 3.1.0 and just build and find it with Cmakes find_package conventionally. This part affects @gafferongames or anyone familiar with the guts of Yojimbo. For libsodium we could just use the vcpkg port and write a custom FindSodium.cmake

Still learning a lot myself here. On the long run using vcpkg/cmake is unbeatable concerning build-comfort, no matter how much we might hate Cmake ;)

McKillroy commented 2 years ago

I hope I could solve some issues with this PR: #4d13cc2 Please check this out and test, so I can fix any possible issues thast might not have happened on my side. I want Yojimbo to be as easy to build and integrate as possible.

gafferongames commented 10 months ago

I've removed the dependency on mbedtls and sodium