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 --only-cached/-C command line flag #203

Merged
merged 1 commit into from
Aug 20, 2018

Conversation

rekka
Copy link
Contributor

@rekka rekka commented Aug 10, 2018

This is a preliminary pull request proposing to add a new command line option. I want to ask for feedback.

With this flag enabled, tectonic will only use resource files that are already stored in the local cache and will not try to fetch them from a web bundle.

Tectonic at the moment accesses the bundle file whenever a new file like a figure is added to a TeX document or a new TeX file is being compiled. In a majority of these cases, this is unnecessary because the files will not be found in the bundle anyway. Usually, this is not a big issue, but it can be annoying to deal with when the web bundle is not accessible. I believe there should be a way to ask tectonic not to try to fetch the unknown files.

This has at least 3 use cases:

  • workaround for web bundle problems like URL to the default config is broken #200
  • no internet connection: preparing a presentation on a plane
  • speed up tectonic's edit-build-view cycle when adding a lot of new figures etc.

With this flag enabled, tectonic will only use resource files that are
already stored in the local cache and will not try to fetch them from a
web bundle.
@pkgw pkgw self-assigned this Aug 13, 2018
@pkgw
Copy link
Collaborator

pkgw commented Aug 13, 2018

Thanks for submitting! This month is very busy for me but I will do my best to give a proper review in timely fashion.

@codecov
Copy link

codecov bot commented Aug 14, 2018

Codecov Report

Merging #203 into master will decrease coverage by 0.07%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #203      +/-   ##
=========================================
- Coverage   37.97%   37.9%   -0.08%     
=========================================
  Files         131     131              
  Lines       60180   60307     +127     
=========================================
+ Hits        22856   22857       +1     
- Misses      37324   37450     +126
Impacted Files Coverage Δ
src/bin/tectonic.rs 0.6% <0%> (-0.23%) ⬇️
src/config.rs 10.9% <0%> (+0.9%) ⬆️
src/io/local_cache.rs 0% <0%> (ø) ⬆️

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 1cfc433...d0ea85d. Read the comment docs.

@pkgw
Copy link
Collaborator

pkgw commented Aug 16, 2018

Looks great. My only request is, can you try to devise a test case so that we get code coverage of this new code? I think it should not be hard to get this PR to ratchet up the code coverage a tiny bit, and that's something I'd like to be better about ...

@rekka
Copy link
Contributor Author

rekka commented Aug 20, 2018

Thanks for the suggestion. The thing is, the files that this commit touches do not currently have any coverage, or more precisely, they do not show in the codecov report. Looking at how the command line arguments are tested at the moment, it would be natural to add a new test to tests/executable.rs. But those tests do not show up in the coverage report. They are in fact disabled when the code coverage is being run.

I guess one way to get coverage would be to rewrite the "executable" tests to call the main method of the binary directly, passing the command line arguments as an argument to main and then load them via clap::App::get_matches_from.

This seems a bit too much for this pull request. Do you have a more straightforward solution?

Another question is how to actually test --only-cached. The result depends on the content of the cache.

@pkgw
Copy link
Collaborator

pkgw commented Aug 20, 2018

Thanks for looking into it. All those points are things that should be addressed but I agree that yes, it's all a bit much for this one PR. And the code change is quite simple ... the kind of logic which of course never comes back to bite anybody ;-)

@pkgw pkgw merged commit 8c737b9 into tectonic-typesetting:master Aug 20, 2018
@rekka rekka deleted the feat-only-cached branch August 24, 2018 03:58
Mrmaxmeier added a commit to Mrmaxmeier/tectonic that referenced this pull request Nov 21, 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.

2 participants