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.

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

Use bindgen for gdal-sys #55

Merged
merged 21 commits into from Mar 10, 2018
Merged

Use bindgen for gdal-sys #55

merged 21 commits into from Mar 10, 2018

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented Feb 16, 2018

Closes #50
Closes #56

TODO:

  • figure out how to find the GDAL include path
  • clean up bindgen config
  • choose between libc and std::os::raw
  • see if it makes sense to split the bindings into sub-modules
  • decide whether to use the modules for enums
  • use modules for the remaining enums
  • pick again between 1 and TRUE
  • clean up the pointer casts
  • bring back re-exports in submodules
  • see where the generated file should live
  • figure out the Windows story
  • decide whether to use bindgen or commit the generated file
  • decide whether to use the system GDAL headers
  • figure out why my headers are missing parameter names
  • try to use the generated handle types instead of *mut c_void
  • consider replacing WkbType with OGRwkbGeometryType
  • decide whether we really want to do this
  • do something about OGRErr
  • look into i32 vs. u32 for enums
  • allow setting the include and link paths
  • allow static GDAL builds
  • update gdal-sys build instructions
  • bump version

@jdroenner
Copy link
Member

This is really great work :)

It was not easy to get the crate building on Windows with mingw as well as the MS tool-chain. So please make sure that it still builds on Windows. If you don't have a Windows system around, just tell me and i will test it.

@lnicola
Copy link
Member Author

lnicola commented Feb 17, 2018

Thanks, but I'm pretty sure it's no longer working. Unfortunately, I don't have a way to test it, neither on MSVC, nor in MinGW. So it would be good if you could look into it.

One weird thing is that I no longer need a cargo:rustc-link-lib declaration (cargo outputs a warning). I expect the same is true on MinGW. The pkg-config part might work there, but it won't know about GDAL_HOME, so it will probably be a no-op.

@jdroenner
Copy link
Member

No problem. I will test it later today :)

@jdroenner
Copy link
Member

Building works but the tests won't run. Will try to fix it tomorrow....

@jdroenner
Copy link
Member

It appears to be a problem with my machine or windows in general. The test are not running on master, too :(

@lnicola
Copy link
Member Author

lnicola commented Feb 20, 2018

Are you getting any error, either as a message box or in Event Viewer? Maybe it can't find the GDAL DLL?

In the meanwhile, do you have any thoughts about the remaining TODO items?

@jdroenner
Copy link
Member

see if it makes sense to split the bindings into sub-modules

I don't know if it makes sense to split the bindings. What modules would that be? Since GDAL 2.x OGR and GDAL are merging...

decide whether to use bindgen or commit the generated file

This is a bit more complicated. I guess we can do a new release for new GDAL releases and running bindgen is not really required for each build...

consider replacing WkbType with OGRwkbGeometryType

This would be the way to go. I think we already use all other enums.

decide whether we really want to do this

we will never be able to cover all of GDAL if we create the bindings by hand

@lnicola
Copy link
Member Author

lnicola commented Feb 21, 2018

I don't know if it makes sense to split the bindings. What modules would that be? Since GDAL 2.x OGR and GDAL are merging...

I didn't realize this.

I guess we can do a new release for new GDAL releases and running bindgen is not really required for each build...

Or we can do both with a feature. But I'm worried abut compatibility. Will old binding work with a newer GDAL? Will new bindings work with an older GDAL as long as the application isn't calling any missing method? In the latter case, won't the OS loader complain, at least on Windows?

I looked at libsqlite3-sys, and it does this: https://github.com/jgallagher/rusqlite/blob/master/libsqlite3-sys/Cargo.toml#L13.

@jdroenner
Copy link
Member

Maybe we should go with the features approach. PyGDAL has a binding for every release but i'm not yet convinced that this is required. GDAL releases are very conservative in my experience.

@jdroenner
Copy link
Member

jdroenner commented Feb 21, 2018

surprisingly it works with the gnu toolchain on windows on a different machine.
It requires installing clang (that was expected) and adding .clang_arg("-IC:\\OSGeo4W64\\include") to the bindgen builder.

The MSVC toolchain does not work but i think i can figure it out with a bit more time.
C:\OSGeo4W64\include/cpl_port.h:137:10: fatal error: 'stdio.h' file not found

@lnicola
Copy link
Member Author

lnicola commented Feb 21, 2018

surprisingly it works with the gnu toolchain on windows on a different machine.
It requires installing clang (that was expected) and adding .clang_arg("-IC:\OSGeo4W64\include") to the bindgen builder.

Maybe we could bundle at least some bindings, so people don't have to deal with that?

The MSVC toolchain does not work but i think i can figure it out with a bit more time.

Ouch. So clang is not finding the VS include path.

@jdroenner
Copy link
Member

jdroenner commented Feb 23, 2018

I just reinstalled the VS build tools and now it also works with the MSVC tool chain 😄
But this time only the bindgen version is working... Both, MSVC and GNU toolchains won't build with the pre-generated bindings.

C:...\target\debug\deps\gdal-b8b9d3d1e4a9112e.1d37qlmqawo0lpmo.rcgu.o: In function gdal::metadata::Metadata::description::h12a3509c01c93404:
C:.../src/metadata.rs:11: undefined reference to GDALGetDescription

Which version of GDAL did you use to generate the bindings?

@jdroenner
Copy link
Member

Uuh this was really obvious...

You have to run the #[cfg(windows)] part regardless if you are using bindgen or use pre-generated bindings.
We should move it into a special method prepare_windows and call it first in the main method #[cfg(windows)] prepare_windows();.

@lnicola
Copy link
Member Author

lnicola commented Mar 4, 2018

I've been trying to make this work for multiple versions of GDAL, but I ran into rust-lang/rust-bindgen#1267.

@jdroenner
Copy link
Member

great! could you add the configurable include paths? I guess this will also allow to build on windows.

@lnicola
Copy link
Member Author

lnicola commented Mar 6, 2018

Yes, I'll try to look into it tomorrow.

@lnicola
Copy link
Member Author

lnicola commented Mar 7, 2018

@jdroenner, @weiznich, can you take a look at 9bde2eb?

@jdroenner
Copy link
Member

i will try it later today when i have access to a windows pc.

@jdroenner
Copy link
Member

all i get is "error: could not find native static library gdal_i.lib, perhaps an -L flag is missing?"

@lnicola
Copy link
Member Author

lnicola commented Mar 7, 2018

Can you add a print or panic! here to see what path it picks?

@lnicola
Copy link
Member Author

lnicola commented Mar 7, 2018

Ah, I think it uses gdal_i.lib instead of gdal_i.

@jdroenner
Copy link
Member

Its working now 👍 Great work!

@lnicola lnicola changed the title [WIP] Use bindgen for gdal-sys Use bindgen for gdal-sys Mar 8, 2018
@lnicola
Copy link
Member Author

lnicola commented Mar 8, 2018

Okay, so I've gone through my TODO list.

@jdroenner
Copy link
Member

This is really great 👍 Thanks a lot!
I guess we need to release a new version of rust-gdal as well :)

@jdroenner jdroenner merged commit 615267b into georust:master Mar 10, 2018
@lnicola
Copy link
Member Author

lnicola commented Mar 10, 2018

As a test, I tried to port https://github.com/t-rex-tileserver/t-rex to the new version. It was trivial, but I had to add a reference to gdal-sys because gdal_sys::OGRwkbGeometryType isn't re-exported.

Should we re-export these? It might be nicer for the users.

@lnicola
Copy link
Member Author

lnicola commented Mar 10, 2018

Oh, it actually is. Please ignore my last comment.

@lnicola
Copy link
Member Author

lnicola commented May 27, 2018

The pre-built files can be obtained with:

bindgen --constified-enum-module ".*" --ctypes-prefix libc --whitelist-function "(CPL|GDAL|OGR|OSR|OCT|VSI).*" wrapper.h

For some reason, that won't generate the docstrings. In practice, I used something like:

bindgen --constified-enum-module ".*" --ctypes-prefix libc --whitelist-function "(CPL|GDAL|OGR|OSR|OCT|VSI).*" wrapper.h -- -I ~/Downloads/gdal/gdal-2.3/usr/include >! prebuilt-bindings/gdal_2.3.rs

See rust-lang/rust-bindgen#1265.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants