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

RFC: Provide install script #4

Merged
merged 3 commits into from
Jul 13, 2022
Merged

RFC: Provide install script #4

merged 3 commits into from
Jul 13, 2022

Conversation

frabert
Copy link
Contributor

@frabert frabert commented Jul 12, 2022

Multiplier is using weggli-native in a CMake project via Corrosion, which works well enough as for now.
The issue is that if Multiplier is used as a library and weggli-native is exposed as a transitive dependency, everything breaks.

I created a script that can be used to install the library and the generated header to a chosen directory, together with a CMake config module that allows users to find the library by doing find_package(weggli_native).

I'm open to hearing different approaches

@CLAassistant
Copy link

CLAassistant commented Jul 12, 2022

CLA assistant check
All committers have signed the CLA.

@woodruffw woodruffw self-assigned this Jul 12, 2022
@woodruffw
Copy link
Member

Hmm. I'm generally not a huge fan of opaque install scripts -- perhaps we could just add the CMake build and config module directly, and have it invoke the Rust build + provide the right installation targets?

@frabert
Copy link
Contributor Author

frabert commented Jul 12, 2022

That sounds interesting, but I'm not sure how exactly it would work. Can you give me more details about what you have in mind?

@woodruffw
Copy link
Member

woodruffw commented Jul 12, 2022

Can you give me more details about what you have in mind?

I had something like uthenticode's CMake build in mind: https://github.com/trailofbits/uthenticode

In particular, we use CMake's install and export directives to copy everything, and to auto-generate a CMake user config:

install(
  TARGETS "${PROJECT_NAME}"
  EXPORT uthenticode-targets
  RUNTIME DESTINATION "bin"
  LIBRARY DESTINATION "lib"
  ARCHIVE DESTINATION "lib"
  PUBLIC_HEADER DESTINATION "include"
)
export(
  TARGETS "${PROJECT_NAME}"
  NAMESPACE uthenticode::
  FILE "${CMAKE_CURRENT_BINARY_DIR}/uthenticode-targets.cmake"
)
install(
  EXPORT uthenticode-targets
  DESTINATION "lib/cmake/uthenticode"
  NAMESPACE uthenticode::
  EXPORT_LINK_INTERFACE_LIBRARIES
)
install(FILES uthenticode-config.cmake DESTINATION "lib/cmake/uthenticode")

@frabert
Copy link
Contributor Author

frabert commented Jul 12, 2022

Ok so here is how far I was able to get:

cmake_minimum_required(VERSION 3.19)

project(weggli_native)

if(NOT ((CMAKE_BUILD_TYPE STREQUAL "Release") OR (CMAKE_BUILD_TYPE STREQUAL "Debug")))
    message(FATAL_ERROR "Only Debug and Release builds are supported")
endif()

find_program(CARGO "cargo" REQUIRED)
find_program(CBINDGEN "cbindgen" REQUIRED)

if(CMAKE_BUILD_TYPE STREQUAL "Release")
    set(CARGO_PROFILE "release")
else()
    set(CARGO_PROFILE "dev")
endif()

string(TOLOWER "${CMAKE_BUILD_TYPE}" TARGET_DIR)

add_custom_target(cargo_build
    COMMAND "${CARGO}" build --profile "${CARGO_PROFILE}"
    WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}")

add_custom_command(
    OUTPUT "${CMAKE_BINARY_DIR}/weggli.h"
    COMMAND "${CBINDGEN}" ARGS -o "${CMAKE_BINARY_DIR}/weggli.h"
    MAIN_DEPENDENCY cargo_build)

add_library(weggli::weggli_native-static STATIC IMPORTED)
add_dependencies(weggli::weggli_native-static cargo_build)
set_target_properties(weggli::weggli_native-static
    PROPERTIES
        PUBLIC_HEADER "${CMAKE_BINARY_DIR}/weggli.h"
        IMPORTED_LOCATION "${CMAKE_SOURCE_DIR}/target/${TARGET_DIR}/libweggli_native.a")

add_library(weggli::weggli_native-shared STATIC IMPORTED)
add_dependencies(weggli::weggli_native-shared cargo_build)
set_target_properties(weggli::weggli_native-shared
    PROPERTIES
        PUBLIC_HEADER "${CMAKE_BINARY_DIR}/weggli.h"
        IMPORTED_LOCATION "${CMAKE_SOURCE_DIR}/target/${TARGET_DIR}/libweggli_native.dylib")

install(
    TARGETS weggli::weggli_native-static weggli::weggli_native-shared
    EXPORT weggli_native-targets
    RUNTIME DESTINATION "bin"
    LIBRARY DESTINATION "lib"
    ARCHIVE DESTINATION "lib"
    PUBLIC_HEADER DESTINATION "include")

problem is, imported targets cannot be installed...

EDIT: also, it seems like it would involve an annoying amount of work to be cross platform

@frabert
Copy link
Contributor Author

frabert commented Jul 13, 2022

This last CMake file is the best I could come up with. I think it's good, but it's not perfect, as the weggli_native-config.cmake file is hardcoded with the CMAKE_PREFIX_PATH inserted during configuration, so it doesn't work properly if another path is passed to cmake --install

@woodruffw
Copy link
Member

Looks pretty good to me. If you're okay with the minor caveat around CMAKE_PREFIX_PATH we can merge for now, and try to come up with a better workaround later.

(We should also look into testing this build in the CI to ensure that we don't accidentally break it, but we that can be another follow-up task for another time.)

@frabert
Copy link
Contributor Author

frabert commented Jul 13, 2022

Yes I think it's sufficient for my uses right now. Things for the future:

  • Windows probably doesn't work like this, as it needs both a .lib and a .dll file for dynamic libraries
  • CMAKE_PREFIX_PATH is hardcoded
  • Needs testing

@woodruffw
Copy link
Member

Sounds good. Mind opening a tracking issue for those?

@woodruffw woodruffw merged commit 17c0971 into trailofbits:main Jul 13, 2022
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