search

rust-transit / gtfs-structure

Read a GTFS file
MIT License
56 stars 32 forks source link

Remove Nonstatic Zip & Reqwest #153

Closed lolpro11 closed 5 months ago

lolpro11 commented 7 months ago

This does not affect the functionality of the crate, as this does not use zstd. Same thing with using rust-tls. We are just switching tls libraries. These changes will allow a static compile of this library.

Tristramg commented 7 months ago

I’d be a bit more cautious concerning the compression algorithm support. We use zip to read files generated by others. It would be annoying if there are some zip files that can not be read anymore; multiple formats seem to used, not only deflate: https://en.wikipedia.org/wiki/ZIP_(file_format)#Compression_methods

No problem for rust-tls

lolpro11 commented 7 months ago

Update: I patched zip to use only Rust versions of bzip2 and zstd, thereby allowing rustc to compile this lib statically

antoine-de commented 7 months ago

We cannot depend directly on a git dependency, we need something properly published on crates.io

By curiosity, would do you need this?

lolpro11 commented 7 months ago

We cannot depend directly on a git dependency, we need something properly published on crates.io

By curiosity, would do you need this?

I am waiting on an upstream PR with zip. Also, I do need this so I can run this in a container. Let me try to find another solution

kylerchin commented 7 months ago

We cannot depend directly on a git dependency, we need something properly published on crates.io

By curiosity, would do you need this?

I completely agree with this.

lolpro11 commented 7 months ago

I’d be a bit more cautious concerning the compression algorithm support. We use zip to read files generated by others. It would be annoying if there are some zip files that can not be read anymore; multiple formats seem to used, not only deflate: https://en.wikipedia.org/wiki/ZIP_(file_format)#Compression_methods

No problem for rust-tls

lolpro11 commented 7 months ago

@Tristramg Are you sure we need support for bzip and zstd? I did a compression method check on Transitland's database and it looks like they don't have a single file with bzip or zstd. GTFScompression.log

dkter commented 5 months ago

I patched it to allow disabling of bzip and zstd via a feature, this shouldn't break anything by default since the feature is enabled by default: https://github.com/dkter/gtfs-structure/commit/cf418953684dd9ab286857d457689117095bad82

If needed I can make another PR

lolpro11 commented 5 months ago

I patched it to allow disabling of bzip and zstd via a feature, this shouldn't break anything by default since the feature is enabled by default: dkter@cf41895

If needed I can make another PR

Can you make another PR for this? That would be great. Also, what do you intend on doing for rust-tls?

dkter commented 5 months ago

Also, what do you intend on doing for rust-tls?

I'm able to use default-tls for my use case but could re-export these if helpful?

reqwest-default = ["reqwest/default"]
reqwest-rustls-tls = ["reqwest/rustls-tls"]
lolpro11 commented 5 months ago

I'm able to use default-tls for my use case but could re-export these if helpful?

reqwest-default = ["reqwest/default"]
reqwest-rustls-tls = ["reqwest/rustls-tls"]

Yeah, that looks good! Thanks. Let me know when you have the PR done so I can close this one.

dkter commented 5 months ago

163