georust / proj

Rust bindings for the latest stable release of PROJ
https://docs.rs/proj
Apache License 2.0
137 stars 45 forks source link

Assume `tiff` to avoid unnecessary libproj builds. #92

Closed michaelkirk closed 2 years ago

michaelkirk commented 2 years ago

This is a preface to unblocking https://github.com/georust/geo/pull/661 (I've integrated these changes with the geo crate here: https://github.com/georust/geo/tree/mkirk/bump_proj_810)


This removes the bundled_proj_tiff feature (added in https://github.com/georust/proj/pull/58/files), and we now, once again, assume all proj installations support tiff. Meaning the user either:

  1. If the user has a recent system installation of libproj, it supports libtiff.
  2. Else if the user has no recent system installation of libproj, build.rs will compile it from source, which requires linking in libtiff

The previous "bundled_proj_tiff" unfortunately conflated needing tiff with needing to build from source, even though most system proj installs will already support tiff.

Longer explanation

proj-sys requires a compatible libproj installation. build.rs will either use a pre-existing system installation of libproj or build libproj from source. Building from source takes a while, so it's preferable to use the system installation if it's compatible.

tiff support is used by libproj's network grid. tiff support is required by libproj by default, though it's possible to opt out.

When Apple first released aarch64 (M1), libtiff was failing to build, so as a stop gap we added a "bundled_proj_tiff" feature which:

  1. forced a from-source build
  2. explicitly enabled tiff support, else tiff support would be disabled, allowing us to build the proj crate on aarch64

The underlying build failures in libtiff have now been fixed, so the original motivation for this feature no longer exists.

There was a cost associated with keeping it - unnecessarily triggering source builds from, e.g. georust/geo crate, whose proj/network feature wants to ensure tiff is enabled. Given that most installations will have this feature, I think it will give most of our users a better experience if we can avoid the compile.

To be clear, there is potential downside with this approach. It's still conceivable that environments exists where tiff is not, or can not be installed, but weighing that against the more common case of having libproj installed with the default configuration, and I think this approach wins.

If this poops on too many parties, we can revisit something like the former behavior in a way that's less deleterious to the default use case - e.g. have a tiff feature that will still use the local install if it supports tiff.

michaelkirk commented 2 years ago

bors try

bors[bot] commented 2 years ago

try

Build failed:

michaelkirk commented 2 years ago

bors try

bors[bot] commented 2 years ago

try

Build succeeded:

michaelkirk commented 2 years ago

bors try

bors[bot] commented 2 years ago

try

Build succeeded:

michaelkirk commented 2 years ago

Ok! I believe this is ready for review. PTAL @urschrei

urschrei commented 2 years ago

LGTM. Thanks for the thorough rationale around the PR, too – we'll hopefully never need to refer to it, but erring on the side of caution is 100 % warranted for -sys crates.

urschrei commented 2 years ago

Shall I cut a new release and rework https://github.com/georust/geo/pull/661 when this has merged?

michaelkirk commented 2 years ago

Shall I cut a new release

Yes please! Or I'm available if you'd like to delegate.

rework georust/geo#661 when this has merged?

Yes please, or if you're busy I can follow up. Feel free to crib from my rebase+adaptation of https://github.com/georust/geo/pull/661 if it's helpful: https://github.com/georust/geo/tree/mkirk/bump_proj_810

urschrei commented 2 years ago

bors r+

bors[bot] commented 2 years ago

Build succeeded: