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

Fix #50: Normalize paths in MemoryIo #77

Merged
merged 2 commits into from
Jun 7, 2017

Conversation

rekka
Copy link
Contributor

@rekka rekka commented Jun 7, 2017

  • Only paths that are valid UTF8 are normalized.
  • std::path::{Path,PathBuf} are not used to normalize in a system
    independent way on Unix/Windows.
  • LaTeX identifies OS by testing if ./texsys.aux exists. This file
    is created as texsys.aux. Without normalization, OS is
    misidentified and filename parsing in LaTeX code then produces wrong
    results. (See pdfpages cannot include paths with a . #50). This fixes the behavior (cache needs to be
    rebuilt).

Note: Normalization currently does not handle path prefix on Windows, but this can be added later (paths on Windows are hard, need to explore what is reasonable).

  - Only paths that are valid UTF8 are normalized.
  - `std::path::{Path,PathBuf}` are not used to normalize in a system
    independent way on Unix/Windows.
  - LaTeX identifies OS by testing if `./texsys.aux` exists. This file
    is created as `texsys.aux`. Without normalization, OS is
    misidentified and filename parsing in LaTeX code then produces wrong
    results. (See tectonic-typesetting#50). This fixes the behavior (cache needs to be
    rebuilt).
@pkgw
Copy link
Collaborator

pkgw commented Jun 7, 2017

Brilliant! The code looks great, only a few high-level thoughts:

  • I think normalize_path should be called try_normalize_tex_path. A function named try_... should return an Option or Result, I think, so I'd prefer to swap the nomenclature there. And then I think we want to establish a terminology of a "TeX path" being one that we force to obey our simplified semantics: Unix-like syntax (/ for separators etc), must be Unicode-able, no symlinks allowed such that .. can be stripped lexically. (Also, perhaps: no newlines allowed. But whitespaces should definitely be allowed; not sure if there is a clean way to come up with forbidden characters.)

  • Then I guess try_normalize_path should become normalize_tex_path, and documentation string should note that the function should operate on &str someday, but we need to transition the internals away from OsStr{,ing} before that can happen.

  • Can you put in some tests for paths containing two consecutive slashes, like abc//xyz?

@rekka
Copy link
Contributor Author

rekka commented Jun 7, 2017

Thanks so much for the review and comments.

Hopefully this commit addresses all of them.
I used a few of them verbatim in the code.

I was sure that I had // tests, nice catch---I sprinkled a few of them around.

@pkgw
Copy link
Collaborator

pkgw commented Jun 7, 2017

Great, I'll merge this momentarily! Three random thoughts:

  • I believe that if you write a freestanding line in the commit message that says Closes #50., GitHub will automatically close issue pdfpages cannot include paths with a . #50 when the commit is merged into master.

  • I believe that it's reliable that /.. = /, so the code path that returns None in try_... could potentially be removed. But that's not a big deal.

  • In fact I'm not a huge fan of letting people write absolute paths at all in their TeX code ... but this seems like an area where we should probably be pragmatic, since I can imagine people that just want to get their damn file to compile and they (feel that they) need to reference some file with an absolute path to get it to work. Something to ponder.

@pkgw pkgw merged commit 4742c9b into tectonic-typesetting:master Jun 7, 2017
@rekka rekka deleted the fix-50 branch June 7, 2017 14:35
@rekka
Copy link
Contributor Author

rekka commented Jun 7, 2017

Thanks! Now I should be able to use tectonic in my workflow.

  • Thanks for the tip.

  • You're are right, it seems that /.. = /. Paths are hard...

  • Absolute paths should be allowed. Maybe a warning is fine, but that also might quickly get quite annoying.

Mrmaxmeier pushed a commit to Mrmaxmeier/tectonic that referenced this pull request Oct 1, 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