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

Implement configuration for automatic local bundle loading. #211

Merged
merged 3 commits into from
Aug 20, 2018

Conversation

crlf0710
Copy link
Contributor

This commit adds an optional local parameter in config.toml, when specified as true, tectonic will try to load bundle from a zip file path specified in the usual url parameter.

@pkgw
Copy link
Collaborator

pkgw commented Aug 15, 2018

Thanks for submitting this as well!

I like the general idea of this PR, but having a boolean local flag that causes the url field to be treated differently seems like a design that would be surprising and confusing for users. How about an approach where file:// URLs are loaded as local Zip bundles?

@crlf0710 crlf0710 force-pushed the local-bundle branch 2 times, most recently from 7af6640 to 811207d Compare August 15, 2018 11:18
@crlf0710
Copy link
Contributor Author

Ok, so i changed it to try to detect an file:// prefix, if the prefix exists, it strips it off and try to load the rest as a path to a zip file. On my Windows machine this works fine.

@codecov
Copy link

codecov bot commented Aug 15, 2018

Codecov Report

Merging #211 into master will decrease coverage by <.01%.
The diff coverage is 7.4%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #211      +/-   ##
==========================================
- Coverage    37.9%   37.89%   -0.01%     
==========================================
  Files         131      131              
  Lines       60307    60339      +32     
==========================================
+ Hits        22857    22864       +7     
- Misses      37450    37475      +25
Impacted Files Coverage Δ
tests/tex-outputs.rs 83.5% <100%> (+0.52%) ⬆️
src/config.rs 11.9% <3.84%> (+0.99%) ⬆️

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 8c737b9...6d8ab54. Read the comment docs.

@crlf0710
Copy link
Contributor Author

@pkgw Please review this when you have time. What can i do to improve code coverage in this case?

src/config.rs Outdated
let url = &self.default_bundles[0].url;
const FILE_SCHEME_PREFIX: &'static str = "file://";
if url.starts_with(FILE_SCHEME_PREFIX) {
return Ok(self.make_local_file_provider(&url[FILE_SCHEME_PREFIX.len()..], status)?);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am a little concerned about URL encoding here. A proper URL might contain a sequence like %20 to encode a space, but this approach won't convert it as appropriate. I think the solution here is to parse the URL for real — Tectonic already depends on the url crate, so that should be used. Then you can check if the scheme is file, and use the handy to_file_path function.

@pkgw
Copy link
Collaborator

pkgw commented Aug 16, 2018

OK, this looks very good. There is one change I think should be made to make the URL parsing more correct but based on my read of the relevant API docs, I think it should be easy to implement.

It might be hard to boost the code coverage here. A small unit test could maybe cover how different URLs are parsed, but since these functions want to do the actual I/O, I suspect that one would need to build a lot of infrastructure to have a test cover all of the code. It is OK if you can't figure out a way to get code coverage for your changes. If you can devise a small new test case that increases coverage in some other part of the codebase, even if it is totally unrelated to this change, that would be great, but I'll be happy to merge this even if the coverage doesn't go up.

@pkgw pkgw self-assigned this Aug 20, 2018
@pkgw
Copy link
Collaborator

pkgw commented Aug 20, 2018

Thanks for addressing this! I had to merge in changes from another PR to master, but will merge this once the CI passes.

@pkgw
Copy link
Collaborator

pkgw commented Aug 20, 2018

OK, as expected, code coverage remains a challenge, but let's get this merged.

@pkgw pkgw merged commit 532e428 into tectonic-typesetting:master Aug 20, 2018
@crlf0710 crlf0710 deleted the local-bundle branch August 20, 2018 15:44
Mrmaxmeier added a commit to Mrmaxmeier/tectonic that referenced this pull request Nov 24, 2019
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.

None yet

2 participants