Make features explicit, error fixes, (#19)

* Make features explicit, error fixes,

* Use `thiserror` for all errors to allow better error reporting
* Make all HTTP errors into proper objects without string formatting -- formatting is delayed until printing, or if not logged by end user, works much faster
* add justfile to simplify development (optional, but useful)
* cleanup CI to make it far simpler (that GH action is not maintained, and not worth it)
* Made cargo.toml explicitly declare all features - instead of auto-exposing optional dependencies as features

.github/workflows/test.yml

@@ -1,13 +1,13 @@
-name: Cargo Test
+name: CI
 
 on:
-  push: {}
+  push:
+    branches: [main]
   pull_request:
-    branches:
-      - main
+    branches: [main]
   release:
-    branches:
-      - main
+    types: [published]
+  workflow_dispatch:
 
 jobs:
   build_and_test:
@@ -17,51 +17,15 @@ jobs:
       - name: Checkout sources
         uses: actions/checkout@v3
 
-      - name: Install stable toolchain
-        uses: actions-rs/toolchain@v1
-        with:
-          toolchain: stable
-
-      - name: Run cargo check
-        uses: actions-rs/cargo@v1
-        with:
-          command: check
-
-      - name: Install fmt and clippy
-        run: rustup component add clippy rustfmt
-
-      - name: Run cargo fmt
-        uses: actions-rs/cargo@v1
-        with:
-          command: fmt
-          args: --all -- --check
-
-      - name: Run cargo clippy
-        uses: actions-rs/cargo@v1
-        with:
-          command: clippy
-          args: --all-targets --all-features -- -D warnings
-
-      - name: Run tests
-        uses: actions-rs/cargo@v1
-        with:
-          command: test
-          args: --all-targets --all-features
-
-      - name: Test http-async
-        uses: actions-rs/cargo@v1
-        with:
-          command: test
-          args: --features http-async
-
-      - name: Test mmap-async-tokio
-        uses: actions-rs/cargo@v1
-        with:
-          command: test
-          args: --features mmap-async-tokio
-
-      - name: Test tilejson
-        uses: actions-rs/cargo@v1
-        with:
-          command: test
-          args: --features tilejson
+      - run: |
+          rustc --version
+          cargo --version
+          rustup --version
+      - run: cargo check
+      - run: rustup component add clippy rustfmt
+      - run: cargo fmt --all -- --check
+      - run: cargo clippy --all-targets --all-features -- -D warnings
+      - run: cargo test --all-targets --all-features
+      - run: cargo test --features http-async
+      - run: cargo test --features mmap-async-tokio
+      - run: cargo test --features tilejson

Cargo.toml

@@ -7,35 +7,38 @@ license = "MIT OR Apache-2.0"
 description = "Implementation of the PMTiles v3 spec with multiple sync and async backends."
 repository = "https://github.com/stadiamaps/pmtiles-rs"
 keywords = ["pmtiles", "gis", "geo"]
+rust-version = "1.61.0"
 
 [features]
 default = []
-http-async = ["reqwest", "tokio"]
-mmap-async-tokio = ["fmmap", "fmmap/tokio-async", "tokio"]
-tilejson = ["dep:tilejson", "serde", "serde_json"]
+http-async = ["dep:tokio", "dep:reqwest"]
+mmap-async-tokio = ["dep:tokio", "dep:fmmap", "fmmap?/tokio-async"]
+tilejson = ["dep:tilejson", "dep:serde", "dep:serde_json"]
 
 # TODO: support other async libraries
 
 [dependencies]
 # TODO: determine how we want to handle compression in async & sync environments
-async-compression = { version = "0.3", features = ["gzip", "zstd", "brotli", "tokio"] }
+# TODO: tokio is always requested here, but the tokio dependency is optional below - maybe make it required?
+async-compression = { version = "0.4", features = ["gzip", "zstd", "brotli", "tokio"] }
 async-recursion = "1"
 async-trait = "0.1"
 bytes = "1"
 fmmap = { version = "0.3", default-features = false, optional = true }
-hilbert_2d = "1.1"
+hilbert_2d = "1"
 reqwest = { version = "0.11", default-features = false, optional = true }
-tokio = { version = "1", default-features = false, features = ["io-util"], optional = true }
-varint-rs = "2"
-tilejson = { version = "0.3", optional = true }
 serde = { version = "1", optional = true }
 serde_json = { version = "1", optional = true }
+thiserror = "1"
+tilejson = { version = "0.3", optional = true }
+tokio = { version = "1", default-features = false, features = ["io-util"], optional = true }
+varint-rs = "2"
 
 [dev-dependencies]
 fmmap = { version = "0.3", features = ["tokio-async"] }
 reqwest = { version = "0.11", features = ["rustls-tls-webpki-roots"] }
 tokio = { version = "1", features = ["test-util", "macros", "rt"] }
-flate2 = "1.0.24"
+flate2 = "1"
 
 [package.metadata.docs.rs]
 all-features = true

README.md

@@ -3,7 +3,8 @@
 [![GitHub](https://img.shields.io/badge/github-stadiamaps/pmtiles--rs-8da0cb?logo=github)](https://github.com/stadiamaps/pmtiles-rs)
 [![crates.io version](https://img.shields.io/crates/v/pmtiles.svg)](https://crates.io/crates/pmtiles)
 [![docs.rs docs](https://docs.rs/pmtiles/badge.svg)](https://docs.rs/pmtiles)
-[![CI build](https://github.com/stadiamaps/pmtiles-rs/workflows/Cargo%20Test/badge.svg)](https://github.com/stadiamaps/pmtiles-rs/actions)
+[![crates.io version](https://img.shields.io/crates/l/pmtiles.svg)](https://github.com/stadiamaps/pmtiles-rs/blob/main/LICENSE-APACHE)
+[![CI build](https://github.com/stadiamaps/pmtiles-rs/workflows/CI/badge.svg)](https://github.com/stadiamaps/pmtiles-rs/actions)
 
 This crate implements the [PMTiles v3 spec](https://github.com/protomaps/PMTiles/blob/master/spec/v3/spec.md),
 originally created by Brandon Liu for Protomaps.
@@ -27,9 +28,25 @@ originally created by Brandon Liu for Protomaps.
 
 PRs welcome!
 
+## Development
+* This project is easier to develop with [just](https://github.com/casey/just#readme), a modern alternative to `make`. Install it with `cargo install just`.
+* To get a list of available commands, run `just`.
+* To run tests, use `just test`.
+
 ## License
 
-This project is dual-licensed as MIT and Apache 2.0. You may select the license most appropriate for your project.
+Licensed under either of
+
+* Apache License, Version 2.0 ([LICENSE-APACHE](LICENSE-APACHE) or <http://www.apache.org/licenses/LICENSE-2.0>)
+* MIT license ([LICENSE-MIT](LICENSE-MIT) or <http://opensource.org/licenses/MIT>)
+  at your option.
+
+### Contribution
+
+Unless you explicitly state otherwise, any contribution intentionally
+submitted for inclusion in the work by you, as defined in the
+Apache-2.0 license, shall be dual licensed as above, without any
+additional terms or conditions.
 
 ## Test Data License
 

justfile

@@ -0,0 +1,37 @@
+#!/usr/bin/env just --justfile
+
+@_default:
+    just --list --unsorted
+
+# Run all tests
+test:
+    # These are the same tests that are run on CI. Eventually CI should just call into justfile
+    cargo check
+    rustup component add clippy rustfmt
+    cargo fmt --all -- --check
+    cargo clippy --all-targets --all-features -- -D warnings
+    cargo test --all-targets --all-features
+    cargo test --features http-async
+    cargo test --features mmap-async-tokio
+    cargo test --features tilejson
+    RUSTDOCFLAGS="-D warnings" cargo doc --no-deps
+
+# Run cargo fmt and cargo clippy
+lint: fmt clippy
+
+# Run cargo fmt
+fmt:
+    cargo +nightly fmt -- --config imports_granularity=Module,group_imports=StdExternalCrate
+
+# Run cargo clippy
+clippy:
+    cargo clippy --workspace --all-targets --bins --tests --lib --benches -- -D warnings
+
+# Build and open code documentation
+docs:
+    cargo doc --no-deps --open
+
+# Clean all build artifacts
+clean:
+    cargo clean
+    rm -f Cargo.lock

src/async_reader.rs

@@ -6,7 +6,7 @@ use async_trait::async_trait;
 use bytes::Bytes;
 #[cfg(feature = "http-async")]
 use reqwest::{Client, IntoUrl};
-#[cfg(feature = "tokio")]
+#[cfg(any(feature = "http-async", feature = "mmap-async-tokio"))]
 use tokio::io::AsyncReadExt;
 
 use crate::directory::{Directory, Entry};

src/error.rs

@@ -1,28 +1,52 @@
 use std::string::FromUtf8Error;
 
-#[derive(Debug)]
+use thiserror::Error;
+
+#[derive(Debug, Error)]
 pub enum Error {
+    #[error("Invalid magic number")]
     InvalidMagicNumber,
+    #[error("Invalid PMTiles version")]
     UnsupportedPmTilesVersion,
+    #[error("Invalid compression")]
     InvalidCompression,
+    #[error("Invalid PMTiles entry")]
     InvalidEntry,
+    #[error("Invalid header")]
     InvalidHeader,
+    #[error("Invalid metadata")]
     InvalidMetadata,
+    #[error("Invalid metadata UTF-8 encoding: {0}")]
+    InvalidMetadataUtf8Encoding(#[from] FromUtf8Error),
+    #[error("Invalid tile type")]
     InvalidTileType,
-    Reading(std::io::Error),
-    #[cfg(feature = "fmmap")]
+    #[error("IO Error {0}")]
+    Reading(#[from] std::io::Error),
+    #[cfg(feature = "mmap-async-tokio")]
+    #[error("Unable to open mmap file")]
     UnableToOpenMmapFile,
-    Http(String),
+    #[cfg(feature = "http-async")]
+    #[error("{0}")]
+    Http(#[from] HttpError),
 }
 
-impl From<std::io::Error> for Error {
-    fn from(e: std::io::Error) -> Self {
-        Self::Reading(e)
-    }
+#[cfg(feature = "http-async")]
+#[derive(Debug, Error)]
+pub enum HttpError {
+    #[error("Unexpected number of bytes returned [expected: {0}, received: {1}].")]
+    UnexpectedNumberOfBytesReturned(usize, usize),
+    #[error("Range requests unsupported")]
+    RangeRequestsUnsupported,
+    #[error("HTTP response body is too long, Response {0}B > requested {1}B")]
+    ResponseBodyTooLong(usize, usize),
+    #[error("HTTP error {0}")]
+    Http(#[from] reqwest::Error),
 }
 
-impl From<FromUtf8Error> for Error {
-    fn from(_: FromUtf8Error) -> Self {
-        Self::InvalidMetadata
+// This is required because thiserror #[from] does not support two-level conversion.
+#[cfg(feature = "http-async")]
+impl From<reqwest::Error> for Error {
+    fn from(e: reqwest::Error) -> Self {
+        Self::Http(HttpError::Http(e))
     }
 }

src/http.rs

@@ -4,7 +4,7 @@ use reqwest::header::{HeaderValue, ACCEPT_RANGES, RANGE};
 use reqwest::{Client, IntoUrl, Method, Request, Url};
 
 use crate::async_reader::AsyncBackend;
-use crate::error::Error;
+use crate::error::{Error, HttpError};
 
 pub struct HttpBackend {
     client: Client,
@@ -30,11 +30,7 @@ impl AsyncBackend for HttpBackend {
         if data.len() == length {
             Ok(data)
         } else {
-            Err(Error::Http(format!(
-                "Unexpected number of bytes returned [expected: {}, received: {}].",
-                length,
-                data.len()
-            )))
+            Err(HttpError::UnexpectedNumberOfBytesReturned(length, data.len()).into())
         }
     }
 
@@ -51,25 +47,19 @@ impl AsyncBackend for HttpBackend {
         let response = self.client.execute(req).await?.error_for_status()?;
 
         if response.headers().get(ACCEPT_RANGES) != Some(&VALID_ACCEPT_RANGES) {
-            return Err(Error::Http("Range requests unsupported".to_string()));
+            return Err(HttpError::RangeRequestsUnsupported.into());
         }
 
         let response_bytes = response.bytes().await?;
 
         if response_bytes.len() > length {
-            Err(Error::Http("HTTP response body is too long".to_string()))
+            Err(HttpError::ResponseBodyTooLong(response_bytes.len(), length).into())
         } else {
             Ok(response_bytes)
         }
     }
 }
 
-impl From<reqwest::Error> for Error {
-    fn from(e: reqwest::Error) -> Self {
-        Error::Http(e.to_string())
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use crate::async_reader::AsyncPmTilesReader;

src/mmap.rs

@@ -3,7 +3,7 @@ use std::path::Path;
 
 use async_trait::async_trait;
 use bytes::{Buf, Bytes};
-use fmmap::tokio::{AsyncMmapFile, AsyncMmapFileExt, AsyncOptions};
+use fmmap::tokio::{AsyncMmapFile, AsyncMmapFileExt as _, AsyncOptions};
 
 use crate::async_reader::AsyncBackend;
 use crate::error::Error;