chore(common): builder script `:target` and `--option` support by mcdurdin · Pull Request #6986 · keymanapp/keyman

mcdurdin · 2022-07-25T04:46:21Z

Note: I suggest reviewing only the first commit, as the second one is just changing over #!/bin/bash to #!/usr/bin/env bash.

Adds support for :target and --option to the builder_() functions in build-utils.sh, along with a consistent way of describing command line parameters in short-hand form, so we can avoid the display_usage getting out of sync with actual command line parameters.

I opted to tweak the existing usage slightly, reversing parameter order for builder_report, so that we could extend builder_report cleanly to support targets.

The new builder_() functions are builder_describe and builder_parse, which should be used in place for builder_init where you want to provide more detailed command-line options (for many scripts, the defaults provided through builder_init may suffice).

builder_describe lets you set a single-line description of the script, along with a list of parameters: actions, :targets and --options. Options may also specify a shorthand form, e.g. --option,-o. Each parameter may optionally include a short description, separated by space(s) from the parameter name.

build-utils.test.sh tests the functionality of the various functions, and provides some examples of using them.

Defaults:

If a target is not specified in builder_describe, a standard target of :project will be available.
If the user does not pass an action on the command line, then build will be used.
If the user does not pass a target on the command line, all targets will have the actions applied.

Note, it is possible to have targets which do not use all actions. These may be still be specified, but can be ignored. For example, test:tools may not be an action:target that you handle in the script. At this time, the builder_() functions do not attempt to check for this.

Example

Demonstrates some of the functionality available:

builder_describe \
  "Builds my little project." \
  "clean        Cleans up any build artifacts for target(s)" \
  "build        Build the target(s)" \
  "test         Run tests on target(s)" \
  ":app         My app" \
  ":tools       Tools for development of my app" \
  "--power,-p   Use powerful mode" \
  "--zoom,-z    Use zoom mode"

builder_parse "$@"

USE_POWER=false
builder_has_option --power && USE_POWER=true

if builder_has_action clean :app; then
  npm run clean
  builder_report success clean
fi

if builder_has_action build :app; then
  npm run build
  builder_report success build
fi

if builder_has_action build :tools; then
  ./tools/build.sh build:tools
  builder_report success build
fi

Sample command lines:

./build.sh -p build:app
./build.sh --zoom clean build:tools

😁

@keymanapp-test-bot skip

Adds support for :target and --option to the builder_() functions in build-utils.sh, along with a consistent way of describing command line parameters in short-hand form, so we can avoid the display_usage getting out of sync with actual command line parameters. I opted to tweak the existing usage slightly, reversing parameter order for `builder_report`, so that we could extend `builder_report` cleanly to support targets. The new builder_() functions are `builder_describe` and `builder_parse`, which should be used in place for `builder_init` where you want to provide more detailed command-line options (for many scripts, the defaults provided through `builder_init` may suffice). `builder_describe` lets you set a single-line description of the script, along with a list of parameters: `actions`, `:targets` and `--options`. Options may also specify a shorthand form, e.g. `--option,-o`. Each parameter may optionally include a short description, separated by space(s) from the parameter name. build-utils.test.sh tests the functionality of the various functions, and provides some examples of using them. Defaults: * If a target is not specified in `builder_describe`, a standard target of `:project` will be available. * If the user does not pass an action on the command line, then `build` will be used. * If the user does not pass a target on the command line, all targets will have the actions applied. Note, it is possible to have targets which do not use all actions. These may be still be specified, but can be ignored. For example, `test:tools` may not be an action:target that you handle in the script. At this time, the builder_() functions do not attempt to check for this.

keymanapp-test-bot · 2022-07-25T04:46:24Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Android
iOS
- Keyman for iOS (simulator image)
- FirstVoices Keyboards for iOS (simulator image)
- TestFlight internal PR build version - 16.0.41 (0.6986.6691)
Linux
- Keyman for Linux
macOS
- Keyman for macOS
Web
- KeymanWeb Test Home
Windows
- Keyman for Windows
- FirstVoices Keyboards for Windows

Fixes up scripts (except under /linux) to use `#!/usr/bin/env bash` instead of `#!/bin/bash` or `#!/bin/sh` so that we don't end up with the ancient version of bash supplied with macOS. This became urgent with this PR, because of bash-4.xisms in build-utils.sh, for example on line 572: ``` if [[ -v _builder_params[$e] ]]; then ```

