Skip to content

WIP: Rust wrapper #3181

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

Closed
wants to merge 73 commits into from
Closed

WIP: Rust wrapper #3181

wants to merge 73 commits into from

Conversation

nyurik
Copy link
Member

@nyurik nyurik commented Jan 27, 2025

TODO/Goals

  • Include core header files
  • Generate/write a tiny C++ wrapper that Rust can communicate with
  • Compile core with cmake, possibly including the tiny C++ generated code - resulting in a statically-linkable (from Rust) lib
  • Use proper Rust strings instead of &CStr and CString where it is not performance critical.
    • PROs: consistency with the Rust ecosystem, proper utf-8 validated strings going in and out of the crate.
    • CONs: requires extra allocation and validation. On invalid string will panic, or we must introduce Result<String> for proper error reporting.
  • Fix unit tests not compiling (linking), while cargo run works ok
  • Camera operations for tile and static image rendering

Implementation ideas

Current thinking is to get cmake to produce a static library. All steps must be initiated by the platform/rust-cxx/build.rs:

  • cxx bridge generates a header file out of the #[cxx::bridge] mod ffi { ... } in platform/rust-cxx/src/main.rs
  • cmake crate executes cmake with special flags to pick up that header file, plus any other CC and H files in the rust-cxx dirs, plus the rest of the core and produces static lib
  • main.rs links to it and calls get_version or some other basic thing

@nyurik nyurik marked this pull request as draft January 27, 2025 18:07
@nyurik nyurik changed the title Rust wrapper WIP: Rust wrapper Jan 27, 2025
@nyurik nyurik force-pushed the rust-wrapper branch 2 times, most recently from 611176c to d2ef4df Compare January 27, 2025 19:44
Copy link

github-actions bot commented Jan 29, 2025

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  [ = ]       0  [ = ]       0    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-3181-compared-to-main.txt

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +32% +36.5Mi  +439% +26.3Mi    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-3181-compared-to-legacy.txt

Copy link

github-actions bot commented Jan 29, 2025

Benchmark Results ⚡

Benchmark                                                     Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------------------
OVERALL_GEOMEAN                                            +0.0109         +0.0109             0             0             0             0

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-3181-compared-to-main.txt

return std::make_unique<TileServerOptions>();
}
std::unique_ptr<TileServerOptions> TileServerOptions_mapbox() {
return std::make_unique<TileServerOptions>(TileServerOptions::MapLibreConfiguration());
Copy link
Member

Choose a reason for hiding this comment

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

These are pending deprecation and removal.

Copy link
Member Author

Choose a reason for hiding this comment

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

For now these are good for testing, but I agree that we should remove these

@@ -111,6 +111,14 @@ jobs:
cmake -B build -GNinja -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_BUILD_TYPE=RelWithDebInfo -DMLN_WITH_CLANG_TIDY=ON -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DMLN_WITH_COVERAGE=ON ${{ env.renderer_flag_cmake }}
cmake --build build --target mbgl-core mbgl-test-runner mbgl-render-test-runner mbgl-expression-test mbgl-render mbgl-benchmark-runner

# test rust platform
- if: matrix.renderer == 'drawable-rust'
Copy link
Member

Choose a reason for hiding this comment

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

We should move this to rust-ci.

@@ -0,0 +1,10 @@
// FIXME: Remove this before merging
Copy link
Member

Choose a reason for hiding this comment

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

.

@CommanderStorm
Copy link
Collaborator

since we moved things to its own wrapper, I think this PR is moot, right?

@nyurik
Copy link
Member Author

nyurik commented Jul 30, 2025

tbh, i would much rather move this here to avoid the unneeded extra complexity of the two repos - esp because now you see many other projects are using it, and it clearly shows value?

@birkskyum
Copy link
Member

birkskyum commented Jul 30, 2025

Taking a step back, it's not just two repos. There is the option to keep the wrapper in maplibre-native-rs, and just add a CI check here in Native that clones the wrapper repo to test it with upcoming native versions. It's a bit of an "ecosystem ci" check, which is an approach that will soon be used for Qt SDK (which prev lived in maplibre-native) and I think strikes a good balance.

It's inspired by the vite-ecosystem-ci project, which keep the vite core repo clean by making integration tests, and it's protecting as well against regressions as a monorepo would.

Without an ecosystem check like that, everything that depends on native has to be part of native core repo to be sure... maplibre-compose, maplibre-react-native, maplibre-native-qt, flutter-maplibre-gl etc., the rs bindings aren't really different in that regard, so i think it's best we try to establish a solution that'll work for other dependants too.

@CommanderStorm
Copy link
Collaborator

I see that the issues that we currently have are not very MLN specific.

Given that otherwise I would have to subscribe to all MLN issues or Bart would need to tag me (both horrible) I think keeping us separate and adding something like ecosystem CI would be nice.

The changes that I see as TODO would unnecessarily spam people, making maintaining something as high traffic as MLN more difficult.

@nyurik
Copy link
Member Author

nyurik commented Jul 30, 2025

oops, i totally misread - i thought this was the PR that added the justfile to the core MLN, not the rust wrapper. I'm fine with the wrapper being separate too

@nyurik nyurik closed this Jul 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants