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

TheBlueMatt · 2020-09-11T18:46:50Z

No description provided.

CONTRIBUTING.md

codecov · 2020-09-11T19:16:57Z

Codecov Report

Merging #688 into main will increase coverage by 13.44%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main     #688       +/-   ##
===========================================
+ Coverage   78.40%   91.85%   +13.44%     
===========================================
  Files          55       35       -20     
  Lines       27568    19911     -7657     
===========================================
- Hits        21616    18290     -3326     
+ Misses       5952     1621     -4331

Impacted Files	Coverage Δ
lightning/src/util/errors.rs	`64.70% <0.00%> (-16.69%)`	⬇️
lightning/src/ln/features.rs	`89.37% <0.00%> (-7.76%)`	⬇️
lightning/src/util/byte_utils.rs	`93.75% <0.00%> (-6.25%)`	⬇️
lightning/src/util/ser.rs	`86.85% <0.00%> (-3.61%)`	⬇️
lightning/src/ln/peer_channel_encryptor.rs	`93.35% <0.00%> (-1.87%)`	⬇️
lightning/src/ln/channelmonitor.rs	`94.99% <0.00%> (-1.76%)`	⬇️
lightning/src/util/events.rs	`27.58% <0.00%> (-1.45%)`	⬇️
lightning/src/util/transaction_utils.rs	`98.64% <0.00%> (-1.36%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`98.76% <0.00%> (-1.24%)`	⬇️
lightning/src/util/test_utils.rs	`85.00% <0.00%> (-1.20%)`	⬇️
... and 44 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 253af8d...3fa0548. Read the comment docs.

ariard

Thanks to add this!

ariard · 2020-09-11T22:09:29Z

CONTRIBUTING.md

+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 your local
+system.


How to test C bindings is explained in lightning-c-bindings/README.md, even if is the explanation is still experimental itself ?

genbindings.sh does test the bindings :)

TheBlueMatt · 2020-09-11T23:36:27Z

Also fixed it so that we really dont require bindings to pass pre-merge/cargo test. Especially given ##649, we dont want to have to wait just for new bindings generation.

TheBlueMatt · 2020-09-12T02:38:00Z

It looks like honggfuzz upstream is broken. Lets see what they say rust-fuzz/honggfuzz-rs#44

julianknutsen · 2020-09-12T03:17:16Z

Tested and confirmed that cargo doc works on 450391c. #689

TheBlueMatt · 2020-09-12T18:33:40Z

honggfuzz got a new release and is working again.

julianknutsen

What are your thoughts on genbindings.sh doing the building and testing long-term? Will there be a C test suite as this gets more mature and stable or will that be covered by more of the other language bindings that build on top of this with their own testing?

julianknutsen · 2020-09-12T23:02:35Z

CONTRIBUTING.md

+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


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.

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.

julianknutsen · 2020-09-12T23:05:55Z

CONTRIBUTING.md

+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


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

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.

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

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

.github/workflows/build.yml

julianknutsen · 2020-09-12T23:11:36Z

.github/workflows/build.yml

@@ -127,6 +120,8 @@ jobs:
          apt-get -y install cargo valgrind lld git g++ clang
      - name: Checkout source code
        uses: actions/checkout@v2
+      - name: Sanity test bindings


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.

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).

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.

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.

julianknutsen · 2020-09-12T23:13:49Z

genbindings.sh

@@ -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/liblightning.a -ldl


What do you think about splitting out the names, and paths into variables? I don't expect this file to change all that much, but it deduplicates a lot of the path names.

Sure, I mean happy to do whatever, not sure its gonna change much independently though.

ariard · 2020-09-14T00:11:26Z

ACK 74cd3c9

genbindings.sh is locally working.

Until we get the bindings generation process super stable, let the bindings get stale with respect to the main repo while still letting `cargo check` pass.

This should fix lightningdevkit#689.

TheBlueMatt · 2020-09-14T00:59:02Z

Squashed the one fixup commit.

jkczyz approved these changes Sep 11, 2020

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

ariard reviewed Sep 11, 2020

View reviewed changes

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

83322e1

TheBlueMatt force-pushed the 2020-09-dev-docs branch 2 times, most recently from 77b35bb to 83b18e7 Compare September 11, 2020 23:35

TheBlueMatt force-pushed the 2020-09-dev-docs branch from 83b18e7 to 29fe59b Compare September 12, 2020 01:12

TheBlueMatt mentioned this pull request Sep 12, 2020

Failure generating documentation with multiple lightning libraries #689

Closed

TheBlueMatt force-pushed the 2020-09-dev-docs branch from 29fe59b to 450391c Compare September 12, 2020 02:33

julianknutsen reviewed Sep 12, 2020

View reviewed changes

TheBlueMatt force-pushed the 2020-09-dev-docs branch from 450391c to 22211f9 Compare September 13, 2020 17:01

TheBlueMatt added 4 commits September 13, 2020 20:58

Remove the bindings crate from the root namespace to let it break

dbf4f58

Until we get the bindings generation process super stable, let the bindings get stale with respect to the main repo while still letting `cargo check` pass.

Note that genbindings.sh is really also a test script.

53182a8

Rename lightning C/C++ bindings library to libldk

dce831e

This should fix lightningdevkit#689.

Add a few more things to gitignore for bindings

3fa0548

TheBlueMatt force-pushed the 2020-09-dev-docs branch from 74cd3c9 to 3fa0548 Compare September 14, 2020 00:58

TheBlueMatt merged commit baa58c1 into lightningdevkit:main Sep 14, 2020

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

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

Uh oh!

Conversation

TheBlueMatt commented Sep 11, 2020

Uh oh!

Uh oh!

codecov bot commented Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Sep 11, 2020

Uh oh!

TheBlueMatt commented Sep 12, 2020

Uh oh!

julianknutsen commented Sep 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Sep 12, 2020

Uh oh!

julianknutsen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julianknutsen Sep 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julianknutsen Sep 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Sep 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Sep 14, 2020

Uh oh!

TheBlueMatt commented Sep 14, 2020

Uh oh!

Uh oh!

codecov bot commented Sep 11, 2020 •

edited

Loading

julianknutsen commented Sep 12, 2020 •

edited

Loading

julianknutsen Sep 12, 2020 •

edited

Loading

julianknutsen Sep 13, 2020 •

edited

Loading

TheBlueMatt Sep 13, 2020 •

edited

Loading