Skip to content

feat(developer): kmc build command 🙀#8300

Merged
mcdurdin merged 6 commits intofeature-ldmlfrom
feat/developer/5283-kmc-command-line-refactor
Feb 27, 2023
Merged

feat(developer): kmc build command 🙀#8300
mcdurdin merged 6 commits intofeature-ldmlfrom
feat/developer/5283-kmc-command-line-refactor

Conversation

@mcdurdin
Copy link
Copy Markdown
Member

@keymanapp-test-bot skip

Starts the refactor of kmc to support the build command. In this PR, only supports building LDML keyboards. But infrastructure is in place to add different types of builds.

I have moved -T option to build-test-data command for now. Not sure exactly how this fits, and even if it should be in kmc at all. But it'll do as a start.

This file layout is intended to help keep options and commands under control and modular:

  • commands/ contains all the command-line processing for each command
  • activities/ contains all the logic and file processing, wrapping file input/output from the corresponding kmc-* module.

It's going to grow substantially!

@mcdurdin mcdurdin added this to the A17S7 milestone Feb 23, 2023
@keymanapp-test-bot
Copy link
Copy Markdown

keymanapp-test-bot bot commented Feb 23, 2023

Comment on lines +47 to +50
mkdir -p "$THIS_SCRIPT_PATH/build/src/util/"
cp "$KEYMAN_ROOT/resources/standards-data/ldml-keyboards/techpreview/ldml-keyboard.schema.json" "$THIS_SCRIPT_PATH/build/src/util/"
cp "$KEYMAN_ROOT/resources/standards-data/ldml-keyboards/techpreview/ldml-keyboardtest.schema.json" "$THIS_SCRIPT_PATH/build/src/util/"
cp "$KEYMAN_ROOT/common/schemas/kvks/kvks.schema.json" "$THIS_SCRIPT_PATH/build/src/util/"
Copy link
Copy Markdown
Contributor

@jahorton jahorton Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So... it seems like the files that the NodeCompilerCallbacks.ts module references need to be stored locally to the module itself?

I can understand needing a copy somewhere in the Developer build output - the files would need to get bundled with everything else for distribution, after all. Just... a small bit surprised they're not being located in build/resources or the like. Not that it's likely to be a major issue.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try to make the compiled output have the same file layout as the distribution. Fewer surprises.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps the resources should be a shared module.

@@ -3,172 +3,44 @@
* kmc - Keyman Next Generation Compiler
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So... it looks like this file is mostly just split + abstracted to prepare for more complex subcommands?

Looks like the following files were spun off from this:

  • activities/buildLdmlKeyboard.ts
  • activities/buildTestData.ts
  • commands/build.ts
  • commands/buildTestData.ts
  • util/NodeCompilerCallbacks.ts

I'm not quite fine-tooth-combing this, but it looks mostly like the same code as before, just reorganized.

Just figured I'd document that in case anyone else wants to review... and to confirm that I'm following the changes reasonably well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, pretty much. The command line has changed to have a 'command' as the first parameter, in prep for future commands.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, build and build-test-data were previously the default and a separate argument, respectively

@mcdurdin mcdurdin force-pushed the feat/developer/5283-kmc-command-line-refactor branch from 226d343 to 91c6cb9 Compare February 24, 2023 06:54
Comment on lines +47 to +50
mkdir -p "$THIS_SCRIPT_PATH/build/src/util/"
cp "$KEYMAN_ROOT/resources/standards-data/ldml-keyboards/techpreview/ldml-keyboard.schema.json" "$THIS_SCRIPT_PATH/build/src/util/"
cp "$KEYMAN_ROOT/resources/standards-data/ldml-keyboards/techpreview/ldml-keyboardtest.schema.json" "$THIS_SCRIPT_PATH/build/src/util/"
cp "$KEYMAN_ROOT/common/schemas/kvks/kvks.schema.json" "$THIS_SCRIPT_PATH/build/src/util/"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps the resources should be a shared module.

import { NodeCompilerCallbacks } from '../util/NodeCompilerCallbacks.js';

export function buildLdmlKeyboard(infile: string, options: any) {
// TODO-LDML: consider hardware vs touch -- touch-only layout will not have a .kvk
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true at what layer do we want to expose that? I guess the kvk will just be null?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure at this point; I think it will come out in the wash as we work on that.

@@ -3,172 +3,44 @@
* kmc - Keyman Next Generation Compiler
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, build and build-test-data were previously the default and a separate argument, respectively

@srl295
Copy link
Copy Markdown
Member

srl295 commented Feb 25, 2023

looks great… one usage typo

Co-authored-by: Steven R. Loomis <srl295@gmail.com>
@mcdurdin mcdurdin merged commit 8ef9be3 into feature-ldml Feb 27, 2023
@mcdurdin mcdurdin deleted the feat/developer/5283-kmc-command-line-refactor branch February 27, 2023 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants