engines/mod: Make `format_to_extension` return Vec by mrshu · Pull Request #93 · tectonic-typesetting/tectonic

mrshu · 2017-06-11T15:00:27Z

Make format_to_extension return a Vector of strings.
Make use of set_extension function of PathBuf instead of creating an
OsString.
Add multiple vectorized values as results of format_to_extension.
Fixes Support more image filename extensions #78

Signed-off-by: mr.Shu mr@shu.io

mrshu · 2017-06-11T15:02:31Z

+            }
+
+            if let FileFormat::Format = format {
+                if let OpenResult::Ok(r) = self.io.input_open_format(&ext.into_os_string(), self.status) {


@pkgw There is most probably a better way of doing this than by repeating OpenResult::Ok(r) twice (same below). Unfortunately, I was not able to figure one out. Do let me know if you can, I will gladly change this.

I guess @-bindings are the preferred solution in this case:

if let result @ OpenResult::Ok(_) = compute_result() { return result }

@Mrmaxmeier Oh, may thanks!

Sorry, I'm not familiar with the @ syntax and I can't see the key part of the diff. @Mrmaxmeier is your example syntax equivalent to this?

if let OpenResult::Ok(r) = compute_result() { return OpenResult::Ok(r) }

Yup, @pkgw.^*

*Recreating the enum variant inside of the if let statement could lead to lost/different type information but in this case both versions are equivalent.

mrshu · 2017-06-11T15:10:08Z

+        FileFormat::Format => vec!["fmt.gz"],
+        FileFormat::FontMap => vec!["map"],
+        FileFormat::MiscFonts => vec!["miscfonts"], /* XXX: no kpathsea suffixes */
+        FileFormat::Ofm => vec!["ofm"], /* XXX: also .tfm */


@pkgw I tried adding tfm here too, but it turns out that adding it here fails the test_space test -- the code actually fails at this point (https://github.com/tectonic-typesetting/tectonic/blob/master/tectonic/dpx-tfm.c#L579).

I therefore omitted it here.

Ah ... I believe that by fixing this bug you're exposing a lingering problem in the font parsing code. I will look at this a bit more but I think the thing to do would be to file an issue about the problem and #[ignore] the failing test until the issue gets fixed.

@pkgw Thanks, I also added a FIXME to the appropriate #[ignore] line.

Mrmaxmeier · 2017-06-11T15:13:24Z

+        FileFormat::Tex => vec!["tex", "sty", "cls", "fd", "aux", "bbl", "def", "clo", "ldf"], /* also .{sty,cls,fd,aux,bbl,def,clo,ldf} */
+        FileFormat::TexPsHeader => vec!["pro"],
+        FileFormat::TFM => vec!["tfm"],
+        FileFormat::TrueType => vec!["ttf", "ttc", "TTF", "TTC", "dfont"], /* XXX: also .ttc, .TTF, .TTC, .dfont */


A bunch of the trailing XXX comments can now be removed.

@Mrmaxmeier roger that, will do. Thanks!

@Mrmaxmeier I left at least a few ones that are still relevant.

* Make `format_to_extension` return a Vector of strings. * Make use of `set_extension` function of PathBuf instead of creating an OsString. * Add multiple vectorized values as results of `format_to_extension`. * Fixes tectonic-typesetting#78 Signed-off-by: mr.Shu <mr@shu.io>

mrshu · 2017-06-11T15:18:57Z

@Mrmaxmeier @pkgw I am wondering if we can put together a test for this -- especially given the problem with tfm, I believe it would make these changes much more reliable and the whole PR safer to merge.

* Make sure `.tfm` are treated in the same way as `.tfm` are. * Since the change mentioned above breaks the `test_space`, it had to be ignored. Signed-off-by: mr.Shu <mr@shu.io>

…onic into mrshu/format_to_extension-refactor

As described in tectonic-typesetting#95, more tests need to be updated. Signed-off-by: mr.Shu <mr@shu.io>

mrshu

@pkgw In the end, there are three #[ignore]d tests.

I also created issue #95 to track these. It seems to me that it would be better to fix the read_ofm issue (i.e. porting it to the new I/O) before merging this PR.

mrshu · 2017-06-13T06:18:25Z

 }

 #[test] // GitHub #31
+#[ignore] // FIXME: GitHub #95


@pkgw it turns out I had to ignore a couple more tests in order for the tests to pass.

pkgw · 2017-06-13T17:46:14Z

So, sorry — upon further investigation, I think that I was just wrong before. I now think the Ofm format should not have tfm as an allowed extension, which means you should be able to un-ignore the executable tests. Apologies for misleading you there.

* Per current discussions in tectonic-typesetting#93, the `tfm` extension should not signify a .ofm file. * Tests can therefore be unignored. Signed-off-by: mr.Shu <mr@shu.io>

rekka · 2017-06-14T02:33:27Z

Commit 7c2e4e6 seems to break tests/xenia/paper.tex. Should I look into it or do you already know the cause?

Engine output:

(paper.tex
LaTeX2e <2016/03/31>
Babel <3.9r> and hyphenation patterns for 83 language(s) loaded.
(aastex6.cls

LaTeX Warning: You have requested document class `aastex6',
               but the document class provides `aastex'.

Document Class: aastex 2016/02/16 1.0/AAS markup document class
(times.sty) (lineno.sty) (revtex4-1.cls
Document Class: revtex4-1 2010/07/25/20:33:00 4.1r (http://publish.aps.org/revt
ex4/ for documentation)
ltxutil[2010/07/25/20:33:00 4.1r utilities package (portions licensed from W. E
. Baxter web at superscript.com)]
ltxfront[2010/07/25/20:33:00 4.1r frontmatter package (AO,DPC)]
ltxgrid[2010/07/25/20:33:00 4.1r page grid package (portions licensed from W. E
. Baxter web at superscript.com)]

Class revtex4-1 Warning: Failed to recognize \@vspace, \@vspacer, \@no@pgbk, \@
newline, and \\; no patches applied. Please get a more up-to-date class, .

(aps4-1.rtx) (aps10pt4-1.rtx) (textcase.sty) (url.sty) (natbib.sty
Local config file natbib.cfg used
(natbib.cfg

! LaTeX Error: Command \bibstyle@chicago already defined.
               Or name \end... illegal, see p.192 of the manual.

See the LaTeX manual or LaTeX Companion for explanation.
Type  H <return>  for immediate help.
 ...                                              
                                                  
l.206 ...tyle@chicago{\bibpunct{(}{)}{;}{a}{,}{,}}
                                                  
No pages of output.
Transcript written on paper.log.

rekka · 2017-06-14T04:25:07Z

Investigating this, the problem is that the file natbib.cfg does not exist, but by trying natbib with various tex extensions, the file natbib.sty is opened instead. But this is clearly not what the code expects, since it is opening natbib.cfg from inside natbib.sty.

Unfortunately this breaks tectonic for me. :)

I believe that we should not change the extension of the file, if provided. Actually, how is the FileFormat used in the original XeTeX?

pkgw · 2017-06-14T16:16:45Z

Gah, I did not test thoroughly enough. Thanks for #100.

use `Into<Vec<u8>>` instead of `&str`

mrshu commented Jun 11, 2017

View reviewed changes

Mrmaxmeier reviewed Jun 11, 2017

View reviewed changes

mrshu force-pushed the mrshu/format_to_extension-refactor branch from 5207013 to 592b50b Compare June 11, 2017 15:15

mrshu force-pushed the mrshu/format_to_extension-refactor branch from 592b50b to 7c2e4e6 Compare June 11, 2017 15:16

mrshu mentioned this pull request Jun 12, 2017

Adding the ability to process .tfm results in failed test_space #95

Closed

src/engines/mod: Consider .tfm an .otf format

5becc5e

* Make sure `.tfm` are treated in the same way as `.tfm` are. * Since the change mentioned above breaks the `test_space`, it had to be ignored. Signed-off-by: mr.Shu <mr@shu.io>

mrshu force-pushed the mrshu/format_to_extension-refactor branch from 4626113 to 5becc5e Compare June 13, 2017 05:33

mrshu added 2 commits June 13, 2017 07:49

Merge branch 'master' of https://github.com/tectonic-typesetting/tect…

95d57c6

…onic into mrshu/format_to_extension-refactor

tests/executable: Ignoring more tests

d950b58

As described in tectonic-typesetting#95, more tests need to be updated. Signed-off-by: mr.Shu <mr@shu.io>

mrshu commented Jun 13, 2017

View reviewed changes

pkgw mentioned this pull request Jun 13, 2017

Change xdvipdfmx's OFM-loading code to use the Rust I/O subsystem #97

Merged

src/engines/mod.rs: tfm not allowed ofm ext.

0c2ab49

* Per current discussions in tectonic-typesetting#93, the `tfm` extension should not signify a .ofm file. * Tests can therefore be unignored. Signed-off-by: mr.Shu <mr@shu.io>

pkgw merged commit 503354f into tectonic-typesetting:master Jun 13, 2017

rekka mentioned this pull request Jun 14, 2017

Do not change the extension of files requested by the engine. #100

Merged

Mrmaxmeier pushed a commit to Mrmaxmeier/tectonic that referenced this pull request Oct 1, 2019

Merge pull request tectonic-typesetting#93 from burrbull/lookup

b89087d

use `Into<Vec<u8>>` instead of `&str`

Uh oh!

Conversation

mrshu commented Jun 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mrmaxmeier Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

*Recreating the enum variant inside of the if let statement could lead to lost/different type information but in this case both versions are equivalent.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrshu commented Jun 11, 2017

Uh oh!

mrshu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkgw commented Jun 13, 2017

Uh oh!

rekka commented Jun 14, 2017

Uh oh!

rekka commented Jun 14, 2017

Uh oh!

pkgw commented Jun 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Mrmaxmeier Jun 12, 2017 •

edited

Loading

*Recreating the enum variant inside of the `if let` statement could lead to lost/different type information but in this case both versions are equivalent.