-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
chore(linux): Fix build scripts #9781
Conversation
User Test ResultsTest specification and instructions User tests are not required |
Creating the coverage option to ninja previously didn't work reliably. This change re-orders the parameters passed to `meson setup` and seems to work better. Additionally this change refactors the scripts and makes use of `builder_run_action`.
a8c2598
to
08a259c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The [ -d "$MESON_PATH" ] && cd "$MESON_PATH"
line is a little sneaky and perhaps will cause confusion if ninja then attempts to run in the wrong folder because configure
hasn't been run, but I understand what you are doing and it's concise.
LGTM
Since keyman-system-service is a dependency of ibus-keyman it gets build when ibus-keyman gets build. However, in that case the `--coverage` flag doesn't get passed through in which case the `coverage-html` target is missing from ninja. This change fixes this problem by checking if ninja has the `coverage-html` target. If not we remove the output directory which causes the configure action to run again with the `--coverage` option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized coverage report creation still wasn't reliably working. keyman-system-service
gets build as a dependency of ibus-keyman
in which case it doesn't get passed the --coverage
option.
if builder_has_option --coverage && [ -d "$MESON_PATH" ]; then | ||
# It's possible that we got configured because we're a dependency of ibus-keyman | ||
# in which case the `--coverage` option wasn't passed along. | ||
cd "$MESON_PATH" | ||
if ! ninja -t targets | grep -q coverage-html ; then | ||
cd "$THIS_SCRIPT_PATH" | ||
clean_action | ||
fi | ||
cd "$THIS_SCRIPT_PATH" | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an easier solution? I don't think our build.sh
system allows to pass the --coverage
option to dependencies, or does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keyman-system-service should be a child build rather than a dependency. A dependency is explicitly something that is required for the project but that you only want to build and use. A child build is a component owned by the parent build.
keyman/resources/build/build-utils.md
Lines 447 to 451 in 1e734a4
If a `+` is appended (after the optional shorthand form, but before the | |
default), then the option will be passed to child scripts. All child scripts | |
_must_ accept this option, or they will fail. It is acceptable for the child | |
script to declare the option but ignore it. However, the option will _not_ be | |
passed to dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's both. You can't run ibus-keyman tests without first building keyman-system-service.
Or do you mean keyman-system-service should be a child of ibus-keyman? But that doesn't make completely sense either because it's not really a component of ibus-keyman.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure what you are trying to solve here. It seems like the top-level linux/build.sh is already building all the modules as child builds, and the --coverage
option already has +
so it should be passed through.
If things are building in the wrong order, you could try putting the keyman-system-service first in linux/build.sh.
Changes in this pull request will be available for download in Keyman version 17.0.194-alpha |
Creating the coverage option to ninja previously didn't work reliably. This change re-orders the parameters passed to
meson setup
and seems to work better. Additionally this change refactors the scripts and makes use ofbuilder_run_action
.@keymanapp-test-bot skip