Skip to content

Commit

Permalink
[makeotfexe] Clarify path resolution of include statements
Browse files Browse the repository at this point in the history
Fix issue that feature files referenced from a UFO font won't work in makeotfexe.

Note that for each include statement encountered, the current logic will try in order:
- If the source font is UFO format, resolve path relative to the UFO's font directory
- resolve path relative to the top-level include file
- resolve path relative to the parent include file

This does mean that a font project will build even if it mixes all three models in different include files. The problem is that there is no way to know which model is intended until the first relative include path is encountered that will work with only one of the models, and even then it would be possible to draw the wrong conclusion. Ultimately, the most reliable approach would be to provide options so that a user could force one method or another. I think that scenarios where this will be a problem are sufficiently rare so as to not to be worth dealing with.

Fixes #164
  • Loading branch information
readroberts authored and miguelsousa committed Oct 5, 2018
1 parent 98e8d12 commit e4506d1
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 22 deletions.
32 changes: 23 additions & 9 deletions c/makeotf/source/cb.c
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
/*
Copyright 2014 Adobe Systems Incorporated (http://www.adobe.com/). All Rights Reserved.
This software is licensed as OpenSource, under the Apache License, Version 2.0.
This software is licensed as OpenSource, under the Apache License, Version 2.0.
This license is available at: http://opensource.org/licenses/Apache-2.0.
*/
/***********************************************************************/
Expand Down Expand Up @@ -452,17 +452,31 @@ static char *findFeatInclFile(cbCtx h, char *filename) {
/* Check if relative path */
if ((filename[0] != '/') && (filename[0] != '\\') && (filename[1] != ':')) {
int i;
/* Look first relative to to the current include directory. If
not found, check relative to the main feature file.
*/
for (i = 1; i >= 0; i--) {
if ((h->feat.includeDir[i] != 0) &&
(h->feat.includeDir[i][0] != '\0')) {
sprintf(path, "%s%s%s", h->feat.includeDir[i], sep(), filename);
/* h->feat.includeDir[0] contains the parent directory of the main feature file. */
/* h->feat.includeDir[1] contains the parent directory of the including parent feature file. */
if ((h->feat.includeDir[0] != 0) &&
(h->feat.includeDir[0][0] != '\0')) {
/* Relative to UFO parent dir, if that is what it is */
sprintf(path, "%s%s%s", h->feat.includeDir[0], sep(), "fontinfo.plist");
if (fileExists(path)) {
sprintf(path, "%s%s..%s%s", h->feat.includeDir[0], sep(), sep(), filename);
if (fileExists(path)) {
goto found;
}
}
/* Relative to the main feature file */
sprintf(path, "%s%s%s", h->feat.includeDir[0], sep(), filename);
if (fileExists(path)) {
goto found;
}
}
/* Relative to parent include file */
if ((h->feat.includeDir[1] != 0) &&
(h->feat.includeDir[1][0] != '\0')) {
sprintf(path, "%s%s%s", h->feat.includeDir[1], sep(), filename);
if (fileExists(path)) {
goto found;
}
}
return NULL; /* Can't find include file (error) */
} else {
Expand Down
2 changes: 1 addition & 1 deletion c/makeotf/source/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@

jmp_buf mark;

#define MAKEOTF_VERSION "2.5.65595"
#define MAKEOTF_VERSION "2.5.65596"
/* Warning: this string is now part of heuristic used by CoolType to identify the
first round of CoolType fonts which had the backtrack sequence of a chaining
contextual substitution ordered incorrectly. Fonts with the old ordering MUST match
Expand Down
18 changes: 14 additions & 4 deletions docs/OpenTypeFeatureFileSpecification.html
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ <h1>OpenType&trade; Feature File Specification</h1>
This software is licensed as OpenSource, under the Apache License, Version 2.0. This license is available at: http://opensource.org/licenses/Apache-2.0.
</p>
<p><b>Version</b></p>
<p>[Document version 1.22 Last updated 22 July 2018]</p>
<p>[Document version 1.23 Last updated 1 Oct 2018]</p>
<p><b>Caution:</b></p>
<p>Portions of the syntax unimplemented by Adobe are subject to change.</p>
<ol>
Expand Down Expand Up @@ -521,10 +521,16 @@ <h1>OpenType&trade; Feature File Specification</h1>
<p>Including files is indicated by the directive:</p>
<pre>
<b>include</b>(&lt;filename&gt;);</pre>
<p>The implementation software is responsible for handling the search paths for the location of the included files.</p>
<p>In a typical implementation, if the file name were absolute then that path would be used. If the file name were relative, then it would be appended to the directory of the including feature file.</p>
<p>For example:</p>
<pre>
include<code>(../features.family);</code></pre>
include(../features.family);</pre>
<p>The implementation software is responsible for handling the search paths for the location of the included files.</p>
<p>Because of variations in implementations over time, a relative include path should be resolved in the order described below; the first which matches should be used.</p>
<ol>
<li>If the source font is UFO format, then relative to the UFO's font directory</li>
<li>relative to the top-level include file</li>
<li>relative to the parent include file</li>
</ol>
<p>A maximum include depth of 5 ensures against infinite include loops (files that include each other).</p>
<p><a name="4"></a><b>4. Specifying features</b></p>
<p><a name="4.a"></a><b>4.a. feature</b></p>
Expand Down Expand Up @@ -1763,6 +1769,10 @@ <h1>OpenType&trade; Feature File Specification</h1>
}</pre>
<p>along with the tag 'sbit'.</p>
<p><a name="11"></a><b>11. Document revisions</b></p>
<p><b>v1.23 [1 Oct 2018]:</b></p>
<ul>
<li>Clarified the resolution of include file paths, as described in <a href="https://github.com/adobe-type-tools/afdko/issues/164">GitHub issue #164</a>.</li>
</ul>
<p><b>v1.22 [20 July 2018]:</b></p>
<ul>
<li>Removed the restriction that 'DFLT' script is allowed to have only the 'dflt' language. This now matches the changes made in version 1.8.2 of the OpenType spec.</li>
Expand Down
12 changes: 4 additions & 8 deletions tests/makeotf_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -519,12 +519,8 @@ def test_feature_includes_ufo_bug164():
input_filename = "bug164/d1/d2/font.ufo"
otf_path = get_temp_file_path()

stderr_path = runner(
CMD + ['-s', '-e', '-o',
'f', '_{}'.format(get_input_path(input_filename)),
'o', '_{}'.format(otf_path)])
runner(CMD + ['-o',
'f', '_{}'.format(get_input_path(input_filename)),
'o', '_{}'.format(otf_path)])

with open(stderr_path, 'rb') as f:
output = f.read()
assert(b"[FATAL] <SourceSans-Test> include file <../../rel_to_main1.fea> "
b"not found [font.ufo/features.fea 1]") in output
assert font_has_table(otf_path, 'head')

0 comments on commit e4506d1

Please sign in to comment.