georust / gdal

Rust bindings for GDAL
https://crates.io/crates/gdal
MIT License
369 stars 94 forks source link

Bump `gdal-src` to 3.10.0 #574

Closed lnicola closed 6 days ago

lnicola commented 1 month ago
lnicola commented 1 month ago

@weiznich quick question, how do you think we should version gdal-src? Maybe 0.3.10? Because I think GDAL sometimes has breaking changes in minor bumps.

ChristianBeilschmidt commented 1 month ago

What is the idea behind using beta versions for gdal-src? Wouldn't it be better to use the latest stable one?

lnicola commented 1 month ago

I'll upgrade this to 3.10 when it comes out. I just wanted to be ready for the release and spot any potential issues.

weiznich commented 1 month ago

quick question, how do you think we should version gdal-src? Maybe 0.3.10? Because I think GDAL sometimes has breaking changes in minor bumps.

Crates like curl-sys and openssl-src use the build metadata part of the version for this.

That would mean we will end up with something like 0.1.1+gdal.3.10.0beta1 as version specifier.

lnicola commented 3 weeks ago

Hmm, not sure what to do about the layout tests. I can generate FILE and VSIStatBuf as opaque types, but that just kicks the problem down the road with pub type FILE = [u64; 27usize];. And there's a bunch of functions that use those, like VSIFOpen.

lnicola commented 3 weeks ago

Looking into the timespec error, it's a struct with two c_longs, that's (understandably) 16 bytes on Linux and 8 bytes on Windows. For now, we can either omit those structs or disable the layout tests.

lnicola commented 2 weeks ago

Okay, I'd like to find a way forward here. I think we have a couple of options:

@weiznich since you added the Windows CI for gdal-src, are you using this crate on that platform? How do you feel about this?

For context: bindgen generates some layout tests, that used to fail on Windows, which is why the tests don't get run. But bindgen 0.70 uses std::mem::size_of in a const context, so the tests fail to compile now. It only exposes a pre-existing problem.

weiznich commented 2 weeks ago

Yes we are using gdal on windows. I personally would prefer to have prebuilt bindings for common platforms, which should include windows.

I had a similar problem with the bindings for mysqlclient-sys. There I could solve the generation problem by just performing the following steps:

If necessary I can try to help with that as soon as I'm back at work.

lnicola commented 2 weeks ago

Well, I did try BINDGEN_EXTRA_CLANG_ARGS="-target x86_64-pc-windows-gnu", but it can't find bits/libc-header-start.h, which doesn't seem to be provided by any Windows toolchain:

$ apt-file find bits/libc-header-start.h
libc6-dev: /usr/include/x86_64-linux-gnu/bits/libc-header-start.h
libc6-dev-amd64-cross: /usr/x86_64-linux-gnu/include/bits/libc-header-start.h
libc6-dev-arc-cross: /usr/arc-linux-gnu/include/bits/libc-header-start.h
libc6-dev-arm64-cross: /usr/aarch64-linux-gnu/include/bits/libc-header-start.h
libc6-dev-armel-cross: /usr/arm-linux-gnueabi/include/bits/libc-header-start.h
libc6-dev-armhf-cross: /usr/arm-linux-gnueabihf/include/bits/libc-header-start.h
libc6-dev-hppa-cross: /usr/hppa-linux-gnu/include/bits/libc-header-start.h
libc6-dev-i386-cross: /usr/i686-linux-gnu/include/bits/libc-header-start.h
libc6-dev-loong64-cross: /usr/loongarch64-linux-gnu/include/bits/libc-header-start.h
libc6-dev-m68k-cross: /usr/m68k-linux-gnu/include/bits/libc-header-start.h
libc6-dev-mips-cross: /usr/mips-linux-gnu/include/bits/libc-header-start.h
libc6-dev-mips64-cross: /usr/mips64-linux-gnuabi64/include/bits/libc-header-start.h
libc6-dev-mips64el-cross: /usr/mips64el-linux-gnuabi64/include/bits/libc-header-start.h
libc6-dev-mips64r6-cross: /usr/mipsisa64r6-linux-gnuabi64/include/bits/libc-header-start.h
libc6-dev-mips64r6el-cross: /usr/mipsisa64r6el-linux-gnuabi64/include/bits/libc-header-start.h
libc6-dev-mipsel-cross: /usr/mipsel-linux-gnu/include/bits/libc-header-start.h
libc6-dev-mipsn32-cross: /usr/mips64-linux-gnuabin32/include/bits/libc-header-start.h
libc6-dev-mipsn32el-cross: /usr/mips64el-linux-gnuabin32/include/bits/libc-header-start.h
libc6-dev-mipsn32r6-cross: /usr/mipsisa64r6-linux-gnuabin32/include/bits/libc-header-start.h
libc6-dev-mipsn32r6el-cross: /usr/mipsisa64r6el-linux-gnuabin32/include/bits/libc-header-start.h
libc6-dev-mipsr6-cross: /usr/mipsisa32r6-linux-gnu/include/bits/libc-header-start.h
libc6-dev-mipsr6el-cross: /usr/mipsisa32r6el-linux-gnu/include/bits/libc-header-start.h
libc6-dev-powerpc-cross: /usr/powerpc-linux-gnu/include/bits/libc-header-start.h
libc6-dev-ppc64-cross: /usr/powerpc64-linux-gnu/include/bits/libc-header-start.h
libc6-dev-ppc64el-cross: /usr/powerpc64le-linux-gnu/include/bits/libc-header-start.h
libc6-dev-riscv64-cross: /usr/riscv64-linux-gnu/include/bits/libc-header-start.h
libc6-dev-s390x-cross: /usr/s390x-linux-gnu/include/bits/libc-header-start.h
libc6-dev-sh4-cross: /usr/sh4-linux-gnu/include/bits/libc-header-start.h
libc6-dev-sparc64-cross: /usr/sparc64-linux-gnu/include/bits/libc-header-start.h
libc6-dev-x32-cross: /usr/x86_64-linux-gnux32/include/bits/libc-header-start.h
libc6.1-dev-alpha-cross: /usr/alpha-linux-gnu/include/bits/libc-header-start.h

