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

Extra paths for shared resources #1106

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rm-dr
Copy link
Contributor

@rm-dr rm-dr commented Oct 17, 2023

For context, see the discussion in #985.
Closes #985.

TLDR:

This PR adds an extra_paths config key that allows a user to specify external resources.
An example file tree and Tectonic.toml are below.

[doc]
name = "Test"
bundle = "https://data1.fullyjustified.net/tlextras-2022.0r0.tar"

# New config key
extra_paths = [
    "../../resources"
]


[[output]]
name = "main"
type = "pdf"
workspace-repo-root/
├── resources/
│   └── class files, etc      <-- All documents pull files from here
│
├── Subdir1/
│   ├── this document/
│   │   ├── src/
│   │   └── Tectonic.toml     <-- this file is above
│   └── more documents...
│   
└── Subdir2/
    └── and so on...

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

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

Project coverage is 46.35%. Comparing base (7432afd) to head (ff3d318).
Report is 6 commits behind head on master.

Files Patch % Lines
crates/docmodel/src/document.rs 75.00% 2 Missing ⚠️
crates/docmodel/src/syntax.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1106   +/-   ##
=======================================
  Coverage   46.35%   46.35%           
=======================================
  Files         171      171           
  Lines       65136    65149   +13     
=======================================
+ Hits        30191    30200    +9     
- Misses      34945    34949    +4     

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

@ratmice
Copy link
Contributor

ratmice commented Oct 17, 2023

FWIW, in the referenced thread, I added a patch which restricts the path to relative paths using cap_std.
This probably needs some error handling, currently it acts like e.g. passing an invalid path to TEXINPUTS, and just ignores them.
that branch is here:

https://github.com/ratmice/tectonic/tree/extrapaths
Edit: My patch/branch does have some issues when running tectonic from a subdirectory of the root, will look at it as soon as I can.

@rm-dr rm-dr marked this pull request as ready for review February 16, 2024 20:22
@rm-dr
Copy link
Contributor Author

rm-dr commented Feb 22, 2024

restricts the path to relative paths using cap_std.

Do we really want to do this? I don't see a reason to restrict input to relative paths, it's generally known that absolute paths will break on different systems.

@ratmice
Copy link
Contributor

ratmice commented Feb 22, 2024

restricts the path to relative paths using cap_std.

Do we really want to do this? I don't see a reason to restrict input to relative paths, it's generally known that absolute paths will break on different systems.

We generally know my opinion, but I wanted to add some additional context.

What I should probably say is not just relative paths but specifically subpaths of the cwd where Tectonic.toml is found. So under cap_std ../bar is still not a valid path even though it is relative. This provides some benefit to reproducibility in that if you have a directory with a Tectonic.toml it should reproduce regardless of any external directories even if all paths are relative.

And we could always add some form of escape hatch like unrestricted_paths along the same lines of shell-escape to disable it.
But indeed it would be good to make a decision on this

@rm-dr
Copy link
Contributor Author

rm-dr commented Feb 23, 2024

What I should probably say is not just relative paths but specifically subpaths of the cwd where Tectonic.toml is found.

Ah, I see. I'd argue against this: there's no need to enforce reproducibility that strictly. Tectonic should have reproducible defaults & easy-to-track bundles, but I don't see a reason to enforce relative paths.

In fact, if you need this feature, you'll most certainly be using assets outside of the root Tectonic.toml dir! Assets inside the document dir could easily be moved into src, and shared assets shouldn't ever find themselves under a single document's directory (they're shared, after all).

@ratmice
Copy link
Contributor

ratmice commented Feb 23, 2024

What I should probably say is not just relative paths but specifically subpaths of the cwd where Tectonic.toml is found.

Ah, I see. I'd argue against this: there's no need to enforce reproducibility that strictly. Tectonic should have reproducible defaults & easy-to-track bundles, but I don't see a reason to enforce relative paths.

In fact, if you need this feature, you'll most certainly be using assets outside of the root Tectonic.toml dir! Assets inside the document dir could easily be moved into src, and shared assets shouldn't ever find themselves under a single document's directory (they're shared, after all).

Mostly I've been using this with git clones of packages, I don't want the package actually in the src/ dir which should be reserved for my document, yes being shared is wonky, but if I use different versions of packages in different projects it is very likely to break when other projects use different versions of the same package. I have used this scheme at times with both the kaobook and acro packages.

@rm-dr rm-dr added the bundles and files Bundle issues, finding fonts, etc. label Feb 27, 2024
@rm-dr
Copy link
Contributor Author

rm-dr commented Mar 1, 2024

I see... but I'd still argue that strict relative path checks aren't necessary here---most use cases would raise "not reproducible" errors, as I detailed above.

@pkgw, I think this is ready to merge! This is a small and non-breaking config API change, but consensus on the details would still be nice.

@rm-dr rm-dr linked an issue Mar 1, 2024 that may be closed by this pull request
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle shared resources for multiple files
2 participants