chore(common): maintenance on build scripts - cd #11329

mcdurdin · 2024-05-01T03:48:35Z

Fixes #11324.

Always cd "$THIS_SCRIPT_PATH"
Remove unnecessary cd from all build.sh
Remove unnecessary set -eu from all build.sh (and # set -x)
Replace old build-utils.sh incantation in a few build.sh scripts
Builder scripts all now use resources/build/builder.inc.sh, and other scripts use build-utils.sh: if you don't call builder_describe and builder_parse, then you are not a builder script!

@keymanapp-test-bot skip

keymanapp-test-bot · 2024-05-01T03:48:39Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

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

mcdurdin · 2024-05-01T03:50:09Z

developer/src/server/build.sh

@@ -5,13 +5,10 @@
 THIS_SCRIPT="$(readlink -f "${BASH_SOURCE[0]}")"
 . "${THIS_SCRIPT%/*}/../../../resources/build/build-utils.sh"
 ## END STANDARD BUILD SCRIPT INCLUDE
-EX_USAGE=64


Unused legacy cruft!

mcdurdin · 2024-05-01T04:06:19Z

Okay, one problem here is that build-utils.sh is used for both builder scripts and other utility scripts, some of which are supposed to not change folder (e.g. mkver.inc.sh).

Two ways to fix:

Have a separate include that is intended to be just for builder scripts. This involves some shuffling:
- Rename build-utils.sh to build-utils.inc.sh. (this is just tidy up)
- Builder scripts should source a new resources/build/builder.inc.sh, and other scripts should source resources/build/build-utils.inc.sh.
- resources/builder.inc.sh remains unchanged (perhaps rename to disambiguate with resources/build/builder.inc.sh)
Or, add a BUILDER_NO_CD flag to scripts that care about initial folder
- alt. could pushd/popd but that looks uglier.

rc-swag · 2024-05-01T05:59:31Z

Okay, one problem here is that build-utils.sh is used for both builder scripts and other utility scripts, some of which are supposed to not change folder (e.g. mkver.inc.sh).

Two ways to fix:

It took me a while to even understand the problem you are solving.
Let me just put it in my words to see if it makes sense. If you add the line cd "$THIS_SCRIPT_PATH" to builder.inc.sh. builder.inc.sh is sourced in build-utils.inc.sh. So now all the subprojects that are sourcing build-utils.inc.sh will have the directory changed to the script path pf the calling script. This is not desirable eg mkver.inc.sh in which you want it to be the directory it is currently in at the time you source build-utils.inc.sh .
I would go with BUILDER_NO_CD flag. That is because adding another script just to not do a cd seems confusing.
What about If $THIS_SCRIPT_PATH was called $EXECUTE_PATH then for those scripts like that do not want to change directory set the $EXECUTE_PATH to pwd directory.

mcdurdin · 2024-05-01T10:21:26Z

Let me just put it in my words to see if it makes sense. If you add the line cd "$THIS_SCRIPT_PATH" to builder.inc.sh. builder.inc.sh is sourced in build-utils.inc.sh. So now all the subprojects that are sourcing build-utils.inc.sh will have the directory changed to the script path pf the calling script. This is not desirable eg mkver.inc.sh in which you want it to be the directory it is currently in at the time you source build-utils.inc.sh .

Yep.

I would go with BUILDER_NO_CD flag. That is because adding another script just to not do a cd seems confusing.
What about If $THIS_SCRIPT_PATH was called $EXECUTE_PATH then for those scripts like that do not want to change directory set the $EXECUTE_PATH to pwd directory.

I kinda started to go this way then realised I'd need to fixup all the random scripts that include build-utils.sh to do this, and it would still leave us in the spot where api contract for builder scripts is vague. So I ended up:

Creating resources/build/builder.inc.sh which imports build-utils.sh and also does the cd
Updating all builder-compliant scripts to use that one
Remaining scripts continue to use build-utils.sh
resources/builder.inc.sh may be renamed in future to avoid confusion

rc-swag · 2024-05-02T01:00:18Z

4. resources/builder.inc.sh may be renamed in future to avoid confusion

Yes I think that needs to happen, something that helps reflect what it does the resources/build/builder.inc.sh doesn't. Actually it is really the other way around isn't?. the later is an extension

mcdurdin · 2024-05-02T01:20:58Z

resources/builder.inc.sh may be renamed in future to avoid confusion

Yes I think that needs to happen, something that helps reflect what it does the resources/build/builder.inc.sh doesn't. Actually it is really the other way around isn't?. the later is an extension

I think resources/builder.inc.sh should become resources/build/builder.implementation.inc.sh or something like that (better not to have it in resources/ directly).

Fixes #11324. * Always `cd "$THIS_SCRIPT_PATH"` * Remove unnecessary `cd` from all build.sh * Remove unnecessary `set -eu` from all build.sh (and `# set -x`) * Replace old build-utils.sh incantation in a few build.sh scripts

Note: there is a bit of potential confusion about the difference between /resources/builder.inc.sh (the full implementation for builder scripts), and /resources/build/builder.inc.sh (the source script that builder scripts should always use). This allows us to make assumptions that will always be true for builder scripts that may not be true for other scripts, such as setting base folder.

builder_describe and builder_parse should come before other tests, so that (a) scripts can return --help details, and (b) so that builder.inc.sh can tell that the script is a valid builder script, even if environment means that the script will not otherwise run.

This prevents issues where Windows-style path pollutes path strings with $KEYMAN_ROOT variable. It typically will only be readonly if we are setting the variable in our own scripts

mcdurdin · 2024-05-02T22:50:17Z

common/linux/build.sh

@@ -23,6 +18,11 @@ builder_describe "Keyman common Linux modules" \

 builder_parse "$@"

+if [[ $BUILDER_OS != linux ]]; then


A builder script must always call builder_describe and builder_parse, so do our platform checks after that. This allows the script to be called with --help even when on the wrong platform, so user can get info about it.

mcdurdin · 2024-05-02T22:50:37Z

common/mac/build.sh

+if [[ $BUILDER_OS != mac ]]; then
+  builder_echo grey "Platform is not macOS; skipping common/mac"
+  exit 0
+fi


A builder script must always call builder_describe and builder_parse, so do our platform checks after that. This allows the script to be called with --help even when on the wrong platform, so user can get info about it.

jahorton

Noting some of the previous conversation and the documentation I saw in the file-changes, I think we have a few removals of change-directory that need to get restored.

common/linux/build.sh

ios/samples/KMSample1/build.sh

ios/samples/KMSample2/build.sh

mac/build.sh

@jahorton

Per @jahorton code review

rc-swag

lgtm

keyman-server · 2024-05-04T18:05:17Z

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

mcdurdin requested review from rc-swag, ermshiperete, jahorton, sgschantz, darcywong00 and SabineSIL as code owners May 1, 2024 03:48

keymanapp-test-bot bot added this to the A18S1 milestone May 1, 2024

mcdurdin commented May 1, 2024

View reviewed changes

mcdurdin marked this pull request as draft May 1, 2024 03:57

mcdurdin force-pushed the chore/common/11324-this-script-path branch from 959137a to e1e7448 Compare May 2, 2024 04:45

mcdurdin added 6 commits May 2, 2024 15:54

chore(common): maintenance on build scripts - cd

0f390d4

Fixes #11324. * Always `cd "$THIS_SCRIPT_PATH"` * Remove unnecessary `cd` from all build.sh * Remove unnecessary `set -eu` from all build.sh (and `# set -x`) * Replace old build-utils.sh incantation in a few build.sh scripts

chore(common): typo in include

c921881

chore(common): typo in test

58db259

chore(common): reset KEYMAN_ROOT if not readonly

41af900

This prevents issues where Windows-style path pollutes path strings with $KEYMAN_ROOT variable. It typically will only be readonly if we are setting the variable in our own scripts

mcdurdin force-pushed the chore/common/11324-this-script-path branch from e1e7448 to 41af900 Compare May 2, 2024 08:54

mcdurdin marked this pull request as ready for review May 2, 2024 22:49

chore(common): rename build-utils.md to builder.md

29f6204

mcdurdin commented May 2, 2024

View reviewed changes

jahorton reviewed May 3, 2024

View reviewed changes

common/linux/build.sh Show resolved Hide resolved

ios/samples/KMSample1/build.sh Outdated Show resolved Hide resolved

ios/samples/KMSample2/build.sh Outdated Show resolved Hide resolved

mac/build.sh Outdated Show resolved Hide resolved

chore(common): revert cd removal from 3 non-builder scripts

50bcfa4

Per @jahorton code review

github-actions bot added ios/samples/ mac/ and removed mac/ ios/samples/ labels May 3, 2024

jahorton approved these changes May 3, 2024

View reviewed changes

rc-swag approved these changes May 3, 2024

View reviewed changes

mcdurdin merged commit c997965 into master May 3, 2024
25 checks passed

mcdurdin deleted the chore/common/11324-this-script-path branch May 3, 2024 23:15

jahorton mentioned this pull request May 8, 2024

chore(web): updates chai, @types/chai dependencies 🏃 #11318

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(common): maintenance on build scripts - cd #11329

chore(common): maintenance on build scripts - cd #11329

mcdurdin commented May 1, 2024 •

edited

keymanapp-test-bot bot commented May 1, 2024 •

edited

mcdurdin May 1, 2024

mcdurdin commented May 1, 2024

rc-swag commented May 1, 2024

mcdurdin commented May 1, 2024

rc-swag commented May 2, 2024

mcdurdin commented May 2, 2024

mcdurdin May 2, 2024

mcdurdin May 2, 2024

jahorton left a comment

rc-swag left a comment

keyman-server commented May 4, 2024

		@@ -23,6 +18,11 @@ builder_describe "Keyman common Linux modules" \

		builder_parse "$@"

		if [[ $BUILDER_OS != linux ]]; then

chore(common): maintenance on build scripts - cd #11329

chore(common): maintenance on build scripts - cd #11329

Conversation

mcdurdin commented May 1, 2024 • edited

keymanapp-test-bot bot commented May 1, 2024 • edited

User Test Results

Test Artifacts

mcdurdin May 1, 2024

Choose a reason for hiding this comment

mcdurdin commented May 1, 2024

rc-swag commented May 1, 2024

mcdurdin commented May 1, 2024

rc-swag commented May 2, 2024

mcdurdin commented May 2, 2024

mcdurdin May 2, 2024

Choose a reason for hiding this comment

mcdurdin May 2, 2024

Choose a reason for hiding this comment

jahorton left a comment

Choose a reason for hiding this comment

rc-swag left a comment

Choose a reason for hiding this comment

keyman-server commented May 4, 2024

mcdurdin commented May 1, 2024 •

edited

keymanapp-test-bot bot commented May 1, 2024 •

edited