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

feat(ffi): initial C# bindings (connector only) #423

Merged
merged 47 commits into from
Apr 5, 2024
Merged

Conversation

irvingoujAtDevolution
Copy link
Contributor

@irvingoujAtDevolution irvingoujAtDevolution commented Mar 25, 2024

Created skeleton for FFI.

Connector module is working now. Please see ffi\dotnet\Devolutions.IronRdp.ConnectExample
image

Note: this is a draft, we'll need to discuss more about

  1. how would we like to expose rustls.
  2. how would we like to handle generics like framed and static channels.
  3. should we expose async interface? or just stick with the sync one?

There are some imperfections/todo/fixme for now, my apologies.

Copy link

github-actions bot commented Mar 25, 2024

Coverage Report 🤖 ⚙️

Past:
Total lines: 29575
Covered lines: 17244 (58.31%)

New:
Total lines: 29575
Covered lines: 17244 (58.31%)

Diff: +0.00%

[this comment will be updated automatically]

ffi/justfile Outdated Show resolved Hide resolved
ffi/src/connector/mod.rs Outdated Show resolved Hide resolved
ffi/src/pdu.rs Outdated Show resolved Hide resolved
@irvingoujAtDevolution
Copy link
Contributor Author

irvingoujAtDevolution commented Mar 26, 2024

formatter seems not working anymore, it does not tell you where the format is wrong, neither could cargo +stable fmt --all fix it.

update: somehow formatter require me to fix a place I never edited, it is inside crates/ironrdp-testsuite-core/tests/pcb.rs

@CBenoit
Copy link
Member

CBenoit commented Mar 27, 2024

  • how would we like to expose rustls.

We don’t expose rustls, we’ll use SslStream in C#.
The API we expose should be sans-IO and we only push and pull bytes from a state machine. This means that as a rule of thumb, we don’t perform any I/O in the ffi crate (not even using TcpStream).

https://sans-io.readthedocs.io/how-to-sans-io.html#why-write-i-o-free-protocol-implementations

IronRDP is already largely following this philosophy (and IronVNC too). We do expose higher level helpers to simplify things in upper layers, but this is not part of the "core crates".

  • how would we like to handle generics like framed and static channels.

Let’s not expose the API for static channels like we do in Rust. I think we can safely hardcode the supported channels like this. We’re not going to implement static channels in C# either, so there is no need to care too deeply about genericity here.

As for "Framed", it’s not supposed to be exposed. This is a higher level helper for Rust code.

  • should we expose async interface? or just stick with the sync one?

We stick to the "core" API, which is "sans-IO" and thus do not require choosing between async or not.

However, we should provide an higher level API which may be async.
The additional handwritten classes or addons should live in a src/ folder like in Picky: https://github.com/Devolutions/picky-rs/tree/master/ffi/dotnet/Devolutions.Picky/src

However, let’s stick to the standard C# library in the higher level helpers. I don’t think we need much more than that and it will save us maintenance time. If a fancy dependency is required, let’s handle that downstream (in RDM).

Cargo.lock Outdated
Comment on lines 143 to 147
[[package]]
name = "anyhow"
version = "1.0.80"
version = "1.0.81"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "5ad32ce52e4161730f7098c077cd2ed6229b5804ccf99e5366be1ab72a98b4e1"
checksum = "0952808a6c2afd1aa8947271f3a60f1a6763c7b912d210184c5149b5cf147247"
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: Avoid to update the Cargo.lock more than necessary. We use dependabot to update it piece by piece in a controlled way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated!

xtask/src/cli.rs Outdated Show resolved Hide resolved
Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

Can you also add a .gitattributes file like we have in Picky?
https://github.com/Devolutions/picky-rs/blob/master/.gitattributes

ffi/src/connector/config.rs Outdated Show resolved Hide resolved
irvingoujAtDevolution and others added 2 commits April 5, 2024 10:34
Co-authored-by: Benoît Cortier <bcortier@proton.me>
Co-authored-by: Benoît Cortier <bcortier@proton.me>
ffi/src/lib.rs Show resolved Hide resolved
ffi/src/connector/mod.rs Outdated Show resolved Hide resolved
irvingoujAtDevolution and others added 4 commits April 5, 2024 10:37
Co-authored-by: Benoît Cortier <bcortier@proton.me>
Co-authored-by: Benoît Cortier <bcortier@proton.me>
Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!!

@CBenoit CBenoit changed the title feat(ffi): initial ffi, ironrdp::connector works feat(ffi): initial C# bindings (connector only) Apr 5, 2024
@CBenoit CBenoit enabled auto-merge (squash) April 5, 2024 15:03
@CBenoit CBenoit merged commit 3bcf513 into master Apr 5, 2024
7 checks passed
@CBenoit CBenoit deleted the create-ffi-bindings branch April 5, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants