georust / proj

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

Put reqwest behind optional network feature #39

Closed michaelkirk closed 4 years ago

michaelkirk commented 4 years ago

The network stuff adds quite a bit to proj dependencies and build times.

Considering the developer has to opt in to networking anyway by calling enable_network on the context, what would you think about making them jump through one more hoop to make things better for the default user?

With reqwest: builds 166 crates

cargo build --release --features network
1019.33s user 22.84s system 658% cpu 2:38.36 total

Without reqwest: builds 81 crates

cargo build --release  
446.73s user 9.64s system 670% cpu 1:08.02 total

If you think it's a good idea, I can open a PR to plumb this feature through geo.

dependency tree:

proj v0.20.3 (/Users/mkirk/src/georust/proj)
├── geo-types v0.6.0
│   ├── approx v0.3.2
│   │   └── num-traits v0.2.12
│   │       [build-dependencies]
│   │       └── autocfg v1.0.0
│   └── num-traits v0.2.12 (*)
├── libc v0.2.73
├── num-traits v0.2.12 (*)
├── proj-sys v0.18.2
│   [build-dependencies]
│   ├── bindgen v0.52.0
│   │   ├── bitflags v1.2.1
│   │   ├── cexpr v0.3.6
│   │   │   └── nom v4.2.3
│   │   │       └── memchr v2.3.3
│   │   │       [build-dependencies]
│   │   │       └── version_check v0.1.5
│   │   ├── cfg-if v0.1.10
│   │   ├── clang-sys v0.28.1
│   │   │   ├── glob v0.3.0
│   │   │   ├── libc v0.2.73
│   │   │   └── libloading v0.5.2
│   │   │       [build-dependencies]
│   │   │       └── cc v1.0.58
│   │   │   [build-dependencies]
│   │   │   └── glob v0.3.0
│   │   ├── clap v2.33.1
│   │   │   ├── ansi_term v0.11.0
│   │   │   ├── atty v0.2.14
│   │   │   │   └── libc v0.2.73
│   │   │   ├── bitflags v1.2.1
│   │   │   ├── strsim v0.8.0
│   │   │   ├── textwrap v0.11.0
│   │   │   │   └── unicode-width v0.1.8
│   │   │   ├── unicode-width v0.1.8
│   │   │   └── vec_map v0.8.2
│   │   ├── env_logger v0.7.1
│   │   │   ├── atty v0.2.14 (*)
│   │   │   ├── humantime v1.3.0
│   │   │   │   └── quick-error v1.2.3
│   │   │   ├── log v0.4.11
│   │   │   │   └── cfg-if v0.1.10
│   │   │   ├── regex v1.3.9
│   │   │   │   ├── aho-corasick v0.7.13
│   │   │   │   │   └── memchr v2.3.3
│   │   │   │   ├── memchr v2.3.3
│   │   │   │   ├── regex-syntax v0.6.18
│   │   │   │   └── thread_local v1.0.1
│   │   │   │       └── lazy_static v1.4.0
│   │   │   └── termcolor v1.1.0
│   │   ├── lazy_static v1.4.0
│   │   ├── lazycell v1.2.1
│   │   ├── log v0.4.11 (*)
│   │   ├── peeking_take_while v0.1.2
│   │   ├── proc-macro2 v1.0.19
│   │   │   └── unicode-xid v0.2.1
│   │   ├── quote v1.0.7
│   │   │   └── proc-macro2 v1.0.19 (*)
│   │   ├── regex v1.3.9 (*)
│   │   ├── rustc-hash v1.1.0
│   │   ├── shlex v0.1.1
│   │   └── which v3.1.1
│   │       └── libc v0.2.73
│   ├── cmake v0.1.44
│   │   └── cc v1.0.58
│   ├── flate2 v1.0.16
│   │   ├── cfg-if v0.1.10
│   │   ├── crc32fast v1.2.0
│   │   │   └── cfg-if v0.1.10
│   │   ├── libc v0.2.73
│   │   └── miniz_oxide v0.4.0
│   │       └── adler v0.2.3
│   ├── pkg-config v0.3.18
│   └── tar v0.4.29
│       ├── filetime v0.2.10
│       │   ├── cfg-if v0.1.10
│       │   └── libc v0.2.73
│       ├── libc v0.2.73
│       └── xattr v0.2.2
│           └── libc v0.2.73
├── reqwest v0.10.6
│   ├── base64 v0.12.3
│   ├── bytes v0.5.6
│   ├── encoding_rs v0.8.23
│   │   └── cfg-if v0.1.10
│   ├── futures-core v0.3.5
│   ├── futures-util v0.3.5
│   │   ├── futures-core v0.3.5
│   │   ├── futures-io v0.3.5
│   │   ├── futures-macro v0.3.5
│   │   │   ├── proc-macro-hack v0.5.18
│   │   │   ├── proc-macro2 v1.0.19 (*)
│   │   │   ├── quote v1.0.7 (*)
│   │   │   └── syn v1.0.35
│   │   │       ├── proc-macro2 v1.0.19 (*)
│   │   │       ├── quote v1.0.7 (*)
│   │   │       └── unicode-xid v0.2.1
│   │   ├── futures-task v0.3.5
│   │   │   └── once_cell v1.4.0
│   │   ├── memchr v2.3.3
│   │   ├── pin-project v0.4.22
│   │   │   └── pin-project-internal v0.4.22
│   │   │       ├── proc-macro2 v1.0.19 (*)
│   │   │       ├── quote v1.0.7 (*)
│   │   │       └── syn v1.0.35 (*)
│   │   ├── pin-utils v0.1.0
│   │   ├── proc-macro-hack v0.5.18
│   │   ├── proc-macro-nested v0.1.6
│   │   └── slab v0.4.2
│   ├── http v0.2.1
│   │   ├── bytes v0.5.6
│   │   ├── fnv v1.0.7
│   │   └── itoa v0.4.6
│   ├── http-body v0.3.1
│   │   ├── bytes v0.5.6
│   │   └── http v0.2.1 (*)
│   ├── hyper v0.13.7
│   │   ├── bytes v0.5.6
│   │   ├── futures-channel v0.3.5
│   │   │   └── futures-core v0.3.5
│   │   ├── futures-core v0.3.5
│   │   ├── futures-util v0.3.5 (*)
│   │   ├── h2 v0.2.6
│   │   │   ├── bytes v0.5.6
│   │   │   ├── fnv v1.0.7
│   │   │   ├── futures-core v0.3.5
│   │   │   ├── futures-sink v0.3.5
│   │   │   ├── futures-util v0.3.5 (*)
│   │   │   ├── http v0.2.1 (*)
│   │   │   ├── indexmap v1.5.0
│   │   │   │   └── hashbrown v0.8.1
│   │   │   │       [build-dependencies]
│   │   │   │       └── autocfg v1.0.0
│   │   │   │   [build-dependencies]
│   │   │   │   └── autocfg v1.0.0
│   │   │   ├── slab v0.4.2
│   │   │   ├── tokio v0.2.21
│   │   │   │   ├── bytes v0.5.6
│   │   │   │   ├── fnv v1.0.7
│   │   │   │   ├── futures-core v0.3.5
│   │   │   │   ├── iovec v0.1.4
│   │   │   │   │   └── libc v0.2.73
│   │   │   │   ├── lazy_static v1.4.0
│   │   │   │   ├── memchr v2.3.3
│   │   │   │   ├── mio v0.6.22
│   │   │   │   │   ├── cfg-if v0.1.10
│   │   │   │   │   ├── iovec v0.1.4 (*)
│   │   │   │   │   ├── libc v0.2.73
│   │   │   │   │   ├── log v0.4.11 (*)
│   │   │   │   │   ├── net2 v0.2.34
│   │   │   │   │   │   ├── cfg-if v0.1.10
│   │   │   │   │   │   └── libc v0.2.73
│   │   │   │   │   └── slab v0.4.2
│   │   │   │   ├── num_cpus v1.13.0
│   │   │   │   │   └── libc v0.2.73
│   │   │   │   ├── pin-project-lite v0.1.7
│   │   │   │   └── slab v0.4.2
│   │   │   ├── tokio-util v0.3.1
│   │   │   │   ├── bytes v0.5.6
│   │   │   │   ├── futures-core v0.3.5
│   │   │   │   ├── futures-sink v0.3.5
│   │   │   │   ├── log v0.4.11 (*)
│   │   │   │   ├── pin-project-lite v0.1.7
│   │   │   │   └── tokio v0.2.21 (*)
│   │   │   └── tracing v0.1.16
│   │   │       ├── cfg-if v0.1.10
│   │   │       ├── log v0.4.11 (*)
│   │   │       └── tracing-core v0.1.11
│   │   │           └── lazy_static v1.4.0
│   │   ├── http v0.2.1 (*)
│   │   ├── http-body v0.3.1 (*)
│   │   ├── httparse v1.3.4
│   │   ├── itoa v0.4.6
│   │   ├── pin-project v0.4.22 (*)
│   │   ├── socket2 v0.3.12
│   │   │   ├── cfg-if v0.1.10
│   │   │   └── libc v0.2.73
│   │   ├── time v0.1.43
│   │   │   └── libc v0.2.73
│   │   ├── tokio v0.2.21 (*)
│   │   ├── tower-service v0.3.0
│   │   ├── tracing v0.1.16 (*)
│   │   └── want v0.3.0
│   │       ├── log v0.4.11 (*)
│   │       └── try-lock v0.2.3
│   ├── hyper-rustls v0.20.0
│   │   ├── bytes v0.5.6
│   │   ├── futures-util v0.3.5 (*)
│   │   ├── hyper v0.13.7 (*)
│   │   ├── log v0.4.11 (*)
│   │   ├── rustls v0.17.0
│   │   │   ├── base64 v0.11.0
│   │   │   ├── log v0.4.11 (*)
│   │   │   ├── ring v0.16.15
│   │   │   │   ├── spin v0.5.2
│   │   │   │   └── untrusted v0.7.1
│   │   │   │   [build-dependencies]
│   │   │   │   └── cc v1.0.58
│   │   │   ├── sct v0.6.0
│   │   │   │   ├── ring v0.16.15 (*)
│   │   │   │   └── untrusted v0.7.1
│   │   │   └── webpki v0.21.3
│   │   │       ├── ring v0.16.15 (*)
│   │   │       └── untrusted v0.7.1
│   │   ├── tokio v0.2.21 (*)
│   │   ├── tokio-rustls v0.13.1
│   │   │   ├── futures-core v0.3.5
│   │   │   ├── rustls v0.17.0 (*)
│   │   │   ├── tokio v0.2.21 (*)
│   │   │   └── webpki v0.21.3 (*)
│   │   └── webpki v0.21.3 (*)
│   ├── lazy_static v1.4.0
│   ├── log v0.4.11 (*)
│   ├── mime v0.3.16
│   ├── mime_guess v2.0.3
│   │   ├── mime v0.3.16
│   │   └── unicase v2.6.0
│   │       [build-dependencies]
│   │       └── version_check v0.9.2
│   │   [build-dependencies]
│   │   └── unicase v2.6.0 (*)
│   ├── percent-encoding v2.1.0
│   ├── pin-project-lite v0.1.7
│   ├── rustls v0.17.0 (*)
│   ├── serde v1.0.114
│   ├── serde_urlencoded v0.6.1
│   │   ├── dtoa v0.4.6
│   │   ├── itoa v0.4.6
│   │   ├── serde v1.0.114
│   │   └── url v2.1.1
│   │       ├── idna v0.2.0
│   │       │   ├── matches v0.1.8
│   │       │   ├── unicode-bidi v0.3.4
│   │       │   │   └── matches v0.1.8
│   │       │   └── unicode-normalization v0.1.13
│   │       │       └── tinyvec v0.3.3
│   │       ├── matches v0.1.8
│   │       └── percent-encoding v2.1.0
│   ├── tokio v0.2.21 (*)
│   ├── tokio-rustls v0.13.1 (*)
│   ├── url v2.1.1 (*)
│   └── webpki-roots v0.19.0
│       └── webpki v0.21.3 (*)
└── thiserror v1.0.20
    └── thiserror-impl v1.0.20
        ├── proc-macro2 v1.0.19 (*)
        ├── quote v1.0.7 (*)
        └── syn v1.0.35 (*)