jahorton · 2022-07-25T05:05:07Z

common/web/lm-worker/build.sh

 # TODO: we should try and rework this to avoid the need to manually wrap

-if builder_has_action clean || builder_has_action build; then
+if builder_has_action clean || builder_has_action build >/dev/null; then


Adding a >/dev/null here intentionally? I'm not clear on why it's needed here but not in many other places.

Yeah, because the builder_has_action build was triggering a build status message here that was confusing -- because we have to do the clean even for build, see comments in previous lines... I'm not that happy with this, and a --quiet option may be better, but this will do for now I think.

jahorton

Example

Demonstrates some of the functionality available:

builder_describe \
  "Builds my little project." \
  "clean        Cleans up any build artifacts for target(s)" \
  "build        Build the target(s)" \
  "test         Run tests on target(s)" \
  ":app         My app" \
  ":tools       Tools for development of my app" \
  "--power,-p   Use powerful mode" \
  "--zoom,-z    Use zoom mode"

builder_parse "$@"

USE_POWER=false
builder_has_option --power && USE_POWER=true

if builder_has_action clean :app; then
  npm run clean
  builder_report success clean
fi

if builder_has_action build :app; then
  npm run build
  builder_report success build
fi

if builder_has_action build :tools; then
  ./tools/build.sh build:tools
  builder_report success build
fi

One major question about all of this that's actually addressable through this example:

Suppose I use this script with a simple build.sh - no parameters whatsoever. Are any of the explicitly-defined actions here run in that case? None of them are set against :project.

resources/build/build-utils.sh

mcdurdin · 2022-07-26T22:16:22Z

Suppose I use this script with a simple build.sh - no parameters whatsoever. Are any of the explicitly-defined actions here run in that case? None of them are set against :project.

Good question.

The target :project is only defined if there are no other targets defined in builder_describe.
If you do not specify a target in your command line call, then all targets will be included.

The default action is currently build (ref line 535 of build-utils.sh). (Perhaps the default action should be --help, or all actions?). Now that I look at this, this is probably not right -- it applies build to all targets, not just the ones (if any) specified on the command line...

keyman/resources/build/build-utils.sh

