From a22434871286c38b4f2394b50f120095e2b93442 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sun, 13 Aug 2023 01:06:39 -0400 Subject: [PATCH] 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 * Lastly, it seems `fmmap` has not been updated for a long time, and because of that it no longer compiles in some cases because of the dependency problems. I submitted a PR, hopefully will be merged/published soonish. --- .github/workflows/test.yml | 72 ++++++++++---------------------------- Cargo.toml | 26 +++++++++----- README.md | 21 +++++++++-- justfile | 37 ++++++++++++++++++++ src/async_reader.rs | 2 +- src/error.rs | 47 +++++++++++++++++++------ src/http.rs | 18 +++------- src/mmap.rs | 2 +- 8 files changed, 133 insertions(+), 92 deletions(-) create mode 100644 justfile diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index fbc69b5..ea34a84 100644 --- a/.github/workflows/test.yml +++ b/.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 diff --git a/Cargo.toml b/Cargo.toml index 3dfef67..799f4eb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -7,35 +7,43 @@ 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 + +[patch.crates-io] +# TODO: remove this once fmmap is updated +# https://github.com/al8n/fmmap/pull/5 +fmmap = { git = "https://github.com/nyurik/fmmap", branch = "upgrade-deps" } diff --git a/README.md b/README.md index 2a12f8b..cb375bc 100644 --- a/README.md +++ b/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 ) +* MIT license ([LICENSE-MIT](LICENSE-MIT) or ) + 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 diff --git a/justfile b/justfile new file mode 100644 index 0000000..c4cf879 --- /dev/null +++ b/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 diff --git a/src/async_reader.rs b/src/async_reader.rs index bdba690..881ccbd 100644 --- a/src/async_reader.rs +++ b/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}; diff --git a/src/error.rs b/src/error.rs index 9c80b3b..b87874d 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,28 +1,53 @@ use std::string::FromUtf8Error; -#[derive(Debug)] +// use std::string::FromUtf8Error; +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 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 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 for Error { + fn from(e: reqwest::Error) -> Self { + Self::Http(HttpError::Http(e)) } } diff --git a/src/http.rs b/src/http.rs index 400ea17..2b35acf 100644 --- a/src/http.rs +++ b/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 for Error { - fn from(e: reqwest::Error) -> Self { - Error::Http(e.to_string()) - } -} - #[cfg(test)] mod tests { use crate::async_reader::AsyncPmTilesReader; diff --git a/src/mmap.rs b/src/mmap.rs index 4eaaf88..70270e8 100644 --- a/src/mmap.rs +++ b/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;