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

Assume tiff to avoid unnecessary libproj builds. #92

Merged
merged 2 commits into from
Oct 20, 2021

Conversation

michaelkirk
Copy link
Member

@michaelkirk michaelkirk commented Oct 20, 2021

This is a preface to unblocking georust/geo#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
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Oct 20, 2021
@bors
Copy link
Contributor

bors bot commented Oct 20, 2021

try

Build failed:

This removes the bundled_proj_tiff feature (added in
https://github.com/georust/proj/pull/58/files), and we now 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.

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

bors try

bors bot added a commit that referenced this pull request Oct 20, 2021
@bors
Copy link
Contributor

bors bot commented Oct 20, 2021

try

Build succeeded:

@michaelkirk
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Oct 20, 2021
@bors
Copy link
Contributor

bors bot commented Oct 20, 2021

try

Build succeeded:

@michaelkirk
Copy link
Member Author

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

@urschrei
Copy link
Member

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

Shall I cut a new release and rework georust/geo#661 when this has merged?

@michaelkirk
Copy link
Member Author

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 georust/geo#661 if it's helpful: https://github.com/georust/geo/tree/mkirk/bump_proj_810

@urschrei
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 20, 2021

Build succeeded:

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.

2 participants