Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Bump gdal-src to 3.10.0 #574

Merged
merged 9 commits into from
Nov 23, 2024
Merged

Bump gdal-src to 3.10.0 #574

merged 9 commits into from
Nov 23, 2024

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented Oct 18, 2024 •
edited
Loading

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

@lnicola lnicola changed the title Gdal src 3 10 Bump gdal-src to 3.10.0beta1 Oct 18, 2024
@lnicola
Copy link
Member Author

lnicola commented Oct 18, 2024

@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
Copy link
Contributor

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

@lnicola
Copy link
Member Author

lnicola commented Oct 20, 2024

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
Copy link
Contributor

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 lnicola changed the title Bump gdal-src to 3.10.0beta1 Bump gdal-src to 3.10.0RC1 Oct 29, 2024
@lnicola lnicola changed the title Bump gdal-src to 3.10.0RC1 Bump gdal-src to 3.10.0RC2 Oct 31, 2024
@lnicola lnicola changed the title Bump gdal-src to 3.10.0RC2 Bump gdal-src to 3.10.0 Nov 7, 2024
@lnicola lnicola marked this pull request as ready for review November 7, 2024 06:27
@lnicola
Copy link
Member Author

lnicola commented Nov 7, 2024 •
edited
Loading

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
Copy link
Member Author

lnicola commented Nov 7, 2024

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
Copy link
Member Author

lnicola commented Nov 13, 2024

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

  • exclude the VSI functions from the bindings; downside: they sound like useful functions
  • mark the problematic types as opaque; downside: it's unsound on Windows
  • refuse to use the pre-built bindings on Windows; downside: no Windows support for users who just want to load a dataset
  • generate pre-built bindings on Windows; downside: that's probably a pain

@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
Copy link
Contributor

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:

  • Use a base docker container
  • Install the library + header files
  • Install binden
  • Install gcc cross compiler toolchains for all targets that should be supported. That might include more platforms than just 64 bit Linux and windows. You also might be able to use the same binds for more than one platform (e.g. Linux + macOS or x86_64 and aarch64). For windows the mingw toolchains worked well.
  • Pass the relevant target via the extra clang flags to bindgen. The target specific should closely match that one from rust.
  • Generate all the required bindings in that container and copy them out.
  • Conditionally include the right binding file via some cfg settings + emit a compile_error for all platforms without prebuilt bindings. I would expect that you get some reports from people using these platforms after the new release of that change.

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

@lnicola
Copy link
Member Author

lnicola commented Nov 13, 2024 •
edited
Loading

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
Copy link
Contributor

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
Copy link
Contributor

@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 🤷

Anyway I filled lnicola#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
Copy link
Member Author

lnicola commented Nov 21, 2024

@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
Copy link
Contributor

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
Copy link
Member Author

lnicola commented Nov 22, 2024

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

@weiznich
Copy link
Contributor

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
Copy link
Member Author

lnicola commented Nov 22, 2024 •
edited
Loading

@weiznich please take a look at a87ff72.

@lnicola
Replace generic prebuilt bindings with target specific bindings
571709b 
* Replace generic prebuilt bindings with target specific bindings

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.
@lnicola
Fix bindings path for docs.rs and bundled
3aabd01
@weiznich
Copy link
Contributor

@lnicola Looks good to me, thanks for fixing

@lnicola
Copy link
Member Author

lnicola commented Nov 23, 2024

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.

@lnicola lnicola merged commit 2c673d6 into georust:master Nov 23, 2024
13 checks passed
@weiznich
Copy link
Contributor

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

@Atreyagaurav
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lnicola @ChristianBeilschmidt @weiznich @Atreyagaurav