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

Add features to allow choosing between reqwest or curl for downloading #525

Closed
wants to merge 3 commits into from

Conversation

malbarbo
Copy link
Contributor

Tectonic used to use hyper, but in #330 it was replaced by reqwest. This PR allow using curl as an alternative to reqwest.

One reason to use curl instead of reqwest is that the number of dependencies is smaller (117 vs 239), reducing the compilation time (9min vs 15min of cpu itme) and size of the binary (3,7MB vs 7,2MB).

Another reason is that curl have builtin support for proxy (although I haven't test it) which would solves #59.

The curl crate will build libcurl if one is not installed in the build system and static link it, so this does not increase the native dependencies.

This also adds a mock download backend allowing to build tectonic without download support, which can be interest to assert that tectonic cannot make network connections.

client.get(url, Some((start, end)))
}

pub fn resolve_url(url: &str, _status: &mut dyn StatusBackend) -> Result<String> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not implement something similar to the reqwest implementation, but I not sure that the implementation is really necessary.

@pkgw
Copy link
Collaborator

pkgw commented Dec 20, 2019

Thanks — I appreciate the work you're doing to increase the system's flexibility here!

The implementation looks solid. The only comment I have is that there should be a bit of documentation. Would you mind adding some text to README.md describing the feature, and CHANGELOG.md listing the change for the next release notes?

@pkgw pkgw self-assigned this Dec 20, 2019
@XVilka
Copy link

XVilka commented Jan 10, 2020

Would be nice to have this also in the oxidized fork of Tectonic - https://github.com/crlf0710/tectonic

@codecov
Copy link

codecov bot commented Jan 20, 2020

Codecov Report

Merging #525 into master will increase coverage by <.01%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #525      +/-   ##
==========================================
+ Coverage   45.51%   45.52%   +<.01%     
==========================================
  Files         137      138       +1     
  Lines       59920    59927       +7     
==========================================
+ Hits        27272    27279       +7     
  Misses      32648    32648
Impacted Files Coverage Δ
src/io/mod.rs 74.91% <ø> (ø) ⬆️
src/errors.rs 40.42% <ø> (ø) ⬆️
src/config.rs 12.85% <ø> (ø) ⬆️
src/io/cached_itarbundle.rs 76.73% <66.66%> (+0.14%) ⬆️
src/io/download_reqwest.rs 79.48% <79.48%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfad100...f3b468c. Read the comment docs.

@pkgw
Copy link
Collaborator

pkgw commented Jan 20, 2020

@malbarbo I've pushed a commit that merges current master into your branch here, which pulls in the updated CI/CD infrastructure that I've put together.

@pkgw
Copy link
Collaborator

pkgw commented Jan 14, 2021

It took a year, but I have finally merged this functionality in #726!

cargo build --no-default-features --features "serialization geturl-curl"

should do it. This is one of the the things that I feel much better implementing using the new sub-crate architecture, made possible by cbindgen, a better understanding of how Rust's sys crates work, and Cranko.

@pkgw pkgw closed this Jan 14, 2021
@malbarbo
Copy link
Contributor Author

Thanks @pkgw for moving this forward.

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.

3 participants