fix(build): restore precompiled MLN builds#242
Conversation
There was a problem hiding this comment.
Pull request overview
This PR restores support for MLN_PRECOMPILE=1 builds by updating the precompiled MapLibre Native release metadata, fixing build-script include/link configuration, and adding CI coverage to prevent future regressions.
Changes:
- Bump the precompiled MapLibre Native release tag used by
MLN_PRECOMPILE=1. - Restore the vendored bridge
include/directory to the C++ bridge include path. - Link
libuvonly on non-Apple targets in the build script, and exercise precompiled builds in CI.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Cargo.toml | Updates package.metadata.mln.release to a newer precompiled MapLibre Native core tag. |
| build.rs | Restores bridge include dirs (adds include/) and adjusts libuv linking to be non-Apple only. |
| .github/workflows/ci.yml | Switches selected matrix legs to precompiled: 1 / amalgam: 1 to cover precompiled builds in CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: yuiseki <yui.matsumura@geolonia.com>
|
Thank you for referring to my PR! FYI: I think it may be helpful for |
|
@yuiseki Thanks. I switched the stringify parts I'd removed to commented-out instead🙏 |
| return std::make_unique<GeoJson>(geojson.get()); | ||
| } | ||
|
|
||
| rust::String stringify(const GeoJson& geojson) { |
There was a problem hiding this comment.
Shall we add a feature flag and disable conditionally?
There was a problem hiding this comment.
I think keeping it commented out and waiting for maplibre/maplibre-native#4345 is the safe option here, rather than adding a feature flag.
Reasons:
- A flag can't link against anything until
stringifyGeoJSONships; then we just un-comment it — no flag needed. - Gating one method — with no clear use case yet — needs a lot of plumbing.
- We're actively embracing breaking changes for now.
| tick_until_ready(|| request.is_ready()); | ||
|
|
||
| let image = request.finish().expect("geometry-fit renderer should render"); | ||
| let image = request.wait().expect("geometry-fit renderer should render"); |
There was a problem hiding this comment.
Can we name it wait_finish? Because wait is quite generic?
There was a problem hiding this comment.
I agree that wait may be a bit generic. That said, this is the blocking utility for a render request, so I think RenderRequest::wait() is still acceptable, similar to JoinHandle::join() or process::Child::wait().
There was a problem hiding this comment.
Of course, suggestions to improve the name are very welcome.
|
|
||
| // The precompiled headers tarball omits `platform/default/` headers | ||
| // (e.g. `mbgl/gfx/headless_frontend.hpp`), so add vendored fallbacks. | ||
| let root = PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap()); |
There was a problem hiding this comment.
We should do this only for amalgam. We don't need this include folder when compiling our self
There was a problem hiding this comment.
I think this is already scoped that way! It's only added inside bundle_precompiled(), so source builds don't pull it in.
| - runs-on: ubuntu-latest | ||
| backend: vulkan | ||
| precompiled: 0 | ||
| amalgam: 0 | ||
| precompiled: 1 | ||
| amalgam: 1 |
There was a problem hiding this comment.
Please add another variant to the matrix not remove a variant from ci.
If CI thakes longer because of this, this is preferable to not checking a path that we could have checked imo
There was a problem hiding this comment.
I was hesitant to grow the matrix, but I think it's the right call. Added it. Thanks!
There was a problem hiding this comment.
Yes, growing beyond 10 jobs means that one gets queued, unless github has extra capa, but that is fine.
There was a problem hiding this comment.
I am fine with this change, but I would prefer if we wait the maybe week or so that the PR in native would take. Seems like an easy fix.
Thanks @yuiseki for looking into this ❤️
|
Thanks for the review and approval 🙏❤️ A small scope note, since the GeoJSON discussion may have drawn the focus: the core fix here is the broken |
|
if there is no real usecase behind this, I am fine with removing it (incl the commnted out code) as well. |
|
Thanks! Merging this now. I'll re-enable the commented-out lines once @yuiseki's maplibre/maplibre-native#4345 lands. |
I found
MLN_PRECOMPILE=1builds are currently broken, but this was not caught by CI. This PR updates the precompiled MapLibre Native release and fixes the link setup:(Some of these changes are based on #237 by @yuiseki - thanks!)
libuvon non-Apple targets,sqlite3on Apple targets)mapbox::geojsonsymbols from the Rust bridge, since they are hidden in the amalgamGeoJson::to_json_string()- no public mbgl API yet feat(core): Add public stringifyGeoJSON to mbgl::style::conversion maplibre-native#4345Note: macOS still does not work with
MLN_PRECOMPILE=1because the amalgam (built witharmerge --keep-symbols 'mbgl.*') also hides the C++ exception RTTI symbols.