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

Bundle rework #1121

Draft
wants to merge 67 commits into
base: master
Choose a base branch
from
Draft

Bundle rework #1121

wants to merge 67 commits into from

Conversation

rm-dr
Copy link
Contributor

@rm-dr rm-dr commented Nov 25, 2023

Our current bundle scheme is causing quite a few bugs. This PR (paired with tectonic-typesetting/tectonic-texlive-bundles#14) resolves many of these by implementing proper search paths in bundles.

These changes are not be breaking. Old zip and bundles will work with this PR.
(I have, however, removed the scripts that create them. They should eventually be deprecated.)

Changes in this PR:

  • A new bundle format (see Huge rewrite, many fixes tectonic-texlive-bundles#14)
  • File search paths (also documented in the bundle repo)
  • Bundle autodetection from url (thus, --web-bundle has been removed, and --bundle now handles both URLs and paths)
  • Bundle caching rework (mostly invisible to the user, enables cleaner code)
  • Cache organization rework (the bundle cache needs fewer files and is in its own subdir)
  • Many minor bundle-related cleanups and optimizations

Most changes in this PR are in the bundles crate. Changes in other files are usually minor side-effects thanks to a slightly different API.

Copy link
Contributor

@vlasakm vlasakm left a comment

Choose a reason for hiding this comment

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

Just some things I noticed at firts glance.

crates/bundles/src/zip.rs Outdated Show resolved Hide resolved
crates/bundles/src/zip.rs Outdated Show resolved Hide resolved
@pkgw
Copy link
Collaborator

pkgw commented Nov 25, 2023

Thanks for all this work! As I remarked in the texlive-bundles PR, I wasn't getting any notifications about your work on this topic. I hope to start reading over your PRs and understanding what you've been working on, although to be honest it will likely take me a lot longer to do so than I'd like.

@rm-dr
Copy link
Contributor Author

rm-dr commented Feb 10, 2024

Alright, that should be it. The new cache is multi-process-safe via renames, tests pass, and clippy errors have been fixed.
Both this and tectonic-typesetting/tectonic-texlive-bundles#14 should be ready to merge.

@rm-dr rm-dr marked this pull request as ready for review February 10, 2024 05:35
Copy link

codecov bot commented Feb 10, 2024

Codecov Report

Attention: Patch coverage is 38.40000% with 462 lines in your changes are missing coverage. Please review.

Project coverage is 46.11%. Comparing base (b3b66f2) to head (4d3f8b6).

Files Patch % Lines
crates/bundles/src/ttb.rs 0.00% 136 Missing ⚠️
crates/bundles/src/ttb_net.rs 0.00% 108 Missing ⚠️
crates/bundles/src/ttb_fs.rs 0.00% 62 Missing ⚠️
crates/bundles/src/itar.rs 72.09% 36 Missing ⚠️
crates/bundles/src/cache.rs 77.69% 29 Missing ⚠️
crates/bundles/src/lib.rs 59.42% 28 Missing ⚠️
crates/bundles/src/dir.rs 0.00% 18 Missing ⚠️
crates/bundles/src/zip.rs 0.00% 18 Missing ⚠️
crates/io_base/src/app_dirs.rs 54.54% 5 Missing ⚠️
src/docmodel.rs 63.63% 4 Missing ⚠️
... and 12 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1121      +/-   ##
==========================================
- Coverage   46.40%   46.11%   -0.30%     
==========================================
  Files         176      179       +3     
  Lines       65118    65413     +295     
==========================================
- Hits        30221    30168      -53     
- Misses      34897    35245     +348     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pkgw
Copy link
Collaborator

pkgw commented Feb 11, 2024

CC @CraftSpider here too.

@rm-dr
Copy link
Contributor Author

rm-dr commented Feb 11, 2024

Hmmm
A few tests are still failing on azure, not sure why.
Everything passes on my machine.

@pkgw
Copy link
Collaborator

pkgw commented Feb 14, 2024

Hmm, I'm not sure why this error would only be showing up in the cross-tests. The one thing that I can think of is that those run through the cross program, which filters the environment variables that propagate down to the subprograms that it runs ... but I don't see anything about this particular error that would depend on environment variables being set.

@rm-dr rm-dr mentioned this pull request Feb 21, 2024
@rm-dr rm-dr added bundles and files Bundle issues, finding fonts, etc. new feature New features bug and removed bug labels Feb 27, 2024
@rm-dr rm-dr marked this pull request as draft February 28, 2024 18:17
@rm-dr
Copy link
Contributor Author

rm-dr commented Feb 29, 2024

Just fixed #74. File names with spaces are now safe in ttbv1 bundles---but newlines will still cause errors.

I'm not sure that's a problem, though, since I've never seen a legitimate use case for files with newlines.

@rm-dr rm-dr linked an issue Feb 29, 2024 that may be closed by this pull request
@rm-dr
Copy link
Contributor Author

rm-dr commented Feb 29, 2024

Here's an idea: should this new bundle format also include latex format files?

Now that the bundle-building code is in Rust, it shouldn't be too hard to generate format files while we build the bundle. (assuming format files are identical on all machines)

Would be nice to do that here, so that we only have one round of big, possibly disruptive changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bundles and files Bundle issues, finding fonts, etc. new feature New features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A more robust manifest format
3 participants