Lines 532 to 537 in ab54c0e

    
           # TODO: not sure if this is appropriate or if we should error? 
        
           if (( ! ${#_builder_chosen_action_targets[@]} )); then 
        
             for e in "${_builder_targets[@]}"; do 
        
               _builder_chosen_action_targets+=("build$e") 
        
             done 
        
           fi

builder_init was unnecessarily and inconsistently duplicating builder_describe and builder_parse functionality. Removing it -- all scripts should use builder_describe and builder_parse instead, which gives us more flexibility and consistency into the future.

jahorton · 2022-07-27T05:37:35Z

The default action is currently build (ref line 535 of build-utils.sh). (Perhaps the default action should be --help, or all actions?).

Why not...

_builder_default_action="build"

builder_default_action() {
  _builder_default_action="$1"
}

# ...
  local option_default

# [line 512ish:  block replacement]
  # TODO: not sure if this is appropriate or if we should error? 
  if (( ! ${#_builder_chosen_action_targets[@]} )); then 
    _builder_item_in_array "$_builder_default_action" "${_builder_actions[@]}" || fail "Default action not available"

    # To facilitate `builder_default_action --help` in a dependent script.
    option_default=_builder_item_in_array "$_builder_default_action" "${_builder_options[@]}" && 1 || 0

    if (( option_default )); then
      _builder_chosen_options+=("$_builder_default_action")
    else
      #_builder_chosen_targets is to-be-defined, in line with other concerns of the quoted comment.
      for e in "${_builder_chosen_targets[@]}"; do 
        _builder_chosen_action_targets+=("$_builder_default_action$e") 
      done 
    fi
  fi

Or you could go crazier if you wanted and make _builder_default_action an array, allow setting multiple actions, and looping over them all in that block. That's... probably overkill though.

jahorton · 2022-07-27T05:40:32Z

Re this part:

Now that I look at this, this is probably not right -- it applies build to all targets, not just the ones (if any) specified on the command line...

# ... [line 458 or so]

  local _builder_chosen_targets=()

# ... [line 487]

    elif (( has_target )); then
      for e in "${_builder_actions[@]}"; do
        _builder_chosen_action_targets+=("$e:$target")
        _builder_chosen_targets+=($target)  # the only altered (well, added) line in this block
      done

#...

Would those changes work, in combination with my prior suggestion?

Default action is 'build' unless overridden in builder_describe. Default target is all targets when no target is specified in command- line.

mcdurdin · 2022-07-28T04:25:28Z

I've gone a slightly different route for defaults:

Default action is 'build' unless overridden in builder_describe, by using a '+' suffix on the default action, e.g. "test+ Test this project".
Default target is all targets when no target is specified in command-line.

This seems to me to be the defaults that will be most useful:

build.sh clean will clean all targets, as generally we want to apply to all targets.
build.sh :app will be just build.sh build:app as I think generally running all actions is not what we want.

We could go more complex but I think we get marginal additional value. We want this to be easy to author but also easy to remember how to use -- so build.sh configure, build.sh build and build.sh test are likely to be our most common invocations.

resources/build/build-utils.sh

jahorton · 2022-07-28T04:36:53Z

resources/build/build-utils.sh

-        _builder_chosen_action_targets+=("$e:$target")
-      done
+      # apply the default action to the selected target
+      _builder_chosen_action_targets+=("$_builder_default_action$target")


How would this handle a case like this?

Script use:

> ./build.sh clean :engine

Script definition:

builder_describe \ "Tests the build-utils.sh builder functions. This is merely an example." \ "clean Cleans up any build artifacts" \ "build+ Do some building" \ "test Does some test stuff" \ ":app" \ ":engine Thomas, y'know" \ "--power,-p Use powerful mode" \ "--zoom,-z Use zoom mode"

In my mind, such a call says "only run the clean action, and only for the :engine target."

Alternatively, I intuitively believe I could use:

./build.sh clean build :engine

to both clean and build the :engine target, and that target alone.

I bring this up because, by my reading of this part of the code and what precedes it, what we actually get for the first example use is:

# From clean - clean:app clean:engine # From the loose :engine - build:engine

which is completely different. Assuming I'm interpreting the code correctly.

So looks like our internal intuition engines are colliding here.

You are right, currently, this:

> ./build.sh clean :engine

does generate this, yes:

# From clean - clean:app clean:engine # From the loose :engine - build:engine

I am open to swapping the inference model to what you've suggested. Pros/cons both ways. Current model leaves each parameter interpreted in isolation which is easy to figure out the action:target pairs. Your model is probably a bit more intuitive but perhaps sticks just a little for more complex invocations, e.g. something like clean:app build:engine test -- which targets would apply to test here; or am I just being difficult? 🤣)

e.g. something like clean:app build:engine test -- which targets would apply to test here; or am I just being difficult? 🤣)

:project, as there's no free-standing target specified otherwise. Admittedly, that example is a bit of a fun parse either way you look at it.

Something like clean:app effectively scopes those commands together, while free-standing actions and free-standing targets have "general" scope. If that makes sense?

I guess that leads to another niche call structure:

> ./build.sh clean:engine :app

would yield

clean:engine build:app # due to freestanding :app target with no specified, general action

Whereas leaving off the :app drops the build:app part; there'd be no need for a default action because there's no unpaired (or, un-actioned?) target. When no target is specified in any fashion, well, :project exists; if no action is specified either, :project is thus unpaired, so the default action gets paired to it.

Though, I admit, this last example set isn't exactly the clearest scenario.

So I feel like either model can lead to confusion, and your suggestion entangles the parameters, whereas in my current model, each parameter can be deduced in isolation, which I think makes it easier to figure out.

So given this, I think I will stick with the existing model:

./build.sh clean :engine is gonna do clean:* and build:engine.

./build.sh clean:engine is gonna do clean:engine.

./build.sh clean build :engine will give us clean:* build:* (the final :engine parameter is effectively irrelevant).

./build.sh will give us build:* (default action is build, default target is *)

As long as this is explicitly documented in ./build_utils.sh.

Is this sufficient? Noting that this is made available with --help in any build script, which is more visible than as a comment in build-utils.sh. If this doesn't provide what you are looking for, can you make a suggestion?

keyman/resources/build/build-utils.sh

Lines 634 to 636 in a00c9a9

echo "* If action is not specified, default action is '$_builder_default_action'"

echo "* If :target is not specified, will apply action to all targets"

echo "* If no actions or :targets are specified, '$_builder_default_action' action will run on all targets"

I do not believe that it is. Remember, that was in place when I asked the questions that led to this discussion. If I made the same mistake, and was this confused about it, I'd be shocked if no one else would have the same interpretation. I'll think on a way to address the concern.

First draft:

echo "* Specify `action:target` to run a specific `action` against a specific `:target`." echo "* If `action` is specified without a `target` suffix, it will be applied to all `:target`s." echo "* If `:target` is specified without an `action` prefix, `$_builder_default_action:target` will be inferred." echo "* If no `action`, `:target`, or `action:target` entries are specified, `$_builder_default_action` will run on all `:target`s."

…ion-support

keyman-server · 2022-07-29T18:01:45Z

Changes in this pull request will be available for download in Keyman version 16.0.41-alpha

mcdurdin requested review from ermshiperete and jahorton as code owners July 25, 2022 04:46

mcdurdin added this to the A16S7 milestone Jul 25, 2022

mcdurdin changed the title ~~chore(common): builder script :target and --option support~~ chore(common): builder script :target and --option support Jul 25, 2022

github-actions bot added common/ common/resources/ Build infrastructure common/web/ web/ chore labels Jul 25, 2022

jahorton mentioned this pull request Jul 25, 2022

feat(web): Gesture Recognizer input sequence recorder 🐵 #6952

Merged

mcdurdin requested review from SabineSIL, darcywong00, rc-swag and sgschantz as code owners July 25, 2022 05:01

github-actions bot added android/ android/app/ android/engine/ android/samples/ developer/ developer/compilers/ ios/ mac/ mac/engine/ labels Jul 25, 2022

mcdurdin force-pushed the chore/common/builder-script-target-and-option-support branch from f5ba9b4 to 767927b Compare July 25, 2022 05:15

jahorton reviewed Jul 25, 2022

View reviewed changes

chore(common): more bin bashes

ab54c0e

mcdurdin mentioned this pull request Jul 25, 2022

feat(web): Gesture Recognizer unit testing 🐵 #6970

Merged

jahorton reviewed Jul 26, 2022

View reviewed changes

resources/build/build-utils.sh Show resolved Hide resolved

chore(common): tweak defaults for builder

3fa1701

Default action is 'build' unless overridden in builder_describe. Default target is all targets when no target is specified in command- line.

jahorton reviewed Jul 28, 2022

View reviewed changes

mcdurdin added 3 commits July 29, 2022 05:45

chore(common): disable debug flag

a00c9a9

chore(common): tweak build help and color support

ba6bc67

Merge branch 'master' into chore/common/builder-script-target-and-opt…

c1118d9

…ion-support

jahorton approved these changes Jul 29, 2022

View reviewed changes

mcdurdin merged commit 6304d52 into master Jul 29, 2022

mcdurdin deleted the chore/common/builder-script-target-and-option-support branch July 29, 2022 01:54

jahorton mentioned this pull request Jul 29, 2022

feat(web): the gesture-recognition engine 🐵 #6842

Merged

1 task

	echo "* If action is not specified, default action is '$_builder_default_action'"
	echo "* If :target is not specified, will apply action to all targets"
	echo "* If no actions or :targets are specified, '$_builder_default_action' action will run on all targets"

Uh oh!

Conversation

mcdurdin commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example

Uh oh!

keymanapp-test-bot bot commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Test Results

Test Artifacts

Uh oh!

jahorton Jul 25, 2022

Choose a reason for hiding this comment

Uh oh!

mcdurdin Jul 25, 2022

Choose a reason for hiding this comment

Uh oh!

jahorton left a comment

Choose a reason for hiding this comment

Example

Uh oh!

Uh oh!

mcdurdin commented Jul 26, 2022

Uh oh!

jahorton commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jahorton commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcdurdin commented Jul 28, 2022

Uh oh!

Uh oh!

Uh oh!

jahorton Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

mcdurdin Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

jahorton Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcdurdin Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

jahorton Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

mcdurdin Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

jahorton Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jahorton Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyman-server commented Jul 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mcdurdin commented Jul 25, 2022 •

edited

Loading

keymanapp-test-bot bot commented Jul 25, 2022 •

edited

Loading

jahorton commented Jul 27, 2022 •

edited

Loading

jahorton commented Jul 27, 2022 •

edited

Loading

jahorton Jul 28, 2022 •

edited

Loading

jahorton Jul 28, 2022 •

edited

Loading

jahorton Jul 28, 2022 •

edited

Loading