[dev-dependencies]
└── assert_approx_eq v1.1.0
urschrei commented 4 years ago

I think that as long as it's clearly documented in proj and geo it's an acceptable increase in complexity – the reduced build footprint is worth it 👍

michaelkirk commented 4 years ago

I think that as long as it's clearly documented in proj and geo

Cool, let me add some docs for the feature flag.

michaelkirk commented 4 years ago

Ok - can you take a look at my doc changes @urschrei?

Screen Shot 2020-09-03 at 3 24 02 PM`

michaelkirk commented 4 years ago

OK - i've got the feature plumbing waiting in the wings: https://github.com/georust/geo/pull/506

I think this is good to merge?

michaelkirk commented 4 years ago

Oh wait - there's a CI failure. Investigating...

michaelkirk commented 4 years ago

Oh wait - there's a CI failure. Investigating...

Test failure on bundled_proj only:

stderr:
thread 'main' panicked at 'assertion failed: `(left !== right)` (left: `499972.7074429854`, right: `500119.7035366755`, expect diff: `0.000001`, real diff: `146.99609369010432`)', src/proj.rs:16:1

I can't reproduce this failure locally.

It's a suspiciously small diff, so I thought maybe related to somehow inadvertently using a network grid, but I don't see how that's possible, given the feature is never enabled in this VM's build. Furthermore, I changed the test locally to use the grid and still wasn't able to reproduce the test failure.

Any theories @urschrei ?

I re-ran CI and the test passed. 👻

urschrei commented 4 years ago

It's a transient failure, and I have no theory as to why it crops up sometimes (and we all know what that means: writing a property test to figure out what terrible thing I've subtly screwed up). But for now, let's merge it.

urschrei commented 4 years ago

(I would say that's quite a big difference?)

michaelkirk commented 4 years ago

(I would say that's quite a big difference?)

Yeah true.

In relative terms it's 0.03% - so definitely large enough to have a meaningful impact - but my understanding is that was within the realm of what the grid offered. I have no idea if it's grid related to be clear.

urschrei commented 4 years ago

Hmm. I need to look at it more carefully, but generally you use a grid for survey-grade conversions where sub-10 cm accuracy is required. So ~150 m (is the datum in meters? I think so…😬) is in Blunder (or just "gross error": see III-5 territory.

michaelkirk commented 4 years ago

bors r=urschrei

On Sep 6, 2020, at 10:34, Stephan Hügel notifications@github.com wrote:

 Hmm. I need to look at it more carefully, but generally you use a grid for survey-grade conversions where sub-10 cm accuracy is required. So ~150 m (is the datum in meters? I think so…😬) is in Blunder (or just "gross error": see III-5 territory.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

bors[bot] commented 4 years ago

Build succeeded: