feat(developer): kmc-package support remote fonts and files 🐻 #12667
Conversation
The concept here is that the 'Name' property for a file can now be a remote reference, rather than a local file. There are two supported formats in this commit: * GitHub: This is a cutdown version of a plain github.com URL, and must match this exact format: ``` github:<owner>/<repo>/raw/<hash>/<filepath/filename> ``` This format is mandated in order to ensure that we always have a hashed version of a file from the origin. This gives us reproducible builds, which avoids churn issues when font files change. Example: `github:silnrsi/fonts/raw/b88c7af5d16681bd137156929ff8baec82526560/fonts/sil/alkalami/Alkalami-Regular.ttf` gets https://github.com/silnrsi/fonts/raw/b88c7af5d16681bd137156929ff8baec82526560/fonts/sil/alkalami/Alkalami-Regular.ttf An alternative could be to just have `https://github.com/silnrsi/fonts/raw/b88c7af5d16681bd137156929ff8baec82526560/fonts/sil/alkalami/Alkalami-Regular.ttf` which could be matched with a regex in the same way as the `github` prefix, and would avoid the need to munge the input URL. **Discuss!** * fonts.languagetechnology.org: references just a font identifier. This is somewhat broken, because if the source file changes, we don't know about it and won't publish an updated version of the package. So this needs some more discussion (we could e.g. embed the version number in the request, e.g. `flo:[email protected]`). **Discuss!** ``` flo:<family> ``` e.g. `flo:andika` gets https://fonts.languagetechnology.org/fonts/sil/andika/Andika-Bold.ttf Future sources could be considered, e.g. noto. We don't want to allow arbitrary URLs, both for stability and for security reasons. This change is entirely compiler-side, so we don't need to make any changes to apps, and so packages will be backwardly compatible. A lot of work will need to be done with the Package Editor in TIKE to support this feature. Fixes: #11236
User Test ResultsTest specification and instructions User tests are not required |
|
Capturing discussion on font versioning. We need to think this through with FLO, caching of fonts, versioning updates. Marc Durdin Marc Durdin Victor Gaultney Marc Durdin Victor Gaultney Marc Durdin Victor Gaultney Marc Durdin Victor Gaultney Marc Durdin Victor Gaultney Marc Durdin Victor Gaultney Marc Durdin Victor Gaultney Marc Durdin Victor Gaultney 4:33 |
|
Another capture marc ross eberhard marc |
|
marc davidrowe marc eberhard silnrsi/fonts marc |
|
My current thinking on flo: references. Perhaps FLO needs to be a tool in Developer which resolves to a GH stable commit reference, as fonts with a url are all GitHub references in FLO. If we always use GH, then it will be exceptionally rare to have a stable uri disappear -- only if repos are removed or nasty actions like force pushes are done -- in which case we probably want to deal with it anyway. Additional feature: we can keep the original flo references as well, and use that to provide a hint message to flag when font updates are available, but do not attempt to use it directly. |
…at/developer/kmc-package-source-files-from-remotes-2
…es-from-remotes-2' of https://github.com/keymanapp/keyman into feat/developer/kmc-package-source-files-from-remotes-2
After the reorganization of the package compiler build, we now need to make sure all files referenced in the .kps exist in order for the unit tests to pass.
…matches As our data files often come from Windows, we need to test if paths are Windows-style absolute paths, with either path separator (e.g. `C:\...`, `\path\...`, `\\server\path\...`, `/path/...`). Technically, this means a posix path that starts with an escaped character would be regarded as absolute, but IMHO those kinds of shenanigans deserve to result in a failing build anyway. ;-)
…ge-source-files-from-remotes-2
Adds error for unsupported versions, hint for low versions when using remote references.
mcdurdin
requested review from
darcywong00,
SabineSIL,
markcsinclair,
srl295 and
jahorton
as code owners
|
|
||
| ## local: `<Source>` element is omitted | ||
|
|
||
| This is the same as previous versions. `<Name>` references a file on the local |
There was a problem hiding this comment.
What does "... same as previous versions." refer to?
There was a problem hiding this comment.
Hmm, this means v7, v12 of the format. I can update.
| * github | ||
| * noto - in the future | ||
|
|
||
| ## local: `<Source>` element is omitted |
There was a problem hiding this comment.
This took me a few re-reads of the part about "<Source> element is omitted."
That phrase doesn't appear in the other source headings (flo, github, noto).
Is this conveying for local sources, the <Source> element will always be omitted?
vs other sources the <Source> element can be omitted?
|
|
||
| ## local: `<Source>` element is omitted | ||
|
|
||
| This is the same as previous versions. `<Name>` references a file on the local |
There was a problem hiding this comment.
Hmm, this means v7, v12 of the format. I can update.
| This is the same as previous versions. `<Name>` references a file on the local | ||
| filesystem. Note that `<Source>` is optional for other sources as well; the | ||
| absence of the element does not mean local -- that is determined by pattern | ||
| match. |
There was a problem hiding this comment.
| This is the same as previous versions. `<Name>` references a file on the local | |
| filesystem. Note that `<Source>` is optional for other sources as well; the | |
| absence of the element does not mean local -- that is determined by pattern | |
| match. | |
| If `<Name>` references a file on the local filesystem, then the | |
| `<Source>` element must be omitted. This matches versions 7.0 and 12.0 | |
| of the .kps format for backward compatibility. | |
| Note that `<Source>` is optional for other sources as well; the | |
| absence of the element does not mean local -- that is determined by pattern | |
| match. |
There was a problem hiding this comment.
Note that
<Source>is optional for other sources as well; the
See above. Maybe better: "Note that <Source> is optional for some of the other sources as well; the" ?
There was a problem hiding this comment.
I'd split out the compatibility part, as it's confusing when mentioning local files (and becomes less important over time by nature, though it's in a prominent location). Using a local filesystem matches 7.0 and 12.0? Or omitting <Source>? Or both?
Perhaps move that sentence to the end:
For backwards compatibility with versions 7.0 and 12.0 of the .kps format, omit the
<Source>element.
| We will add a `Source` element to .kps `Package.Files.File`. We would support | ||
| several sources: | ||
| * local - local file system | ||
| * flo - fonts.languagetechnology.org | ||
| * github | ||
| * noto - in the future |
There was a problem hiding this comment.
| We will add a `Source` element to .kps `Package.Files.File`. We would support | |
| several sources: | |
| * local - local file system | |
| * flo - fonts.languagetechnology.org | |
| * github | |
| * noto - in the future | |
| There may a separate `<Source>` element as well, representing the following possible original sources for a file: | |
| * Local file system -- omit the `<Source>` element | |
| * fonts.languagetechnology.org -- `<Source>` should have the pattern `flo:...` | |
| * `https://github.com/...` -- `<Source>` should have a URL to a file stored in GitHub | |
| * `noto:...` - to be supported in the future | |
| The `<Source>` element is always optional; if it is not present, then kmc-package will not check for updated versions of the source file. |
There was a problem hiding this comment.
The
<Source>element is always optional; if it is not present, then kmc-package will not check for updated versions of the source file.
The first half of the sentence doesn't make sense to me after looking at the examples. If I want to specify a font from flo it seems I have to use the <Source> element, so it's not optional in that case.
There may a separate
<Source>element as well,
I think there's a "be" missing...
| `Keyboard file ${def(o.filename)} was not found. Has it been compiled?`); | ||
| static ERROR_KeyboardFileNotFound = SevError | 0x0011; | ||
| static Error_KeyboardFileNotFound = (o:{filename:string}) => m(this.ERROR_KeyboardFileNotFound, | ||
| `Keyboard file ${def(o.filename)} was not found. Has it been compiled?`); |
There was a problem hiding this comment.
nit to revert whitespace?
| <FileVersion>18.0</FileVersion> | ||
| </System> | ||
| <Info> | ||
| <Name URL="">error_font_file_could_not_be_downloaded</Name> |
There was a problem hiding this comment.
| <Name URL="">error_font_file_could_not_be_downloaded</Name> | |
| <Name URL="">hint_source_file_has_changed</Name> |
| <Description>File splash.gif</Description> | ||
| <CopyLocation>0</CopyLocation> | ||
| <FileType>.gif</FileType> | ||
| </File> | ||
| </Files> |
There was a problem hiding this comment.
what keyboard content file is not found for this test fixture?
There was a problem hiding this comment.
khmer_angkor.kmx is missing, have added a comment
| </File> | ||
| <!-- warn_font_in_flo_does_not_have_default_ttf: this uses a mismatching font, but | ||
| we won't get that far. This handles the scenario where FLO previously lists a font | ||
| but no ttf is available --> |
There was a problem hiding this comment.
In the future when FLO makes updates, are there scenarios that could that impact some text fixture assumptions?
For example, a listed font that previously doesn't have ttf changes to include one?
There was a problem hiding this comment.
The fixtures are all stored offline to avoid this arising (and have performant and consistent tests), see https://github.com/keymanapp/keyman/blob/c6baddad1237e09a6cdc14ccc64dd6d63253d63a/developer/src/kmc-package/test/fixtures/online/fonts.languagetechnology.org/%23families.json for example
|
@ermshiperete and @srl295 would love your feedback on this before merge, particularly considering systemic issues (docs are included in the PR; most of the files are fixtures for tests). See also the TODO list in #11236 for some things I am already thinking about. |
mcdurdin
changed the title
| This is the same as previous versions. `<Name>` references a file on the local | ||
| filesystem. Note that `<Source>` is optional for other sources as well; the | ||
| absence of the element does not mean local -- that is determined by pattern | ||
| match. |
There was a problem hiding this comment.
I'd split out the compatibility part, as it's confusing when mentioning local files (and becomes less important over time by nature, though it's in a prominent location). Using a local filesystem matches 7.0 and 12.0? Or omitting <Source>? Or both?
Perhaps move that sentence to the end:
For backwards compatibility with versions 7.0 and 12.0 of the .kps format, omit the
<Source>element.
| </File> | ||
| ``` | ||
|
|
||
| ### fonts.languagetechnology.org `flo:<id>` |
There was a problem hiding this comment.
I think I am not going to bikeshed the URI and schema structure here. These are reasonable, a small set, and unlikely to collide with anything.
| ```xml | ||
| <File> | ||
| <Name>https://github.com/silnrsi/fonts/raw/b88c7af5d16681bd137156929ff8baec82526560/fonts/sil/alkalami/Alkalami-Regular.ttf</Name> | ||
| <Source>https://github.com/silnrsi/fonts/raw/b88c7af5d16681bd137156929ff8baec82526560/fonts/sil/alkalami/Alkalami-Regular.ttf</Source> |
There was a problem hiding this comment.
is this then a redundant <Source> just for documentation? What's the benefit of the <Source> element here?
There was a problem hiding this comment.
No benefit here -- it'd just be for documentation. Possibly misleading. Will consider removing this one.
| A GitHub raw reference, from a branch, with refs/heads/ excluded: | ||
|
|
||
| ```xml | ||
| <File> | ||
| <Name>https://github.com/silnrsi/fonts/raw/b88c7af5d16681bd137156929ff8baec82526560/fonts/sil/alkalami/Alkalami-Regular.ttf</Name> | ||
| <Source>https://github.com/silnrsi/fonts/raw/main/fonts/sil/alkalami/Alkalami-Regular.ttf</Source> | ||
| </File> | ||
| ``` | ||
|
|
||
| A GitHub raw reference, from a branch, with refs/heads/ included: | ||
|
|
There was a problem hiding this comment.
Not sure refs/head really needs to be called out. High bit for users is that both of these work.
| A GitHub raw reference, from a branch, with refs/heads/ excluded: | |
| ```xml | |
| <File> | |
| <Name>https://github.com/silnrsi/fonts/raw/b88c7af5d16681bd137156929ff8baec82526560/fonts/sil/alkalami/Alkalami-Regular.ttf</Name> | |
| <Source>https://github.com/silnrsi/fonts/raw/main/fonts/sil/alkalami/Alkalami-Regular.ttf</Source> | |
| </File> | |
| ``` | |
| A GitHub raw reference, from a branch, with refs/heads/ included: | |
| Two examples of GitHub raw references, from a branch, which reference the same file: | |
| ```xml | |
| <File> | |
| <Name>https://github.com/silnrsi/fonts/raw/b88c7af5d16681bd137156929ff8baec82526560/fonts/sil/alkalami/Alkalami-Regular.ttf</Name> | |
| <Source>https://github.com/silnrsi/fonts/raw/main/fonts/sil/alkalami/Alkalami-Regular.ttf</Source> | |
| </File> |
There was a problem hiding this comment.
Yeah, I will simplify it but will include example because refs/heads/ is often found in UX in GitHub so helps users to know they can use either.
| </File> | ||
| ``` | ||
|
|
||
| A GitHub blob reference, as copied from a URL in GitHub: |
There was a problem hiding this comment.
this could even be merged in with the above too - three examples…
unless we are really wanting to tutor users on how to construct these?
There was a problem hiding this comment.
I'm trying to be comprehensive on the different supported formats both for our reference as well as users. There's enough ambiguity in the URL formats that I think having all the examples helps. We'll probably only have the basic example for keyboard authors in the upcoming documentation associated with the package editor, but this is the reference doc.
| We will add a `Source` element to .kps `Package.Files.File`. We would support | ||
| several sources: | ||
| * local - local file system | ||
| * flo - fonts.languagetechnology.org | ||
| * github | ||
| * noto - in the future |
There was a problem hiding this comment.
The
<Source>element is always optional; if it is not present, then kmc-package will not check for updated versions of the source file.
The first half of the sentence doesn't make sense to me after looking at the examples. If I want to specify a font from flo it seems I have to use the <Source> element, so it's not optional in that case.
There may a separate
<Source>element as well,
I think there's a "be" missing...
| This is the same as previous versions. `<Name>` references a file on the local | ||
| filesystem. Note that `<Source>` is optional for other sources as well; the | ||
| absence of the element does not mean local -- that is determined by pattern | ||
| match. |
There was a problem hiding this comment.
Note that
<Source>is optional for other sources as well; the
See above. Maybe better: "Note that <Source> is optional for some of the other sources as well; the" ?
|
|
||
| ```xml | ||
| <File> | ||
| <Name>[relative-or-absolute-path/]{filename}</Name> |
There was a problem hiding this comment.
I think it's mentioned elsewhere, but would it be good to repeat that absolute paths are not recommended?
| ### fonts.languagetechnology.org `flo:<id>` | ||
|
|
||
| References a font by ID from fonts.languagetechnology.org. The resource must be | ||
| resolved to a permanent URL. If the `<Name>` field is omitted, the compiler will |
There was a problem hiding this comment.
"Omitted": To me this sounds like the <Name> field is optional which is confusing since above it talks about <Source> being optional. Would it be better to phrase as:
| resolved to a permanent URL. If the `<Name>` field is omitted, the compiler will | |
| resolved to a permanent URL. If the `<Name>` field is missing, the compiler will |
?
| ### GitHub | ||
|
|
||
| A reference to a resource located on GitHub. The resource must be resolved to a | ||
| permanent URL. If the `<Name>` field is omitted, the compiler will return an |
| return `https://github.com/${matches.groups.name}/${matches.groups.repo}/raw/${commit.sha}/${matches.groups.path}`; | ||
| } | ||
|
|
||
| async function getFileDataFromGitHub(callbacks: CompilerCallbacks, inputFilename: string, matches: RegExpExecArray): Promise<KmpCompilerFileDataResult> { |
There was a problem hiding this comment.
To keep the order the same as with the previous functions where we define the outer function first and then the function that gets called by the outer function, you could move this function after getFileDataFromRemote. Would make it easier to read when reading the whole file.
| @@ -468,49 +478,25 @@ export class KmpCompiler implements KeymanCompiler { | |||
|
|
|||
| const hasKmpInf = !!data.files.find(file => file.name == KMP_INF_FILENAME); | |||
|
|
|||
| let failed = false; | |||
| data.files.forEach((value) => { | |||
| for(const value of data.files) { | |||
There was a problem hiding this comment.
Out of curiosity: why did you change data.files.forEach to a for loop?
There was a problem hiding this comment.
It was so I could break out of the loop with return null on line 490 and not have the loop continue.
| static HINT_RemoteReferencesShouldBeVersion18Plus = SevHint | 0x0031; | ||
| static Hint_RemoteReferencesShouldBeVersion18Plus = (o:{filename: string, kpsVersion: string}) => m( | ||
| this.HINT_RemoteReferencesShouldBeVersion18Plus, | ||
| `The source package includes a reference to URL '${def(o.filename)}', and the package source version is '${def(o.kpsVersion)}'; the package source version should be at least '18.0'`, |
There was a problem hiding this comment.
Maybe?
| `The source package includes a reference to URL '${def(o.filename)}', and the package source version is '${def(o.kpsVersion)}'; the package source version should be at least '18.0'`, | |
| `The source package includes a reference to URL '${def(o.filename)}' but the package source version is '${def(o.kpsVersion)}'; the package source version should be at least '18.0'`, |
| @@ -4,9 +4,13 @@ import { KeyboardMetadataCollection } from './package-metadata-collector.js'; | |||
| import { KpsFile, CompilerCallbacks } from '@keymanapp/developer-utils'; | |||
|
|
|||
| export const DEFAULT_KEYBOARD_VERSION = '1.0'; | |||
| export const MIN_LM_FILEVERSION_KMP_JSON = '12.0'; | |||
|
|
|||
| /** minimum FileVersion for keyboard packages */ | |||
| assert.equal(matches.groups.repo, 'fonts'); | ||
| assert.equal(matches.groups.hash, 'b88c7af5d16681bd137156929ff8baec82526560'); | ||
| assert.equal(matches.groups.path, 'fonts/sil/alkalami/Alkalami-Regular.ttf'); | ||
| const res = await getFileDataEndpoints.getFileDataFromGitHub(callbacks, '', matches); |
There was a problem hiding this comment.
Does this get the file from GitHub or is this mocked somehow?
There was a problem hiding this comment.
This is mocked in the TestCompilerCallbacks class:
keyman/developer/src/common/web/test-helpers/index.ts
Lines 9 to 18 in b344ca7
It fetches files from test/fixtures/online with munged up paths as can be seen in this PR, e.g. /developer/src/kmc-package/test/fixtures/online/fonts.languagetechnology.org/#fonts#sil#andika#Andika-Regular.ttf. There are some limits to this, e.g. it doesn't support # or ? components of a URL, so it would need more work to do those. It's a bit quick-and-dirty but fits the bill for now.
TestCompilerCallbacks has a mode where it runs online and retrieves and saves fixtures, which we can enable with the TEST_SAVE_FIXTURES env var. See the source for details; this mode makes it easy to build a coherent test that will work with online-sourced data and be reproducible.
|
I think I have addressed all the comments in the reviews, thanks all! |
…ce-files-from-remotes-2
mcdurdin
deleted the
feat/developer/kmc-package-source-files-from-remotes-2
branch
Note: epic/shared-fonts is brought to you by the Share Bear 🐻
The concept here is that the 'Name' property for a file can now be a remote stable reference to a file stored on GitHub, rather than a local file. Additionally, we add a 'Source' property that may change over time, so we can be notified of changes to files.
File.Source
The
Nameelement may be either a local filename, or a GitHub raw permanentURL, conforming to the pattern:
We will add a
Sourceelement to .kpsPackage.Files.File. We would supportseveral sources:
local:
<Source>element is omittedThis is the same as previous versions.
<Name>references a file on the localfilesystem. Note that
<Source>is optional for other sources as well; theabsence of the element does not mean local -- that is determined by pattern
match.
Example
fonts.languagetechnology.org
flo:<id>References a font by ID from fonts.languagetechnology.org. The resource must be
resolved to a permanent URL. If the
<Name>field is omitted, the compiler willreturn an error, reporting the URL it discovers, so the author should be able to
paste that URL in.
If the URL in the
<Name>field differs from the resolved URL given by FLO,then a hint will be issued by the compiler, allowing the author to easily update
to a new version of the resource at their convenience. It is anticipated that
this may be scripted on the keyboards repository in the future.
Example
GitHub
A reference to a resource located on GitHub. The resource must be resolved to a
permanent URL. If the
<Name>field is omitted, the compiler will return anerror, reporting the URL it discovers, so the author should be able to paste
that URL in.
If the URL in the
<Name>field differs from the resolved URL from the<Source>element, then a hint will be issued by the compiler, allowing theauthor to easily update to a new version of the resource at their convenience.
It is anticipated that this may be scripted on the keyboards repository in the
future.
Examples
A GitHub raw reference, from a commit, resolves identically.
A GitHub raw reference, from a branch, with refs/heads/ excluded:
A GitHub raw reference, from a branch, with refs/heads/ included:
A GitHub blob reference, as copied from a URL in GitHub:
noto
In a future version.
Additional file format notes
At the same time
CopyLocation,Description, andFileTypeelements shouldbe deprecated and totally ignored by the compiler. They are busydata.
Future sources could be considered, e.g. noto. We don't want to allow arbitrary URLs, both for stability and for security reasons.
This change is entirely compiler-side, so we don't need to make any changes to apps, and so packages will be backwardly compatible.
Things to do
--> moved to #11236
Replaces: #12336
Fixes: #11236
@keymanapp-test-bot skip