Skip to content

Add developer guideline notes for C/C++ bindings generation #688

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Sep 14, 2020
Merged

Add developer guideline notes for C/C++ bindings generation #688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
83322e1
Add developer guideline notes for C/C++ bindings generation
TheBlueMatt Sep 11, 2020
dbf4f58
Remove the bindings crate from the root namespace to let it break
TheBlueMatt Sep 11, 2020
53182a8
Note that genbindings.sh is really also a test script.
TheBlueMatt Sep 11, 2020
dce831e
Rename lightning C/C++ bindings library to libldk
TheBlueMatt Sep 12, 2020
3fa0548
Add a few more things to gitignore for bindings
TheBlueMatt Sep 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,12 @@ jobs:
include:
- toolchain: stable
build-net-tokio: true
build-bindings: true
- toolchain: beta
build-net-tokio: true
build-bindings: true
- toolchain: 1.39.0
build-net-tokio: true
build-bindings: true
coverage: true
- toolchain: 1.34.2
build-bindings: true
runs-on: ubuntu-latest
steps:
- name: Checkout source code
Expand All @@ -43,9 +39,6 @@ jobs:
- name: Build on Rust ${{ matrix.toolchain }}
if: "! matrix.build-net-tokio"
run: RUSTFLAGS="-C link-dead-code" cargo build --verbose --color always -p lightning
- name: Build bindings on Rust ${{ matrix.toolchain }}
if: matrix.build-bindings
run: RUSTFLAGS="-C link-dead-code" cargo build --verbose --color always -p lightning-c-bindings
- name: Test on Rust ${{ matrix.toolchain }} with net-tokio
if: matrix.build-net-tokio
run: RUSTFLAGS="-C link-dead-code" cargo test --verbose --color always
Expand Down Expand Up @@ -127,6 +120,9 @@ jobs:
apt-get -y install cargo valgrind lld git g++ clang
- name: Checkout source code
uses: actions/checkout@v2
- name: Sanity test bindings
Copy link

Choose a reason for hiding this comment

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

An alternative could be to also run genbindings.sh with the continue-on-error keyword. That way we could still see the output if it gets out of sync, but it doesn't prevent the job from failing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure what the value would be - if the bindings fall out of sync, genbindings.sh isn't going to give you a lot of useful output that cargo check isn't? I added it in part to short-circuit all the /stuff/ if things are stale anyway (because the intent would be one of the full-timers or regular contributors can go fix it later).

Copy link

@julianknutsen julianknutsen Sep 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Understanding the lifecycle for the generated code makes this clearer. I guess you would have to pair genbindings.sh with a git diff or something to show that "hey it's out of sync" since cargo check doesn't always identify mismatches, but that isn't really the goal so it seems fine as-is.

Copy link
Collaborator Author

@TheBlueMatt TheBlueMatt Sep 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The build script a bit further doesn't does do some git diff stuff (though noting that the bindings aren't fully deterministic in terms of ordering, so letting that fluctuate a bit), but cargo check is a much quicker way to get that.

