Put bindgen behind a seperate feature

[weiznich]

This commit changes proj-sys to not depend on bindgen by default as that introduces a quite heavy build time dependency (libclang) which might not be there on all systems. Instead bundled bindings for the proj version build by the bundled_proj feature are provided.

[urschrei]

I don't know whether there's a prevailing best practice when it comes to sys crates, but my assumption has always been that the crate defaults to trying to use system libraries and the bundled library is an option / fallback. There's also a technical question around the bundled bindings: do they work correctly cross-platform? on 32-bit vs 64-bit?

[weiznich]

At least most of the *-sys crates I've interacted with provide bundled bindings by default as that remove the dependency on libclang.

Examples:

• libsqlite3-sys
• pq-sys
• mysqclient-sys
• openssl-sys
• netcdf-sys
• libz_sys
• gdal_sys

Some of them provide the possibility to use bindgen at build time to build your own bindings instead.

There's also a technical question around the bundled bindings: do they work correctly cross-platform? on 32-bit vs 64-bit?

That depends on the header as far as I can tell. If it's written in a cross-platform compatible way it does work otherwise there might be issues for 32-bit vs 64bit or for windows vs linux or …

Some of the crates above handle that by just providing different bindings for different platforms and conditionally including the right bindings for the target. Other provide the some feature flag to run bindgen at buildtime for that, and others do both.

[weiznich]

Is there any interest in moving this PR forward from the relevant maintainers?

[weiznich]

@urschrei @michaelkirk @lnicola What is required to move this forward?

[lnicola]

In gdal-sys we bundle pre-built bindings for every minor GDAL version and I don't think we've ran into any compatibility issues (except on Windows where I'm not convinced that people manage to use it).

I don't know the ABI compatibility story of GDAL, but I doubt that they are backwards-compatible.

[weiznich]

@urschrei @michaelkirk @lnicola This is another gentle ping if there is any interest in moving this forward

[urschrei]

@michaelkirk I'm inclined to accept this: libproj doesn't update often enough for this to cause maintainer overhead of a magnitude that would concern me (we may need to add some docs on how to do it though). WDYT?

[lnicola]

LGTM. but it would be nice to add a line to the README mentioning the version of libproj we ship the bindings for, and instructions for how to update them. For example, in gdal-sys, we use something like:

bindgen --constified-enum-module ".*" --ctypes-prefix libc --allowlist-function "(CPL|CSL|GDAL|OGR|OSR|OCT|VSI).*" wrapper.h -- $(pkg-config --cflags-only-I gdal) -fretain-comments-from-system-headers

I think we want -fretain-comments-from-system-headers for a distro-provided libproj, but we could use std::ffi instead of libc.

I'd also prefer buildtime-bindgen instead of buildtime_bindgen, but that's a trivial thing.

[michaelkirk]

In theory I'm on board. Can you also include instructions and/or a script for the maintainers to run when we want to refresh the bindings?

I think we want -fretain-comments-from-system-headers for a distro-provided libproj,

Nice - I didn't know about this flag.

I'd also prefer buildtime-bindgen instead of buildtime_bindgen

My estimation is that buildtime_bindgen is a more popular spelling, so we should leave the bikeshed painted as it is.

https://github.com/search?q=%22buildtime_bindgen%22&type=code https://github.com/search?q=%22buildtime-bindgen%22&type=code

[weiznich]

In theory I'm on board. Can you also include instructions and/or a script for the maintainers to run when we want to refresh the bindings?

I've pushed https://github.com/georust/proj/pull/191/commits/e381d58677c0de487f0c8d673b30b8e4d432b697 to add a DEVELOPMENT.md file to the root directory that contains the exact command used to generate the bindings. It matches what's currently used for the buildtime_bindgen feature. I also added a note in the build.rs file that for changes there you also need to update the command in DEVELOPMENT.md to help keeping them in sync.

[weiznich]

Seems like CI failed due to a container failure: https://github.com/georust/proj/actions/runs/9495284106/job/26167576710?pr=191

[weiznich]

@michaelkirk its certainly possible to add that as well if that's wanted by the maintainers.

That written as a more general note: It becomes really hard to keep this going due to how the communication from your side. Everytime everything seems to be resolved things stale for longer time and after that someone comes back with new requests. This PR is now open for nearly a half a year. While I can understand that time for maintaining crates is something that is always scarce, I do not think that this change is that large or complicated that it requires that much time to review and decide what to do. Given the experience of my contributions here and for the gdal bindings as well I seriously consider if it might be a better idea for us to just drop the whole thing and have our own more minimal version of that internally. After all it does look to me that you are either not interested in contributions from the outside or that you just do not have any capacity for maintaining left. Neither is a good sign for something to depend on.

[urschrei]

Speaking purely for myself, I can understand your frustration. However, I'm time-constrained (and so is every other maintainer of this repository) and I am not in a position to change that. As you're no doubt aware, the problem is inherent to open source.

That having been said, I am not interested in entertaining passive-aggressive "thinking out loud" comments about what you should do. We are not responsible for your pressing issues, and it is unhelpful for you to express your frustration in this way, given its systemic causes.

You've proposed a number of fairly large changes to the build system, and we're in the process of accepting them. You are more than welcome to help us to maintain the two libraries if you feel you have time and that you can work with us. If you'd prefer not to, you can either make your own arrangements, or wait for your changes to land.

[weiznich]

To be clear here: My main criticism is not that it takes time. That's ok, although half a year is quite a bit of time. My main criticism is that you seemingly approve changes, but do not merge the and if I ask for any progress weeks late you (as some of the maintainers) come up with yet another thing to discuss or yet another change to make. As someone that maintains several crates large than any of the gdal related crates: The only thing you manage to do with this moving goalpost behaviour is to scare away people that might help you. You as a maintainer team need to find a better solution to that!

This is especially frustrating for features like the gdal bundling feature where I reached out quite a bit beforehand to see if you would be interested in that feature and to get some feedback of the requirements.

[michaelkirk]

Should we surface this buildtime_bindgen feature in the proj crate as well?

My understanding is that most people who use proj in rust are using the proj crate, not the proj-sys crate directly. If you don't surface it to the crate that people actually use, no one will be able to opt out, right?

[weiznich]

Cargo unifies crate features across your dependency tree. That means a user can always add a dependency on proj-sys as well and enable the feature there. For sys crates cargo will also ensure that it only uses a single version of that crate.

I personally prefer keeping features like that one on the sys crate only as I expect that it will only be used by a rather small fraction of potential users (Mainly distro-maintainers that want to ensure that the bindings match the packaged version?).