Replace generic prebuilt bindings with target specific bindings #1
Conversation
This commit replaces the generic gdal bindings with architecture specific ones. This is necessary as bindgen 0.71 introduced a feature that essentially turned the size assertions into a compile time check. This pointed to several potential mismatches between the bindings and the actual type definitions. This commit now introduces different bindings for 32 and 64 bit platforms. Additionally it differentiates between windows and linux/macos bindings. For any unsupported platform a build error is generated that the built_time bindgen feature needs to be used there. Finally it adds documentation on how to generate these bindings.
| let is_windows = std::env::var("CARGO_CFG_WINDOWS").is_ok(); | ||
| let ptr_size = std::env::var("CARGO_CFG_TARGET_POINTER_WIDTH").expect("Set by cargo"); | ||
|
|
||
| let binding_name = match (target_arch.as_str(), ptr_size.as_str(), is_windows) { |
There was a problem hiding this comment.
Can we use env::var("TARGET") here?
There was a problem hiding this comment.
I wouldn't want to do that as it makes the match below much longer
As currently written we accept anything that's a 64-bit system independently from the OS as long as it uses x86_64 ar aarch64 as architecture. If we would use targets there this would become much more complicated as you would need to all possible target variants, which might be ever changing.
gdal-sys/build.rs
Outdated
| let binding_path = PathBuf::from(format!( | ||
| "prebuilt-bindings/gdal_{}_{}.rs", | ||
| version.major, version.minor | ||
| "prebuilt-bindings/{}.{}/{binding_name}", |
There was a problem hiding this comment.
Nit: I can't test it right now, but I don't know if Explorer shows directory extensions by default. Maybe we should stick to {}_{}.
There was a problem hiding this comment.
I've just moved the files in f49cb3a
DEVELOPMENT.md
Outdated
| # Generating Binding | ||
|
|
||
| ```bash | ||
| docker run -it --rm -v ./gdal-sys:/gdal_sys ghcr.io/osgeo/gdal:ubuntu-full-$GDAL_VERSION /bash |
There was a problem hiding this comment.
I didn't try this, but it suspect it will work fine:
| docker run -it --rm -v ./gdal-sys:/gdal_sys ghcr.io/osgeo/gdal:ubuntu-full-$GDAL_VERSION /bash | |
| docker run -it --rm -v ./gdal-sys:/gdal_sys ghcr.io/osgeo/gdal:ubuntu-small-$GDAL_VERSION /bash |
There was a problem hiding this comment.
That image seems to be sufficient, changed the docs in f49cb3a
DEVELOPMENT.md
Outdated
| apt install -y libclang-dev mingw-w64 gcc gcc-i686-linux-gnu pkg-config | ||
|
|
||
| # install bindgen | ||
| cargo install bindgen-cli |
There was a problem hiding this comment.
If we grab the binary from e.g. https://github.com/rust-lang/rust-bindgen/releases/tag/v0.70.1, we won't even need to install Rust, right? That would be nice, since I kept getting throttled.
There was a problem hiding this comment.
That works. It's implemented in f49cb3a
|
Thanks, this is great! I meant to do it, but you were faster :-). |
This commit replaces the generic gdal bindings with architecture specific ones. This is necessary as bindgen 0.71 introduced a feature that essentially turned the size assertions into a compile time check. This pointed to several potential mismatches between the bindings and the actual type definitions.
This commit now introduces different bindings for 32 and 64 bit platforms. Additionally it differentiates between windows and linux/macos bindings. For any unsupported platform a build error is generated that the built_time bindgen feature needs to be used there.
Finally it adds documentation on how to generate these bindings.
CHANGES.mdif knowledge of this change could be valuable to users.