Conversation
lseelenbinder
left a comment
There was a problem hiding this comment.
All good other than one potential async blocking issue.
build_pbf_glyphs/src/main.rs
Outdated
| // The above utility always returns a single stack | ||
| glyphs_combined += stack.stacks[0].glyphs.len(); | ||
|
|
||
| let encoded_bytes = stack.encode_to_vec(); |
There was a problem hiding this comment.
This looks suspiciously heavy for async.
There was a problem hiding this comment.
Ahh, fair point. It's not actually an issue in the current usage (this isn't the similar library function but an internal helper at the end of build_pbf_glyphs) but yeah this probably should be on a blocking thread generally. Fixed
michaelkirk
left a comment
There was a problem hiding this comment.
Just a drive-by reviewer. I've used prost over the years and have had no surprises.
| std::env::set_var("PROTOC", protobuf_src::protoc()); | ||
|
|
||
| // Vendored protoc binary | ||
| #[cfg(feature = "protoc-vendored")] |
There was a problem hiding this comment.
My understanding is you are intending these features to be mutually exclusive - which is always a little tricky (no judgment, I've done it before too 😆)
So, my guess is you're expecting users who want to build from source to do something like:
pbf_font_tools = { default-features = false, features = ["protoc-from-src"] }
That's reasonable. Though not super plausible, it's possible that a naive user could:
pbf_font_tools = { features = ["protoc-from-src", "protoc-vendored"] }
I think more plausibly a naive user could:
pbf_font_tools = { features = ["protoc-from-src"] }
And I think they'd be surprised to find they still use the vendored protoc in that case.
A couple alternatives:
- have a check that explodes if both features are enabled
- re-organize the build script so that the default version occurs first - allowing the protoc-from-src to override it.
- delete the
protoc-vendoredfeature, and instead usenot(feature = "protoc-from-src")
I'd lean towards alternative 3, but I'm just a random reviewer and have no authority in this repo. =)
There was a problem hiding this comment.
Yes, you're correct on all points (I had considered this as a possibility, but not enough to put much thought into a "better way" since I just kindof accept that cargo features are very imperfect at this point).
I actually like option 3 a lot; lemme give that a quick try.
There was a problem hiding this comment.
Bit of a delay getting back to this after meetings, but here's why I'm not sure we can actually achieve 3.
- One request was the ability to bring your own (from a Nix user; they presumably want to have their own path specified). This is easier if the protoc features are all feature flagged.
- We'd prefer to leave vendored as the default since it "just works." It's a less than ideal DX if you have to install an external dependency OR enable a feature flag.
So while it's not ideal, I've gone for option 1 and panic the build script. I considered 2, but I think that it's probably better to be explicit and let the user know they did something silly. Presumably if they are building from source, they do NOT want to also download the binary as part of the build!
There was a problem hiding this comment.
Ah, I didn't realize BYO was an option. That totally makes sense.
nyurik
left a comment
There was a problem hiding this comment.
seems good from the first glance... TODO: adapt the whole CI pipeline we have for pmtiles...
Co-authored-by: Yuri Astrakhan <yuriastrakhan@gmail.com>
Co-authored-by: Yuri Astrakhan <yuriastrakhan@gmail.com>
This switches to prost, a tokio project, for protobuf parsing and codegen. Also cleans up a few deps. (This is NOT as ambitious of an upgrade as some other PRs, so hopefully CI passes without a hitch.)
Prost is maintained under the tokio umbrella, and has a nicer interface. I also thought it included a pure Rust
protoc, but that turned out not to be the case. However, I was still able to make the protoc selection more flexible using feature flags. I expect this will resolve the complaint in issue #21.On speed, I have seen claims that this crate is faster than some others for decoding, but I could not verify those. When running
build_pbf_glyphson this branch vsmain, there is no measurable difference (~1% variance between runs on my M1 Max). I verified the generated directory trees (sha256 hashes of the files) for the font set we use at Stadia Maps and they match.