Skip to content

chore(core): cleanup meson build#8279

Merged
mcdurdin merged 11 commits intofeature-ldmlfrom
chore/core/cleanup-meson-build
Feb 23, 2023
Merged

chore(core): cleanup meson build#8279
mcdurdin merged 11 commits intofeature-ldmlfrom
chore/core/cleanup-meson-build

Conversation

@mcdurdin
Copy link
Member

@mcdurdin mcdurdin commented Feb 21, 2023

Fixes #8272.

This updates our meson min version on Windows to 1.0, which means that we can avoid having Visual Studio on the path, as meson can find it anyway. With this change, we can eliminate the batch-wrapper for the build, significantly improving build performance and simplifying the scripts.

This also adds a change to disable tests for dependency builds of core, along with the --no-tests command line option for doing builds without tests.

Minor cleanup includes eliminating various warnings from meson.build files, and standardizing WASM cross-platform build options for Windows.

I have also specified --no-tests in all dependent builds.

@keymanapp-test-bot skip

This updates our meson min version on Windows to 1.0, which means that
we can avoid having Visual Studio on the path, as meson can find it
anyway. With this change, we can eliminate the batch-wrapper for the
build, significantly improving build performance and simplifying the
scripts.

This also adds a change to disable tests for dependency builds of core,
along with the `--no-tests` command line option for doing builds without
tests.

Minor cleanup includes eliminating various warnings from meson.build
files, and standardizing WASM cross-platform build options for Windows.
@mcdurdin mcdurdin added this to the A17S7 milestone Feb 21, 2023
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Feb 21, 2023

User Test Results

Test specification and instructions

User tests are not required

@mcdurdin
Copy link
Member Author

Turns out meson doesn't support building x86 targets with this model. Seriously:

https://github.com/mesonbuild/meson/blob/088727164de8496c4bada040c2f4690e42f66b69/mesonbuild/utils/vsenv.py#L76-L84

So we'll have to roll back to using our disgusting .bat wrapper around the meson build, but at least we can ensure that we remove other Visual Studio versions from the path for a more stable build.

@mcdurdin
Copy link
Member Author

Turns out meson doesn't support building x86 targets with this model.

I've opened mesonbuild/meson#11435 on the meson project to hopefully get this sorted -- not entirely sure of the appropriate way to do it there so will see if the meson maintainers respond first.

Comment on lines +77 to +79
# meson forces us to configure tests, including building compilers, even
# if we don't plan to run them, for example when doing a dependency build
# in CI

Choose a reason for hiding this comment

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

including building compilers

If installing a compiler for this is undesirable in CI, maybe you'd prefer to conditionally add the language with have_somelang = add_languages('somelang', required: tests_mandatory) and then only configure the tests if the language is available.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review!

Sadly, we kinda need to jump through some hoops here anyway: we don't want to build the test dependencies when building, for example, Keyman for Windows, for performance reasons; these dependencies are TypeScript and not built with meson.

I guess the comment in build.sh is a bit misleading in that regard.

The approach we took in the end was to declare a "no tests" option, because it was more explicit, as opposed to previously where we tested for the presence of nodejs, and implicitly disabled certain tests. This means that the default behaviour is to build tests, and fail if something is missing, which is better than silently passing.

@mcdurdin mcdurdin linked an issue Feb 22, 2023 that may be closed by this pull request
"references": [
{ "path": "../../../../common/web/keyman-version/tsconfig.esm.json" },
{ "path": "../../../../common/web/types/" },
{ "path": "../../../../common/tools/hextobin/" },
Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed to fix a build failure that arises due to order of compilation, otherwise.

Copy link
Contributor

@jahorton jahorton left a comment

Choose a reason for hiding this comment

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

I'm not too familiar with the Core build, but I think this is LGTM-worthy?

Copy link
Contributor

@rc-swag rc-swag 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 38603e8 into feature-ldml Feb 23, 2023
@mcdurdin mcdurdin deleted the chore/core/cleanup-meson-build branch February 23, 2023 02:28
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.

chore(core): avoid building kmc unless running tests

4 participants