Uh oh!
There was an error while loading. Please reload this page.
Bindings Updates for #681 and more Clones - #749
Merged
TheBlueMatt merged 12 commits intoNov 23, 2020
Conversation
valentinewallace
self-requested a review
November 16, 2020 19:13
jkczyz
reviewed
Nov 22, 2020
jkczyz
left a comment
Contributor
There was a problem hiding this comment.
Not sure if it is related, but I'm getting an error when running genbindings.sh on macOS:
+ cargo rustc -v --release -- -C lto
Compiling cc v1.0.65
Compiling bitcoin_hashes v0.8.0
Compiling bech32 v0.7.2
Running `rustc --crate-name cc --edition=2018 /Users/jkczyz/.cargo/registry/src/github.com-1ecc6299db9ec823/cc-1.0.65/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -Cembed-bitcode=no -C metadata=00dcc370fd0f6eb8 -C extra-filename=-00dcc370fd0f6eb8 --out-dir /Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps -L dependency=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps --cap-lints allow`
Running `rustc --crate-name bitcoin_hashes /Users/jkczyz/.cargo/registry/src/github.com-1ecc6299db9ec823/bitcoin_hashes-0.8.0/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -Cembed-bitcode=no --cfg 'feature="default"' --cfg 'feature="std"' -C metadata=3f72b8b01c4f2e37 -C extra-filename=-3f72b8b01c4f2e37 --out-dir /Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps -L dependency=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps --cap-lints allow`
Running `rustc --crate-name bech32 /Users/jkczyz/.cargo/registry/src/github.com-1ecc6299db9ec823/bech32-0.7.2/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -Cembed-bitcode=no -C metadata=848e3766e44cacd7 -C extra-filename=-848e3766e44cacd7 --out-dir /Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps -L dependency=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps --cap-lints allow`
Compiling secp256k1-sys v0.2.0
Running `rustc --crate-name build_script_build /Users/jkczyz/.cargo/registry/src/github.com-1ecc6299db9ec823/secp256k1-sys-0.2.0/build.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type bin --emit=dep-info,link -C opt-level=3 -Cembed-bitcode=no --cfg 'feature="std"' -C metadata=73b76d4b12ae7a7f -C extra-filename=-73b76d4b12ae7a7f --out-dir /Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/build/secp256k1-sys-73b76d4b12ae7a7f -L dependency=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps --extern cc=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps/libcc-00dcc370fd0f6eb8.rlib --cap-lints allow`
Running `/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/build/secp256k1-sys-73b76d4b12ae7a7f/build-script-build`
Running `rustc --crate-name secp256k1_sys /Users/jkczyz/.cargo/registry/src/github.com-1ecc6299db9ec823/secp256k1-sys-0.2.0/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -Cembed-bitcode=no --cfg 'feature="std"' -C metadata=caf2e22195643b8f -C extra-filename=-caf2e22195643b8f --out-dir /Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps -L dependency=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps --cap-lints allow -L native=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/build/secp256k1-sys-6ea31cf4a9c94cb1/out -l static=secp256k1`
Compiling secp256k1 v0.18.0
Running `rustc --crate-name secp256k1 /Users/jkczyz/.cargo/registry/src/github.com-1ecc6299db9ec823/secp256k1-0.18.0/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -Cembed-bitcode=no --cfg 'feature="default"' --cfg 'feature="std"' -C metadata=1eabee0c3285b09c -C extra-filename=-1eabee0c3285b09c --out-dir /Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps -L dependency=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps --extern secp256k1_sys=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps/libsecp256k1_sys-caf2e22195643b8f.rmeta --cap-lints allow -L native=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/build/secp256k1-sys-6ea31cf4a9c94cb1/out`
Compiling bitcoin v0.24.0
Running `rustc --crate-name bitcoin /Users/jkczyz/.cargo/registry/src/github.com-1ecc6299db9ec823/bitcoin-0.24.0/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -Cembed-bitcode=no -C metadata=e1b66b4ed9a90379 -C extra-filename=-e1b66b4ed9a90379 --out-dir /Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps -L dependency=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps --extern bech32=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps/libbech32-848e3766e44cacd7.rmeta --extern bitcoin_hashes=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps/libbitcoin_hashes-3f72b8b01c4f2e37.rmeta --extern secp256k1=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps/libsecp256k1-1eabee0c3285b09c.rmeta --cap-lints allow -L native=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/build/secp256k1-sys-6ea31cf4a9c94cb1/out`
Compiling lightning v0.0.11 (/Users/jkczyz/src/rust-lightning-review/lightning)
Running `rustc --crate-name lightning /Users/jkczyz/src/rust-lightning-review/lightning/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -Cembed-bitcode=no -C metadata=79da2e4794c19ffc -C extra-filename=-79da2e4794c19ffc --out-dir /Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps -L dependency=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps --extern bitcoin=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps/libbitcoin-e1b66b4ed9a90379.rmeta -L native=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/build/secp256k1-sys-6ea31cf4a9c94cb1/out`
Compiling lightning-c-bindings v0.0.1 (/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings)
Running `rustc --crate-name ldk --edition=2018 src/lib.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type staticlib --crate-type cdylib --emit=dep-info,link -C opt-level=3 -Cembed-bitcode=no -C lto -C metadata=fe50c7fd2ceda483 --out-dir /Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps -L dependency=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps --extern bitcoin=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps/libbitcoin-e1b66b4ed9a90379.rlib --extern lightning=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps/liblightning-79da2e4794c19ffc.rlib -L native=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/build/secp256k1-sys-6ea31cf4a9c94cb1/out`
error: options `-C embed-bitcode=no` and `-C lto` are incompatible
error: could not compile `lightning-c-bindings`.
Caused by:
process didn't exit successfully: `rustc --crate-name ldk --edition=2018 src/lib.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type staticlib --crate-type cdylib --emit=dep-info,link -C opt-level=3 -Cembed-bitcode=no -C lto -C metadata=fe50c7fd2ceda483 --out-dir /Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps -L dependency=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps --extern bitcoin=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps/libbitcoin-e1b66b4ed9a90379.rlib --extern lightning=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/deps/liblightning-79da2e4794c19ffc.rlib -L native=/Users/jkczyz/src/rust-lightning-review/lightning-c-bindings/target/release/build/secp256k1-sys-6ea31cf4a9c94cb1/out` (exit code: 1)
Uh oh!
There was an error while loading. Please reload this page.
TheBlueMattforce-pushed
the
2020-11-bindings-clones
branch
from
November 23, 2020 15:29
6a3db9c to
674559cCompareTheBlueMatt
commented
Nov 23, 2020
CollaboratorAuthor
No, but Val did note it previously and I wasn't able to reproduce on newer rustc so I forgot about it, sorry about that. I pushed a commit to fix it mostly based on Val's suggested patch at #721 (comment) |
Codecov Report
@@ Coverage Diff @@## main #749 +/- ##
==========================================
- Coverage 91.39% 91.29% -0.10%
==========================================
Files 37 37 Lines 22249 22274 +25 ==========================================
+ Hits 20334 20335 +1 - Misses 1915 1939 +24
Continue to review full report at Codecov.
|
…gdevkit#681) Previously we'd ignored generic arguments in traits, leading to bogus code generation after the Persister trait was added in lightningdevkit#681. This adds minimal support for it, fixing code generation on latest upstream.
When the only reference to the transaction bytes is via Transaction::data, my understanding of the C const rules is that it would then be invalid to write to it. While its unlikely this would ever pose an issue, its not hard to simply make it *mut, so we do that here.
CVecTempl previously called Vec.clone_from_slice() on a newly-allocated Vec, which immediately panics as [T].clone_from_slice() requires that the Vec/target slice already has the same length as the source slice. This should have been Vec.extend_from_slice() which exhibits the correct behavior.
This somewhat assumes that every public enum implements clone in some way, but that is currently the case.
There is no reason not to and Clone can be useful especially in the bindings context.
When a trait is required to implement eq/clone (eg in the case of `SocketDescriptor`), the generated trait struct contains an eq/clone function which takes a `this_arg` pointer. Since the trait object can always be read to get the `this_arg` pointer, there is no loss of generality to pass the trait object itself, and it provides a bit more flexibility when the trait could be one of several implementations (which we use in the Java higher-level bindings).
Newer versions of cargo broke `cargo rustc -- -Clto` by passing `-Cembed-bitcode=no` even in `--release`. Its somewhat unclear why this is still the case on latest cargo given it broke, at least, compilation of Firefox, as discovered by Val at [1]. We take the approach they used there, even though they later walked it back [2] but the issues noted there, especially in [3] don't seem particularly concerning in our case. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1640982 [2] https://bugzilla.mozilla.org/show_bug.cgi?id=1654465 [3] https://bugzilla.mozilla.org/show_bug.cgi?id=1654465#c5
TheBlueMattforce-pushed
the
2020-11-bindings-clones
branch
from
November 23, 2020 16:12
674559c to
9fe3124CompareTheBlueMatt
commented
Nov 23, 2020
CollaboratorAuthor
Also rebased on upstream git which changed ever so slightly. |
jkczyz
approved these changes
Nov 23, 2020
valentinewallace
approved these changes
Nov 23, 2020
valentinewallace
left a comment
Contributor
There was a problem hiding this comment.
LGTM! Thanks for the fixes on genbindings, that seems to have worked for me (haven't figured out how to get the right clang version so haven't tested out the address-sanitizer parts, but otherwise the headers are generated 💪)
| writeln!(w, "#[no_mangle]\npub extern \"C\" fn {}_free(this_ptr: {}) {{ }}", e.ident, e.ident).unwrap(); | ||
| } | ||
| writeln!(w, "#[no_mangle]").unwrap(); | ||
| writeln!(w, "pub extern \"C\" fn {}_clone(orig: &{}) -> {} {{", e.ident, e.ident, e.ident).unwrap(); |
Contributor
There was a problem hiding this comment.
Just to confirm, it's fine that some pub(enum)s in RL don't implement Clone?
CollaboratorAuthor
There was a problem hiding this comment.
Hmm, I mean I guess we should have them match, but its not an issue per se currently - if all the fields support Clone (as they would have to for the bindings crate to not fail to compile), then exposing a Clone to bindings seems fine.
TheBlueMatt
commented
Nov 23, 2020
CollaboratorAuthor
Taking cause we need to get 0.0.12 out the door. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was one trivial bit to fix for #681, but more importantly this adds much better and broader support for calling
clonefrom the C bindings, which we rely on extensively in the Java bindings.