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

[spec] Clarify path resolution of include statements #164

Closed
brawer opened this issue Feb 13, 2017 · 6 comments
Closed

[spec] Clarify path resolution of include statements #164

brawer opened this issue Feb 13, 2017 · 6 comments
Assignees
Labels
fea spec urgent fix needed very important and urgent

Comments

@brawer
Copy link
Contributor

brawer commented Feb 13, 2017

It might be good to adjust the wording of Section 3 of the feature file syntax. Currently, it could be interpreted as if include statements in feature files worked like in other programming languages, such as the C preprocessor:

If the file name were relative, then it would be appended to the directory of the including feature file.

However, according to @miguelsousa in fonttools/fonttools#838, this is actually not the case. Instead, include paths are getting resolved relative to the first file being compiled; see Miguel’s example. It would be good if the feature file spec could make the (somewhat unusual) behavior of include statements more explicit.

@readroberts
Copy link
Contributor

readroberts commented Jul 26, 2018

@brawer @miguelsousa @bobh0303 @behdad @anthrotype @justvanrossum @typesupply
I finally read the threads associated with this issue, and fonttools 838, and was startled to see the decision that feature file include statements with relative paths should always be relative to the base feature file, which is contrary to the feature file spec. Although it is true that makeotf supports this relative-to-base-file model, this was for backwards compatibility; makeotf does in fact support relative-to-including-file model as well, and has for some time. I was going to push back on this decision, because there is good reason to prefer the relative-to-including-file system - it is not only familiar from programming environments, but is much easier to maintain complex inheritance trees that way, as it allows you to change references to one directory node without having to adjust any of the references to filed lower on the tree. However, in testing makeotf's current behavior, I noticed that Miguel had made clever use of the relative-to-base-file model in SourceSerifPro, and now think that this model is actually better for a directory tree of feature include files. I think follows from a big difference in font data vs program code: the includes always point upwards.

A case where the relative-to-base-file model is better is referencing files that are specific to a font instances, such as:
include (os2.fea);
include (mark.fea);
include (kern.fea);
In the relative-to-including-file system, these include statements must be present in each instance's base feature file. In order to add or remove a reference at this level, each of the base feature files in the family must be edited. However, in the relative-to-base-file model, each base feature file can contain the single reference:
include (../../../../base_features.fea);

Then 'base_features.fea' alone can contain the references:
include (os2.fea);
include (mark.fea);
include (kern.fea);
Adding or remove a reference at the instance level then requires editing only this file. This approach can be used widely, and I now think it makes maintaining the chain of includes easier than the relative-to-including-file system. I initially had some trouble imagining how this model works, but one of Miguel's comments mentions a clear way to understand how this model is applied: imagine that include files work just like the 'source' command in the Unix shell script, where the text from the included file is inserted into the base file, and executed in the same context, so that all includes are relative to the base file, because they are interpreted in the context of the base file.

I now favor changed the feature file spec to support the relative-to-base-file model. Because of history, however, I would also say that implementations should additionally support the relative-to-including-file system. More specifically, a feature file should first be located using the relative-to-base-file model, If this fails, the implementation should check to see if can be found with the relative-to-including-file.

AS an additional issue, I suggest changing the feature file spec to state a specific exception for UFO fonts, that include statements are relative to the directory of the font directory, rather than the base feature file. I don't really like this much, but I understand that paths which are relative to a hidden file may be confusing to UFO users.

Comments?

@miguelsousa
Copy link
Member

@readroberts sounds fine to me.

@behdad
Copy link
Contributor

behdad commented Aug 2, 2018

I see the use for both models. I, again, propose the following:

  • include "..." relative to current file.
  • include <...> relative to search path. By default, this is the directory of the original feature file.
  • include (...) whatever current behavior / relative to UFO outer directory.

@benkiel
Copy link
Contributor

benkiel commented Aug 2, 2018

As long as it's clear that only include (...) is for use in UFO, due to the model being conceptually one file (important for compressed UFO, once that PR is merged...)

Yes, this is likely to be non-ideal in some use cases, but it also means that data that isn't part of the UFO Spec won't be ignored/deleted.

Do like the proposed syntax!

@miguelsousa miguelsousa added the urgent fix needed very important and urgent label Sep 26, 2018
@miguelsousa
Copy link
Member

@readroberts an urgent fix for this issue is needed. Currently it's impossible to run fontmake and makeotf on the same source files of an UFO-based project, due to the non-compatible handling of include().

@brawer
Copy link
Contributor Author

brawer commented Sep 26, 2018

a feature file should first be located using the relative-to-base-file model, If this fails, the implementation should check to see if can be found with the relative-to-including-file.

Sounds good to me.

miguelsousa pushed a commit that referenced this issue Oct 5, 2018
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
miguelsousa pushed a commit that referenced this issue Oct 5, 2018
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
miguelsousa pushed a commit that referenced this issue Oct 5, 2018
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fea spec urgent fix needed very important and urgent
Projects
None yet
Development

No branches or pull requests

5 participants