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

Make CLI-only features optional #62

Merged
merged 5 commits into from
Jun 17, 2023
Merged

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Jun 15, 2023

This greatly speeds up build for the users of the lib when CLI is not needed. Sorry forgot to include it in the previous round.

  • Users of this crate should put this line in their Cargo.toml (needs to be documented somewhere?): spreet = { version = "...", default-features = false }
  • assert_fs dependency seemed not to be needed by the primary code, only by the tests
  • made all other CLI-only dependencies as optional, but enabled by default via the cli feature

This greatly speeds up build for the users of the lib when CLI is not
needed.  Sorry forgot to include it in the previous round.
nyurik added a commit to maplibre/martin that referenced this pull request Jun 16, 2023
Dynamically create image sprites for MapLibre rendering, given a
directory with images.

### TODO
* [x] Work with @flother to merge these PRs
  * [x] flother/spreet#59  (must have)
  * [x] flother/spreet#57
  * [x] flother/spreet#56
* [ ] flother/spreet#62 (not required but nice
to have, can upgrade later without any code changes)
* [x] Add docs to the book
* [x] Add CLI param, e.g. `--sprite <dir_path>`
* [x] Don't output `.sprites` in auto-genned config when not in use

### API
Per [MapLibre sprites
API](https://maplibre.org/maplibre-style-spec/sprite/), we need to
support the following:
* `/sprite/<sprite_id>.json` metadata about the sprite file - all coming
from a single directory
* `/sprite/<sprite_id>.png` all images combined into a single PNG
* `/sprite/<sprite_id>@2x.json` same but for high DPI devices
* `/sprite/<sprite_id>@2x.png`

Multiple sprite_id values can be combined into one sprite with the same
pattern as for tile joining:
`/sprite/<sprite_id1>,<sprite_id2>,...,<sprite_idN>[.json|.png|@2x.json|@2x.png]`.
No ID renaming is done, so identical names will override one another.

### Configuration
[Config file](https://maplibre.org/martin/config-file.html) and possibly
CLI should have a simple option to serve sprites. The configuration may
look similar to how mbtiles and pmtiles are configured:

```yaml
# Publish sprite images
sprites:
  paths:
    # scan this whole dir, matching all image files, and publishing it as "my_images" sprite source
    - /path/to/my_images
  sources:
    # named source matching source name to a directory
    my_sprites: /path/to/some_dir
```

Implement #705
Copy link
Owner

@flother flother left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small change required before this is merged (see suggestion below).

Besides that, it would be nice to add tests to check that Spreet still works with the CLI disabled. It doesn't seem like that's something Cargo supports particularly well, so my best idea is to mark existing tests as CLI-feature only, add tests to check use of Spreet as a library, and run cargo test --no-default-features during CI. I'm happy to try that out after this is merged, but if you have any better ideas let me know

Cargo.toml Outdated Show resolved Hide resolved
@nyurik
Copy link
Contributor Author

nyurik commented Jun 17, 2023

Good point about the dep: - i never used them before, but they make so much more sense!

@flother
Copy link
Owner

flother commented Jun 17, 2023

Thanks for your extra work in sorting out the tests. I've added a commit to add the docs you suggested on using the crate in Cargo.toml.

I'll merge this now and flesh out the library-based tests in tests/sprite.rs.

@flother flother merged commit f42aed9 into flother:master Jun 17, 2023
flother added a commit that referenced this pull request Jun 17, 2023
@nyurik nyurik deleted the make-cli-optional branch October 12, 2023 06:28
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