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

feat: use new build script and kmc #226

Merged

Conversation

mcdurdin
Copy link
Member

@mcdurdin mcdurdin commented Oct 9, 2023

Fixes #224.

Replaces the old build script with a cleaner version that utilizes builder.inc.sh, standardizing our build commands.

Uses kmc to build all source models, in a single command rather than one-by-one as previously. Handles external models more simply.

TODO:


Please note: this will merge into staging-17.0, in preparation for 17.0 release. Changes on the staging-17.0 branch will not be published to keyman.com, until they are merged into master when we go to release (or when we determine the updated build tools are sufficiently stable).

Replaces the old build script with a cleaner version that utilizes
builder.inc.sh, standardizing our build commands.

Uses `kmc` to build all source models, in a single command rather than
one-by-one as previously. Handles external models more simply.

Awaiting merge of changes to kmc to keymanapp/keyman/master before
committing updated package.json.
@mcdurdin mcdurdin added this to the A17S23 milestone Oct 9, 2023
@mcdurdin mcdurdin added the feat label Oct 9, 2023
@mcdurdin mcdurdin marked this pull request as ready for review October 17, 2023 23:12
@mcdurdin mcdurdin modified the milestones: A17S24, A17S25 Oct 27, 2023
@mcdurdin mcdurdin modified the milestones: A17S25, A17S26 Nov 13, 2023
build.sh Show resolved Hide resolved
#
# Build a list of all targets that we will be building, in a specific order:
# * release
# * experimental
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this also add to the list of external targets in build_external_targets.txt?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so, yes.


MODELINFO_SCHEMA_JSON="$MODELROOT/tools/model_info.source.json"
MODELINFO_SCHEMA_DIST_JSON="$MODELROOT/tools/model_info.distribution.json"
. "./resources/util.inc.sh"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still needed?
I thought die can be replaced with builder_die (external.inc.sh). And I didn't see parse_args still being called.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will review

Copy link
Member Author

Choose a reason for hiding this comment

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

ci.sh is still using a lot of the functions in util.inc.sh, so that will need separate cleanup.

@mcdurdin mcdurdin modified the milestones: A17S26, A17S27 Nov 27, 2023
@darcywong00
Copy link
Contributor

Should ./build.sh of the repo work on Linux now? I got these warnings/errors

gonzalez_quint_coto.cjp-latn.cabecar.model.kps - warn KM0400F: The file ..\build\gonzalez_quint_coto.cjp-latn.cabecar.model.js does not follow the recommended filename conventions. The extension should be all lower case, and the filename should include only alphanumeric characters, -, _, + and .
gonzalez_quint_coto.cjp-latn.cabecar.model.kps - warn KM0400F: The file ..\LICENSE.md does not follow the recommended filename conventions. The extension should be all lower case, and the filename should include only alphanumeric characters, -, _, + and .
gonzalez_quint_coto.cjp-latn.cabecar.model.kps - info KM05007: release/gonzalez_quint_coto/gonzalez_quint_coto.cjp-latn.cabecar/source/gonzalez_quint_coto.cjp-latn.cabecar.model.kps failed to build.
gonzalez_quint_coto.cjp-latn.cabecar.kpj - info KM05010: The build failed because option "treat warnings as errors" is enabled and there are one or more warnings.
gonzalez_quint_coto.cjp-latn.cabecar.kpj - info KM0500C: Project release/gonzalez_quint_coto/gonzalez_quint_coto.cjp-latn.cabecar/gonzalez_quint_coto.cjp-latn.cabecar.kpj failed to build.

Is it not liking the path separator \?

@mcdurdin
Copy link
Member Author

mcdurdin commented Nov 29, 2023

Should ./build.sh of the repo work on Linux now? I got these warnings/errors

These look like bugs in kmc-package which we recently fixed in keymanapp/keyman#10064. So we should re-test with version 17.0.219-alpha on Linux before merging this.

@mcdurdin
Copy link
Member Author

mcdurdin commented Dec 4, 2023

Tested with 17.0.222-alpha and this has now been resolved, looking for a review thx

package-lock.json Outdated Show resolved Hide resolved
Copy link
Contributor

@darcywong00 darcywong00 left a comment

Choose a reason for hiding this comment

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

lgtm

@mcdurdin mcdurdin merged commit 3f4e39a into chore/add-license-to-packages Dec 4, 2023
2 checks passed
@mcdurdin mcdurdin deleted the feat/use-new-build-script-with-kmc branch December 4, 2023 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

feat: build with kmc instead of kmlmc
2 participants