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

Remove Nonstatic Zip & Reqwest #153

Closed
wants to merge 6 commits into from
Closed

Conversation

lolpro11
Copy link

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.

Copy link
Collaborator

@antoine-de antoine-de left a comment

Choose a reason for hiding this comment

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

fine for me, thanks for this!

@Tristramg
Copy link
Collaborator

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
Copy link
Author

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

@antoine-de
Copy link
Collaborator

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

By curiosity, would do you need this?

@lolpro11
Copy link
Author

lolpro11 commented Feb 1, 2024

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
Copy link
Contributor

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
Copy link
Author

lolpro11 commented Feb 6, 2024

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

  • Store (no compression),
  • Shrink (LZW),
  • Reduce (levels 1–4; LZ77 + probabilistic),
  • Implode,
  • Deflate,
  • Deflate64,
  • bzip2,
  • LZMA,
  • WavPack,
  • PPMd,
  • LZ77 variant provided by IBM z/OS CMPSC instruction

@lolpro11
Copy link
Author

lolpro11 commented Feb 7, 2024

@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
Copy link

dkter commented Apr 2, 2024

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

@lolpro11
Copy link
Author

lolpro11 commented Apr 2, 2024

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
Copy link

dkter commented Apr 2, 2024

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
Copy link
Author

lolpro11 commented Apr 2, 2024

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
Copy link

dkter commented Apr 2, 2024

#163

@lolpro11 lolpro11 closed this Apr 2, 2024
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.

5 participants