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

Try building FFI in CI, including Windows #17

Merged
merged 13 commits into from
Jun 28, 2022
7 changes: 5 additions & 2 deletions .github/actions/cache/action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,19 @@ inputs:
rust_version:
default: "latest"
required: false
build_profile:
default: "unspecified"
required: false

runs:
using: composite
steps:
- name: ⚡ Cache
uses: actions/cache@v2
uses: actions/cache@v3
with:
path: |
~/.cargo/registry/
~/.cargo/git/db/
~/.cargo/bin/
target/
key: v1-${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.toml') }}-${{ inputs.rust_version }}
key: v1-${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.toml') }}-${{ inputs.rust_version }}-${{ inputs.build_profile }}
Copy link
Contributor

Choose a reason for hiding this comment

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

should we consider the lock file ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think since we commit Cargo.lock anyway that it doesn't matter. If we update a version, then it won't be in the cache and it will pull it.

65 changes: 63 additions & 2 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,19 @@ name: Test
on: [push]
env:
CARGO_TERM_COLOR: always
RUST_VERSION: 1.60.0

jobs:
test:
name: "cargo test --workspace #${{ matrix.platform }} ${{ matrix.rust_version }}"
runs-on: ${{ matrix.platform }}
strategy:
matrix:
platform: [windows-latest, ubuntu-latest, macos-latest]
platform: [windows-latest, ubuntu-latest, macos-latest]
rust_version: [""]
include:
- platform: "ubuntu-latest"
rust_version: "1.60"
rust_version: "${RUST_VERSION}"
steps:
- name: Checkout sources
uses: actions/checkout@v2
Expand All @@ -30,3 +31,63 @@ jobs:
run: cargo build --workspace --verbose
- name: "[${{ steps.rust-version.outputs.version}}] cargo test --workspace --verbose"
run: cargo test --workspace --verbose

ffi:
name: "FFI #${{ matrix.platform }} ${{ matrix.rust_version }}"
runs-on: ${{ matrix.platform }}
strategy:
matrix:
platform: [windows-latest, ubuntu-latest, macos-latest]
rust_version: [""]
include:
- platform: "ubuntu-latest"
rust_version: "${RUST_VERSION}"
steps:

- name: Checkout sources
uses: actions/checkout@v2

- name: Setup output dir
shell: bash
run: |
WORKSPACE_PATH=${{ github.workspace }}
if [[ "${{ matrix.platform }}" == "windows-latest" ]]; then
WORKSPACE_PATH=$(cygpath -ua '${{ github.workspace }}')
fi
echo "OUTPUT_FOLDER=$WORKSPACE_PATH/artifacts" >> $GITHUB_ENV

- name: Cache
uses: ./.github/actions/cache
with:
rust_version: ${{ matrix.rust_version }}
pawelchcki marked this conversation as resolved.
Show resolved Hide resolved
build_profile: "release"

- name: Install Rust ${{ matrix.rust_version }}
if: ${{ matrix.rust_version != '' }}
run: rustup install ${{ matrix.rust_version }} && rustup default ${{ matrix.rust_version }}

- id: rust-version
run: "echo ::set-output name=version::$(rustc --version)"

- name: "Generate FFI"
shell: bash
run: |
chmod +x ffi-build.sh
./ffi-build.sh ${OUTPUT_FOLDER}

- name: 'Publish libdatadog'
uses: actions/upload-artifact@v2
if: '${{ always() }}'
with:
if-no-files-found: error
name: libdatadog.${{ matrix.platform }}
path: ${{ github.workspace }}/artifacts
retention-days: 1

- name: "Test building C bindings"
shell: bash
run: |
mkdir examples/ffi/build
cd examples/ffi/build
cmake -S .. -DDDProf_ROOT=$OUTPUT_FOLDER
cmake --build .
20 changes: 18 additions & 2 deletions ddprof-ffi/cbindgen.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,24 @@ sys_includes = ["stdbool.h", "stddef.h", "stdint.h"]

after_includes = """

#if defined(_MSC_VER)
#define DDPROF_FFI_CHARSLICE_C(string) \\
/* NOTE: Compilation fails if you pass in a char* instead of a literal */ ((ddprof_ffi_CharSlice) {.ptr = "" string, .len = sizeof(string) - 1})"""
/* NOTE: Compilation fails if you pass in a char* instead of a literal */ {.ptr = "" string, .len = sizeof(string) - 1}
#else
#define DDPROF_FFI_CHARSLICE_C(string) \\
/* NOTE: Compilation fails if you pass in a char* instead of a literal */ ((ddprof_ffi_CharSlice){ .ptr = "" string, .len = sizeof(string) - 1 })
#endif

#if defined(__cplusplus) && (__cplusplus >= 201703L)
# define DD_CHECK_RETURN [[nodiscard]]
#elif defined(_Check_return_) /* SAL */
# define DD_CHECK_RETURN _Check_return_
#elif (defined(__has_attribute) && __has_attribute(warn_unused_result)) || \\
(defined(__GNUC__) && (__GNUC__ >= 4))
# define DD_CHECK_RETURN __attribute__((__warn_unused_result__))
#else
# define DD_CHECK_RETURN
#endif"""

[export]
prefix = "ddprof_ffi_"
Expand All @@ -28,7 +44,7 @@ prefix_with_name = true
rename_variants = "ScreamingSnakeCase"

[fn]
must_use = "__attribute__((warn_unused_result))"
must_use = "DD_CHECK_RETURN"

[parse]
parse_deps = true
Expand Down
21 changes: 18 additions & 3 deletions examples/ffi/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,22 @@ cmake_minimum_required(VERSION 3.19)
find_package(DDProf)

add_executable(exporter exporter.cpp)
target_compile_features(exporter PRIVATE cxx_std_11)
target_link_libraries(exporter DDProf::FFI)

add_executable(profiles profiles.c)
target_link_libraries(profiles DDProf::FFI)

set(EXTERNAL_LIBS DDProf::FFI)

if (CMAKE_CXX_COMPILER_ID MATCHES "MSVC")
set(EXTERNAL_LIBS "${EXTERNAL_LIBS};NtDll;UserEnv;Bcrypt;crypt32;wsock32;ws2_32;shlwapi")
endif()

if (CMAKE_CXX_COMPILER_ID MATCHES "MSVC")
target_compile_features(exporter PRIVATE cxx_std_20) # needed for designated initializers
target_compile_definitions(exporter PUBLIC _CRT_SECURE_NO_WARNINGS)
else()
target_compile_features(exporter PRIVATE cxx_std_11)
endif()

target_link_libraries(exporter ${EXTERNAL_LIBS})

target_link_libraries(profiles ${EXTERNAL_LIBS})
2 changes: 1 addition & 1 deletion examples/ffi/exporter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ extern "C" {
#include <thread>

static ddprof_ffi_Slice_c_char to_slice_c_char(const char *s) {
return (ddprof_ffi_Slice_c_char){.ptr = s, .len = strlen(s)};
return {.ptr = s, .len = strlen(s)};
}

struct Deleter {
Expand Down
2 changes: 1 addition & 1 deletion examples/ffi/profiles.c
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ int main(void) {

struct ddprof_ffi_Location root_location = {
// yes, a zero-initialized mapping is valid
.mapping = (struct ddprof_ffi_Mapping){},
.mapping = (struct ddprof_ffi_Mapping){0},
.lines = (struct ddprof_ffi_Slice_line){&root_line, 1},
};
int64_t value = 10;
Expand Down
36 changes: 26 additions & 10 deletions ffi-build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ mkdir -v -p "$destdir/include/ddprof" "$destdir/lib/pkgconfig" "$destdir/cmake"
version=$(awk -F\" '$1 ~ /^version/ { print $2 }' < ddprof-ffi/Cargo.toml)
target="$(rustc -vV | awk '/^host:/ { print $2 }')"
shared_library_suffix=".so"
static_library_suffix=".a"
library_prefix="lib"
remove_rpath=0
fix_macos_rpath=0

Expand All @@ -32,17 +34,28 @@ case "$target" in
# on alpine musl, Rust adds some weird runpath to cdylibs
remove_rpath=1
;;

"x86_64-apple-darwin")
expected_native_static_libs=" -framework Security -framework CoreFoundation -liconv -lSystem -lresolv -lc -lm -liconv"
native_static_libs="${expected_native_static_libs}"
shared_library_suffix=".dylib"
# fix usage of library in macos via rpath
fix_macos_rpath=1
;;

"x86_64-unknown-linux-gnu"|"aarch64-unknown-linux-gnu")
expected_native_static_libs=" -ldl -lrt -lpthread -lgcc_s -lc -lm -lrt -lpthread -lutil -ldl -lutil"
native_static_libs=" -ldl -lrt -lpthread -lc -lm -lrt -lpthread -lutil -ldl -lutil"
;;

"x86_64-pc-windows-msvc")
expected_native_static_libs="" # I don't know what to expect
native_static_libs="" # I don't know what to expect
Comment on lines +52 to +53
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. How did we not end up with test failures with these set to blank?

Copy link
Contributor

Choose a reason for hiding this comment

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

hum good question, maybe because cargo was not able to retrieve those informations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the libs NtDll;UserEnv;Bcrypt;crypt32;wsock32;ws2_32;shlwapi should be in this list, rather than hard-coding them in the CMakeLists.txt. That's how it works on other platforms. That is, if cargo is working "correctly" and knows about the deps (it should).

Copy link
Contributor

Choose a reason for hiding this comment

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

but how would you compile using the CMake if you do not specify the list. (sorry new to that)
If the CMakeList is generated why pushing it ?

shared_library_suffix=".dll"
static_library_suffix=".lib"
library_prefix=""
;;

*)
>&2 echo "Unknown platform '${target}'"
exit 1
Expand Down Expand Up @@ -71,28 +84,31 @@ cp -v LICENSE LICENSE-3rdparty.yml NOTICE "$destdir/"

export RUSTFLAGS="${RUSTFLAGS:- -C relocation-model=pic}"

echo "Building the libddprof_ffi library (may take some time)..."
echo "Building the ddprof_ffi library (may take some time)..."
cargo build --release --target "${target}"
cp -v "target/${target}/release/libddprof_ffi.a" "target/${target}/release/libddprof_ffi${shared_library_suffix}" "$destdir/lib/"

shared_library_name="${library_prefix}ddprof_ffi${shared_library_suffix}"
static_library_name="${library_prefix}ddprof_ffi${static_library_suffix}"
cp -v "target/${target}/release/$static_library_name" "target/${target}/release/$shared_library_name" "$destdir/lib/"

if [[ "$remove_rpath" -eq 1 ]]; then
patchelf --remove-rpath "$destdir/lib/libddprof_ffi${shared_library_suffix}"
patchelf --remove-rpath "$destdir/lib/${shared_library_name}"
fi

if [[ "$fix_macos_rpath" -eq 1 ]]; then
install_name_tool -id @rpath/libddprof_ffi${shared_library_suffix} "$destdir/lib/libddprof_ffi${shared_library_suffix}"
install_name_tool -id @rpath/${shared_library_name} "$destdir/lib/${shared_library_name}"
fi

# objcopy might not be available on macOS
if command -v objcopy > /dev/null; then
if command -v objcopy > /dev/null && [[ "$target" != "x86_64-pc-windows-msvc" ]]; then
# Remove .llvmbc section which is not useful for clients
objcopy --remove-section .llvmbc "$destdir/lib/libddprof_ffi.a"
objcopy --remove-section .llvmbc "$destdir/lib/${static_library_name}"

# Ship debug information separate from shared library, so that downstream packages can selectively include it
# https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html
objcopy --only-keep-debug "$destdir/lib/libddprof_ffi${shared_library_suffix}" "$destdir/lib/libddprof_ffi${shared_library_suffix}.debug"
strip -S "$destdir/lib/libddprof_ffi${shared_library_suffix}"
objcopy --add-gnu-debuglink="$destdir/lib/libddprof_ffi${shared_library_suffix}.debug" "$destdir/lib/libddprof_ffi${shared_library_suffix}"
objcopy --only-keep-debug "$destdir/lib/$shared_library_name" "$destdir/lib/$shared_library_name.debug"
strip -S "$destdir/lib/$shared_library_name"
objcopy --add-gnu-debuglink="$destdir/lib/$shared_library_name.debug" "$destdir/lib/$shared_library_name"
fi

echo "Checking that native-static-libs are as expected for this platform..."
Expand All @@ -118,7 +134,7 @@ if [ -n "$unexpected_native_libs" ]; then
fi
cd -

echo "Generating the ddprof/ffi.h header..."
echo "Generating the $destdir/ddprof/ffi.h header..."
morrisonlevi marked this conversation as resolved.
Show resolved Hide resolved
cbindgen --crate ddprof-ffi --config ddprof-ffi/cbindgen.toml --output "$destdir/include/ddprof/ffi.h"

# CI doesn't have any clang tooling
Expand Down