(same for x86_64-pc-windows-msvc)

EDIT: it's ClangDiagnostic("/usr/include/stdio.h:28:10: fatal error: 'bits/libc-header-start.h' file not found\n"), so maybe it's picking up the wrong sysroot.

$ clang --target=x86_64-pc-windows-gnu --sysroot=/usr/share/mingw-w64 -isysroot=/usr/share/mingw-w64/include -isystem /usr/include gdal-sys/wrapper.h
In file included from gdal-sys/wrapper.h:5:
In file included from /usr/include/cpl_atomic_ops.h:18:
In file included from /usr/include/cpl_port.h:103:
/usr/include/stdio.h:28:10: fatal error: 'bits/libc-header-start.h' file not found
   28 | #include <bits/libc-header-start.h>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
$ ls /usr/share/mingw-w64/include/stdio.h 
/usr/share/mingw-w64/include/stdio.h
weiznich commented 2 weeks ago

So I gave this a try locally and the following chain of commands seem to work for me:

sudo docker run -it --rm rust:1.82.0 bash
# everything else is run inside the container
apt update
apt install libgdal-dev libclang-dev mingw-w64
cargo install bindgen-cli
rustup component add rustfmt

wget https://github.com/georust/gdal/raw/refs/heads/master/gdal-sys/wrapper.h
# you might need to add whatever flags are set by default for gdal by the project setup
# these are only the minimal set of flags to get it working with the rust image
bindgen wrapper.h -- -I /usr/include/gdal -target x86_64-pc-windows-gnu
weiznich commented 1 week ago

@lnicola I had another more detailed look at this and it turns out I get the same error if I use the osgeo containers and gdal >=3.9. In the end I "solved" this by just removing the system headers manually, after all it's only an ephemeral container :shrug:

Anyway I filled https://github.com/lnicola/gdal/pull/1 which adds some documentation how to generate this bindings for all gdal versions + adds the actual bindings for 32 + 64 platforms + linux/macos + windows. Hopefully that helps getting the next gdal-rs release finally over the finish line.

lnicola commented 1 week ago

@weiznich uh, yesterday I managed to run bindgen for Windows in the 3.10 container using mingw-w64. I assumed some other package I had installed was causing that error.

weiznich commented 1 week ago

Seems like I missed this location where bindings are also loaded: https://github.com/georust/gdal/blob/master/gdal-sys/build.rs#L84-L87

It might be useful to factor out the target detection that was added in 759df53 in a separate function + call that from there as well? I can add another update for that if you like

lnicola commented 1 week ago

For docs.rs I think we just want to pick one version of the pre-built bindings, like the Linux x64 one.

weiznich commented 1 week ago

The problem there is that this is currently used for docs.rs and the bundled source code. The later variant requires platform dependent bindings.

lnicola commented 1 week ago

@weiznich please take a look at https://github.com/georust/gdal/pull/574/commits/a87ff7262ec6c5c7f7c95dd8cd2be1bef924d0d2.

weiznich commented 6 days ago

@lnicola Looks good to me, thanks for fixing

lnicola commented 6 days ago

I guess I'll merge this later today.

I know you're waiting for a new release, but I hope to get in #581, #584 and maybe #585. And I can't publish a pre-release version to crates.io.

weiznich commented 3 days ago

@lnicola Is there anything I can help with these PR's?

Atreyagaurav commented 2 days ago

@lnicola let me know if I need to do anything about #584 and #585 as well. I'm relatively free today and tomorrow.