Update to use proj 0.23.1, using libproj 8.1.0 #661

urschrei · 2021-08-18T18:44:19Z

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.

urschrei · 2021-08-18T20:04:22Z

bors try

bors · 2021-08-18T20:13:39Z

try

Build failed:

ci result

urschrei · 2021-08-18T20:14:22Z

Hmmmm missing cmake in the base images?

michaelkirk · 2021-08-19T17:43:05Z

Hmmmm missing cmake in the base images?

The geo-ci dockerfile copies the proj installation from libproj-builder:

https://github.com/georust/docker-images/blob/master/rust-1.53/geo-ci.Dockerfile#L29

So if it's trying to build proj, something has gone wrong.

michaelkirk · 2021-08-19T17:47:16Z

One thought: are you sure that you'd built and tagged the latest libproj-builder:rust-1.53 (with proj 8) before rebuilding and tagging the geo-ci:rust-1.53 container?

...I have no idea how to audit this =(

I'll try to reproduce it locally.

urschrei · 2021-08-19T18:44:34Z

I thought I had…

michaelkirk · 2021-08-19T20:09:40Z

--- stderr
  feature flags specified source build  👀 👀 👀 👀 
  building libproj from source
  thread 'main' panicked at '
  failed to execute command: No such file or directory (os error 2)
  is `cmake` not installed?

That stderr line is from:

16 #[cfg(not(feature = "nobuild"))]
 17 fn main() -> Result<(), Box<dyn std::error::Error>> {
 18     let include_path = if cfg!(feature = "bundled_proj") {
 19         eprintln!("feature flags specified source build");
 20         build_from_source()?                                   
 21     } else {

I wonder why we're triggering that?

michaelkirk · 2021-08-19T20:28:51Z

Oh, I guess I didn't realize (or maybe forgot) that using geo --features=proj-network will build the bundled_proj:

geo/Cargo.toml

proj-network = ["use-proj", "proj/network"]

proj/Cargo.toml

network = ["bundled_proj_tiff", "reqwest"]

and

bundled_proj_tiff = [ "proj-sys/bundled_proj_tiff" ]

proj-sys/Cargo.toml

bundled_proj_tiff = ["bundled_proj"]

So I guess this is intended. I'm not sure why this was added - I guess maybe it's hard to probe whether the existing proj installation has the tiff feature?

Maybe in the immediate, the thing to do is simply to install cmake on geo-ci, as @urschrei suggested long ago. 😬

urschrei · 2021-08-19T21:28:32Z

It's the easier option for now, for sure.

michaelkirk · 2021-10-19T00:09:11Z

Sorry that this has sat here so long @urschrei!

I confess, I'm still kind of confused as to why we now need to build proj from source when we (apparently?) didn't previously need to.

One thing that might help me trace this back - can you push up a proj-0.20.3 tag?

michaelkirk · 2021-10-19T00:16:58Z

BTW, I did go to add cmake to the geo-ci base docker image, but then the build failed because it needed sqlite... and presumably all the other proj deps.

Ultimately I'd be OK with having just one heavy base docker image for ci, it's no big deal.

But the primary reason it gives me pause was because now I'm wondering if we'd be newly requiring geo users to have to build proj from source even if they already have a compatible version installed.

For example, a (very) cursory look shows that debian (sid) and macos homebrew include libtiff. It seems likes those users shouldn't need to build libproj from source if we get things set up nicely.

michaelkirk · 2021-10-19T00:59:28Z

Hm... maybe this is the history: georust/proj#58

Seems like we first added a way to disable the building of tiff in hopes of getting proj building on M1, but we've since tracked down a different root cause and everything is now peachy w/ aarch64 and libproj.

Since tiff is enabled by default by libproj... would it be too bold to deprecate that feature and assume all people are either:

using a reasonably default proj install that includes libtiff
or if they're doing something fancy, they're on their own and will have to decipher the trail of error messages.

I guess weighing the downsides of that vs. requiring everyone who uses geo w/ proj to compile libproj from source...

urschrei · 2021-10-19T09:10:06Z

I think it's reasonable to assume that most people with an existing libproj also have libtiff, and anyone else is comfortable hacking.

urschrei · 2021-10-19T09:20:46Z

(I also pushed a tag for 0.20.3)

92: Assume `tiff` to avoid unnecessary libproj builds. r=urschrei a=michaelkirk 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. Co-authored-by: Michael Kirk <michael.code@endoftheworl.de>

michaelkirk · 2021-10-27T23:02:29Z

ping @urschrei - I'm happy to take this one over if you're busy.

urschrei · 2021-11-02T14:12:59Z

Let's see if this will merge even though cargo-tarpaulin is failing with an error

urschrei · 2021-11-02T14:13:07Z

bors r+

661: Update to use proj 0.23.1, using libproj 8.1.0 r=urschrei a=urschrei - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Co-authored-by: Stephan Hügel <shugel@tcd.ie>

bors · 2021-11-02T14:21:23Z

Build failed:

ci result

urschrei · 2021-11-02T14:26:16Z

bors try

[skip coverage]

urschrei · 2021-11-02T14:45:27Z

OK! But will the merge commit skip the coverage check 🧐?

urschrei · 2021-11-02T14:46:23Z

bors r+

661: Update to use proj 0.23.1, using libproj 8.1.0 r=urschrei a=urschrei - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Co-authored-by: Stephan Hügel <shugel@tcd.ie>

urschrei · 2021-11-02T14:49:53Z

OK, now what? Manual merge?

bors · 2021-11-02T15:11:19Z

Build failed:

ci result

michaelkirk · 2021-11-02T16:48:18Z

In the immediate, I'm totally fine with a manual merge. I'm going to open a separate issue about tarpaulin (here: #676)

bors · 2021-11-05T20:04:37Z

try

Timed out.

bors bot added a commit that referenced this pull request Aug 18, 2021

Try #661:

690c310

urschrei changed the title ~~Update to use proj 0.23.0, using libproj 8.1.0~~ Update to use proj 0.23.1, using libproj 8.1.0 Aug 18, 2021

michaelkirk mentioned this pull request Oct 20, 2021

Assume tiff to avoid unnecessary libproj builds. georust/proj#92

Merged

urschrei force-pushed the bump_proj_810 branch 2 times, most recently from 6e2077c to 7beced3 Compare November 2, 2021 14:12

bors bot added a commit that referenced this pull request Nov 2, 2021

Try #661:

740ad6c

urschrei force-pushed the bump_proj_810 branch from 48b808e to 2e50e11 Compare November 2, 2021 14:31

Update to use proj 0.23.1, using libproj 8.1.0

e0aa25f

[skip coverage]

urschrei force-pushed the bump_proj_810 branch from 2e50e11 to e0aa25f Compare November 2, 2021 14:38

urschrei merged commit 61ad69f into master Nov 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to use proj 0.23.1, using libproj 8.1.0 #661

Update to use proj 0.23.1, using libproj 8.1.0 #661

urschrei commented Aug 18, 2021

urschrei commented Aug 18, 2021

bors bot commented Aug 18, 2021

urschrei commented Aug 18, 2021

michaelkirk commented Aug 19, 2021

michaelkirk commented Aug 19, 2021 •

edited

Loading

urschrei commented Aug 19, 2021

michaelkirk commented Aug 19, 2021 •

edited

Loading

michaelkirk commented Aug 19, 2021 •

edited

Loading

urschrei commented Aug 19, 2021

michaelkirk commented Oct 19, 2021

michaelkirk commented Oct 19, 2021 •

edited

Loading

michaelkirk commented Oct 19, 2021 •

edited

Loading

urschrei commented Oct 19, 2021

urschrei commented Oct 19, 2021

michaelkirk commented Oct 27, 2021

urschrei commented Nov 2, 2021

urschrei commented Nov 2, 2021

bors bot commented Nov 2, 2021

urschrei commented Nov 2, 2021

urschrei commented Nov 2, 2021

urschrei commented Nov 2, 2021

urschrei commented Nov 2, 2021

bors bot commented Nov 2, 2021

michaelkirk commented Nov 2, 2021 •

edited

Loading

bors bot commented Nov 5, 2021

Update to use proj 0.23.1, using libproj 8.1.0 #661

Update to use proj 0.23.1, using libproj 8.1.0 #661

Conversation

urschrei commented Aug 18, 2021

urschrei commented Aug 18, 2021

bors bot commented Aug 18, 2021

try

urschrei commented Aug 18, 2021

michaelkirk commented Aug 19, 2021

michaelkirk commented Aug 19, 2021 • edited Loading

urschrei commented Aug 19, 2021

michaelkirk commented Aug 19, 2021 • edited Loading

michaelkirk commented Aug 19, 2021 • edited Loading

urschrei commented Aug 19, 2021

michaelkirk commented Oct 19, 2021

michaelkirk commented Oct 19, 2021 • edited Loading

michaelkirk commented Oct 19, 2021 • edited Loading

urschrei commented Oct 19, 2021

urschrei commented Oct 19, 2021

michaelkirk commented Oct 27, 2021

urschrei commented Nov 2, 2021

urschrei commented Nov 2, 2021

bors bot commented Nov 2, 2021

urschrei commented Nov 2, 2021

urschrei commented Nov 2, 2021

urschrei commented Nov 2, 2021

urschrei commented Nov 2, 2021

bors bot commented Nov 2, 2021

michaelkirk commented Nov 2, 2021 • edited Loading

bors bot commented Nov 5, 2021

try

michaelkirk commented Aug 19, 2021 •

edited

Loading

michaelkirk commented Aug 19, 2021 •

edited

Loading

michaelkirk commented Aug 19, 2021 •

edited

Loading

michaelkirk commented Oct 19, 2021 •

edited

Loading

michaelkirk commented Oct 19, 2021 •

edited

Loading

michaelkirk commented Nov 2, 2021 •

edited

Loading