working-directory: lightning-c-bindings
run: cargo check
- name: Install cbindgen
run: cargo install --force cbindgen
- name: Rebuild bindings, and check the sample app builds + links
Expand Down
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
/target/
c-bindings-gen/target/
lightning-c-bindings/target/
lightning-c-bindings/a.out
/hfuzz_target/
/net-tokio/target/
**/*.rs.bk
Expand Down
11 changes: 11 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,17 @@ Fuzzing is heavily encouraged: you will find all related material under `fuzz/`

Mutation testing is work-in-progress; any contribution there would be warmly welcomed.

C/C++ Bindings
--------------

You can learn more about the C/C++ bindings that are made available by reading the
[C/C++ Bindings README](lightning-c-bindings/README.md). If you are not using the C/C++ bindings,
you likely don't need to worry about them, and during their early experimental phase we are not
requiring that pull requests keep the bindings up to date (and, thus, pass the bindings_check CI
run). If you wish to ensure your PR passes the bindings generation phase, you should run the
`genbindings.sh` script in the top of the directory tree to generate, build, and test C bindings on
Copy link

Choose a reason for hiding this comment

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

I noticed a lot of the build process uses shell scripts. What does the supported build platform matrix look like? Is all the tooling mostly focused on mac/linux or are there Windows use cases?

In general, how much of the entire project (lightning/tokio/c-bindings/etc) should be buildable on Mac and Windows? Trying to wrap my head around the supported platform matrix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, I don't think we have anyone working with/spending too much time on Windows, but if you're less interested in testing and only want a shared library build, just running cargo build in c-lightning-bindings should work. Eventually it probably makes sense to spend some time on Windows but it hasn't been a priority.

Copy link

@julianknutsen julianknutsen Sep 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Running genbindings.sh generates a few files that aren't in the .gitignore. Is it worth adding them?

c-bindings-gen/target/
lightning-c-bindings/target/

Should this end up in target/ or .gitignore?
lightning-c-bindings/include/lightningpp.hpp

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

target should definitely be in .gitignore, but at a high level our goal here is to check in the C bindings that are generated so that users don't theoretically need to run the generation scripts (ie can just cd lightning-c-bindings && cargo test). CI tests that the versions in git are the same as the ones that are auto-generated.

Copy link

Choose a reason for hiding this comment

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

Ok. That makes sense why lightningpp.hpp is outside of target. Thanks for the background.

Copy link

Choose a reason for hiding this comment

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

Verified the target files are now ignored in 74cd3c9. Thanks!

your local system.

Going further
-------------

Expand Down
1 change: 0 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
members = [
"lightning",
"lightning-net-tokio",
"lightning-c-bindings",
]

# Our tests do actual crypo and lots of work, the tradeoff for -O1 is well worth it
Expand Down
22 changes: 11 additions & 11 deletions genbindings.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,21 +48,21 @@ fi
# Finally, sanity-check the generated C and C++ bindings with demo apps:

# Naively run the C demo app:
gcc -Wall -g -pthread demo.c ../target/debug/liblightning.a -ldl
gcc -Wall -g -pthread demo.c target/debug/libldk.a -ldl
./a.out

# And run the C++ demo app in valgrind to test memory model correctness and lack of leaks.
g++ -std=c++11 -Wall -g -pthread demo.cpp -L../target/debug/ -llightning -ldl
g++ -std=c++11 -Wall -g -pthread demo.cpp -Ltarget/debug/ -lldk -ldl
if [ -x "`which valgrind`" ]; then
LD_LIBRARY_PATH=../target/debug/ valgrind --error-exitcode=4 --memcheck:leak-check=full --show-leak-kinds=all ./a.out
LD_LIBRARY_PATH=target/debug/ valgrind --error-exitcode=4 --memcheck:leak-check=full --show-leak-kinds=all ./a.out
echo
else
echo "WARNING: Please install valgrind for more testing"
fi

# Test a statically-linked C++ version, tracking the resulting binary size and runtime
# across debug, LTO, and cross-language LTO builds (using the same compiler each time).
clang++ -std=c++11 -Wall -pthread demo.cpp ../target/debug/liblightning.a -ldl
clang++ -std=c++11 -Wall -pthread demo.cpp target/debug/libldk.a -ldl
./a.out >/dev/null
echo " C++ Bin size and runtime w/o optimization:"
ls -lha a.out
Expand All @@ -75,19 +75,19 @@ if [ "$HOST_PLATFORM" = "host: x86_64-unknown-linux-gnu" ]; then
if [ -x "$(which clang-$LLVM_V)" ]; then
cargo +nightly clean
cargo +nightly rustc -Zbuild-std --target x86_64-unknown-linux-gnu -v -- -Zsanitizer=memory -Zsanitizer-memory-track-origins -Cforce-frame-pointers=yes
mv ../target/x86_64-unknown-linux-gnu/debug/liblightning.* ../target/debug/
mv target/x86_64-unknown-linux-gnu/debug/libldk.* target/debug/

# Sadly, std doesn't seem to compile into something that is memsan-safe as of Aug 2020,
# so we'll always fail, not to mention we may be linking against git rustc LLVM which
# may differ from clang-llvm, so just allow everything here to fail.
set +e

# First the C demo app...
clang-$LLVM_V -std=c++11 -fsanitize=memory -fsanitize-memory-track-origins -Wall -g -pthread demo.c ../target/debug/liblightning.a -ldl
clang-$LLVM_V -std=c++11 -fsanitize=memory -fsanitize-memory-track-origins -Wall -g -pthread demo.c target/debug/libldk.a -ldl
./a.out

# ...then the C++ demo app
clang++-$LLVM_V -std=c++11 -fsanitize=memory -fsanitize-memory-track-origins -Wall -g -pthread demo.cpp ../target/debug/liblightning.a -ldl
clang++-$LLVM_V -std=c++11 -fsanitize=memory -fsanitize-memory-track-origins -Wall -g -pthread demo.cpp target/debug/libldk.a -ldl
./a.out >/dev/null

# restore exit-on-failure
Expand Down Expand Up @@ -153,11 +153,11 @@ if [ "$HOST_PLATFORM" = "host: x86_64-unknown-linux-gnu" -o "$HOST_PLATFORM" = "
mv Cargo.toml.bk Cargo.toml

# First the C demo app...
$CLANG -fsanitize=address -Wall -g -pthread demo.c ../target/debug/liblightning.a -ldl
$CLANG -fsanitize=address -Wall -g -pthread demo.c target/debug/libldk.a -ldl
ASAN_OPTIONS='detect_leaks=1 detect_invalid_pointer_pairs=1 detect_stack_use_after_return=1' ./a.out

# ...then the C++ demo app
$CLANGPP -std=c++11 -fsanitize=address -Wall -g -pthread demo.cpp ../target/debug/liblightning.a -ldl
$CLANGPP -std=c++11 -fsanitize=address -Wall -g -pthread demo.cpp target/debug/libldk.a -ldl
ASAN_OPTIONS='detect_leaks=1 detect_invalid_pointer_pairs=1 detect_stack_use_after_return=1' ./a.out >/dev/null
else
echo "WARNING: Please install clang-$RUSTC_LLVM_V and clang++-$RUSTC_LLVM_V to build with address sanitizer"
Expand All @@ -168,7 +168,7 @@ fi

# Now build with LTO on on both C++ and rust, but without cross-language LTO:
cargo rustc -v --release -- -C lto
clang++ -std=c++11 -Wall -flto -O2 -pthread demo.cpp ../target/release/liblightning.a -ldl
clang++ -std=c++11 -Wall -flto -O2 -pthread demo.cpp target/release/libldk.a -ldl
echo "C++ Bin size and runtime with only RL (LTO) optimized:"
ls -lha a.out
time ./a.out > /dev/null
Expand All @@ -180,7 +180,7 @@ if [ "$HOST_PLATFORM" != "host: x86_64-apple-darwin" -a "$CLANGPP" != "" ]; then
# packaging than simply shipping the rustup binaries (eg Debian should Just Work
# here).
cargo rustc -v --release -- -C linker-plugin-lto -C lto -C link-arg=-fuse-ld=lld
$CLANGPP -Wall -std=c++11 -flto -fuse-ld=lld -O2 -pthread demo.cpp ../target/release/liblightning.a -ldl
$CLANGPP -Wall -std=c++11 -flto -fuse-ld=lld -O2 -pthread demo.cpp target/release/libldk.a -ldl
echo "C++ Bin size and runtime with cross-language LTO:"
ls -lha a.out
time ./a.out > /dev/null
Expand Down
7 changes: 6 additions & 1 deletion lightning-c-bindings/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,16 @@ Utilities to fetch the chain from Bitcoin Core REST/RPC Interfaces and feed them
"""

[lib]
name = "lightning"
name = "ldk"
crate-type = ["staticlib"
# Note that the following line is matched exactly by genbindings to turn off dylib creation
,"cdylib"]

[dependencies]
bitcoin = "0.24"
lightning = { version = "0.0.11", path = "../lightning" }

# We eventually want to join the root workspace, but for now, the bindings generation is
# a bit brittle and we don't want to hold up other developers from making changes just
# because they break the bindings
[workspace]
6 changes: 6 additions & 0 deletions lightning-c-bindings/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,12 @@ These include:
**It is highly recommended that you test any code which relies on the C (or C++) bindings in
valgrind, AddressSanitizer, MemorySanitizer, or other similar tools to ensure correctness.**

Process
=======

`genbindings.sh` is currently a catch-all script for bindings - it generates the latest Rust/C/C++
code for bindings from the rust-lightning source code, builds it, and then runs various test apps.

Note that after running `genbindings.sh`, if possible, the static lib in target/debug (eg
target/debug/liblightning.a) will be linked with address sanitizer. In order to build against it,
you will need to link with `clang` with `-fsanitize=address` with the same version of LLVM as
Expand Down