chore(mac): rework of main build script #11444

jahorton · 2024-05-15T03:14:23Z

This PR reworks /mac/build.sh to use the same patterns and formats as the primary build.sh scripts for all of our other platforms, thus making it builder-based.

User Testing

TEST_ABILITY_TO_TEST: Verify that the test-bot provides a valid link to the Keyman for macOS installer. Also do very light testing to ensure the app works as before.

There are no changes to the app itself, but since the build script's being overhauled, we want to make sure we didn't break things in subtle ways without realizing it.

keymanapp-test-bot · 2024-05-15T03:14:27Z

User Test Results

Test specification and instructions

✅ TEST_ABILITY_TO_TEST (PASSED): I tested this issue with the attached "Keyman 18.0.45-alpha-local" build on the macOS environment. Here is my observation. (notes)

Test Artifacts

macOS
- Keyman for macOS

mac/build.sh

jahorton · 2024-05-15T03:20:53Z

mac/build.sh

-        builder_die "Writing download_info file failed!"
+do_install() {
+  if ! builder_has_option --quick; then
+    do_notarize


Note: ./build.sh install --debug will fail if not --quick. See do_notarize, which fails when code signing is disabled (currently, "new" line 215)... which is part of debug builds.

The conversion to the format does mean we can no longer do code-signed --debug builds. Push come to shove, we could add a builder-option to force it, though.

Perhaps it would be better to flip the option and/or condition the install-type based on --debug? Talking with @sgschantz, he mentioned doing most local builds the equivalent of --debug and --quick.

If based on --debug, I'm not sure how to best give instructions to resolve the error regarding "Disable SecAssessment". It existed before (see "old" line 270), and I don't want to break something where I'm still a bit unclear.

I currently believe that dropping --quick and basing install-action based notarization on --debug is likely best... or at least, most convenient. That's just my position though, and I can see good arguments being made to the contrary... so between that note and my lack of clarity on the error/warning, I maintained the complexity of the original script pending further feedback.

Let's keep with the original design for those parameters for now and revisit once this is ready.

jahorton · 2024-05-15T03:30:13Z

mac/build.sh

+  "build" \
+  "publish   Publishes debug info to Sentry and builds DMG, .download_info for distribution" \
+  "test" \
+  "install   Installs result of Keyman4MacIM locally." \


Formerly -deploy - either local or quicklocal variant. Renamed to match the install action from #11421, as it has the same purpose.

jahorton · 2024-05-15T03:31:36Z

mac/build.sh

+  "clean" \
+  "configure" \
+  "build" \
+  "publish   Publishes debug info to Sentry and builds DMG, .download_info for distribution" \


Formerly -upload-sentry + -deploy preprelease. As with #11421, these make sense paired as part of a similar, release-oriented action. Developer has a publish action, and consistent nomenclature among our scripts is generally a good thing - it's part of why we made the whole "builder-script" thing.

jahorton · 2024-05-15T03:33:10Z

mac/build.sh

+  ":engine   KeymanEngine4Mac" \
+  ":im   Keyman4MacIM" \
+  ":testapp   Keyman4Mac (test harness)" \
+  "--quick,-q  Bypasses notarization for $(builder_term install)" 


Exists to provide the local vs quicklocal distinction. That said, I suspect we should drop this in favor of simplicity, basing the distinction based on presence of --debug. (See a different comment for more details + request for guidance in the matter.)

mcdurdin

This is looking good. A few minor feedback points. I will work on the CI integration soon (including ensuring that 17.0 continues to work...)

mac/build.sh

mcdurdin · 2024-05-18T10:52:08Z

mac/build.sh

-        builder_die "Writing download_info file failed!"
+do_install() {
+  if ! builder_has_option --quick; then
+    do_notarize


Let's keep with the original design for those parameters for now and revisit once this is ready.

mac/build.sh

mcdurdin · 2024-05-18T10:55:57Z

mac/build.sh

-    echo "For deployment, even locally, the app must be code-signed and notarized by Apple (see README.md for more"
-    echo "options). This requires a valid code certificate and an active Appstoreconnect Apple ID. These must be"
-    echo "supplied in environment variables or in localenv.sh in this folder"
-    echo
-    echo "  * CERTIFICATE_ID: Specifies a certificate from your keychain (e.g. use sha1 or name of certificate)"
-    echo "          Use with -no-codesign if you don't have access to the core developer certificates."
-    echo "          Core development team members should not need to specify this as the certificate reference is"
-    echo "          already configured in the project."
-    echo "  * APPSTORECONNECT_PROVIDER: The shortname of the preferred provider in your Apple Developer account"
-    echo "          To find this, run:"
-    echo "          /Applications/Xcode.app/Contents/Developer/usr/bin/iTMSTransporter \\"
-    echo "            -m provider -u 'USERNAME' -p 'PASSWORD' -account_type itunes_connect -v off"
-    echo "  * APPSTORECONNECT_USERNAME: Your Apple ID login name."
-    echo "  * APPSTORECONNECT_PASSWORD: Your Apple ID password (may need to be a app-specific password)."
-    echo "  * DEVELOPMENT_TEAM: Your development team ID; if unspecified uses the default Keyman development team."


I see this information is captured in mac/README.md

mac/build.sh

darcywong00 · 2024-05-20T00:33:26Z

mac/build.sh

+  fi
+
+  # Create download info
+  eval "$KM4MIM_BASE_PATH/write-download_info.sh"


I think we're not doing evals from an earlier review comment.

probably outside the scope here. but could write-download-info.sh be an "inc"? (e.g. write-download-file.inc.sh that other platforms can use to generate their .download_info files?

Could this replace some of the PowerShell on CI?

probably outside the scope here. but could write-download-info.sh be an "inc"? (e.g. write-download-file.inc.sh that other platforms can use to generate their .download_info files?

Could this replace some of the PowerShell on CI?

Yes outside scope here but looking forward to consolidating this in the future.

I've gone ahead and created #11595 for this.

jahorton · 2024-05-20T01:42:23Z

mac/build.sh

+    execCodeSign eval --force --sign $CERTIFICATE_ID --timestamp --verbose --preserve-metadata=identifier,entitlements "$KM4MIM_BASE_PATH/build/$CONFIG/Keyman.app/Contents/Frameworks/Sentry.framework"

-        execCodeSign eval --force --sign $CERTIFICATE_ID --timestamp --verbose --preserve-metadata=identifier,entitlements "$KM4MIM_BASE_PATH/build/$CONFIG/Keyman.app/Contents/Frameworks/KeymanEngine4Mac.framework"
+    execCodeSign eval --force --sign $CERTIFICATE_ID --timestamp --verbose --preserve-metadata=identifier,entitlements "$KM4MIM_BASE_PATH/build/$CONFIG/Keyman.app/Contents/Frameworks/KeymanEngine4Mac.framework"

-        execCodeSign eval --force --sign $CERTIFICATE_ID --timestamp --verbose -o runtime \
-            --entitlements "$KM4MIM_BASE_PATH/$ENTITLEMENTS_FILE" \
-            --requirements "'=designated => anchor apple generic and identifier \"\$self.identifier\" and ((cert leaf[field.1.2.840.113635.100.6.1.9] exists) or ( certificate 1[field.1.2.840.113635.100.6.2.6] exists and certificate leaf[field.1.2.840.113635.100.6.1.13] exists and certificate leaf[subject.OU] = \"$DEVELOPMENT_TEAM\" ))'" \
-            "$KM4MIM_BASE_PATH/build/$CONFIG/Keyman.app"
-    fi
+    execCodeSign eval --force --sign $CERTIFICATE_ID --timestamp --verbose -o runtime \


I've gone ahead and eliminated the calls that are straightforward from my perspective, but these are a few evals that I'm less clear on how to fix at the moment. I'm away from my Mac dev machine this week due to travel, and I'd definitely want to test changes to these lines locally first.

git blame points me to #10243. There's no linked issue and the description is very sparse, so I'm currently unable to infer details about why the eval vs direct pattern is in place. (I see it was about "code signing robustness" in some way, but that's about all I can glean.)

It's a fairly recent PR as well, so I get the feeling I should leave the eval bits here unless directed otherwise by someone with "headspace documentation"..

Yeah, I am not sure why eval is needed. I would have to experiment, and I have a vague recollection of this being more difficult than it seemed like it should have to be. Let's leave the execCodeSign eval calls for now -- it works after all.

mcdurdin · 2024-05-21T05:16:09Z

I have updated CI to use the new build script. The release build appears to have the wrong search path -- see debug in the paths referenced below -- for the engine tests, in a release build (never before run in that config I guess?):

https://build.palaso.org/buildConfiguration/Keyman_KeymanMac_PullRequests/464688?buildTab=log

[12:08:36] :	 [Step 5/5] 3 warnings generated.
[12:08:36] :	 [Step 5/5] ld: warning: search path '/Users/marc_durdin/buildAgent/work/99b311828f4ee7c/keyman/mac/KeymanEngine4Mac/../../core/build/mac-x86_64/debug/subprojects/icu/source/common' not found
[12:08:36] :	 [Step 5/5] ld: warning: search path '/Users/marc_durdin/buildAgent/work/99b311828f4ee7c/keyman/mac/KeymanEngine4Mac/../../core/build/mac-x86_64/debug/subprojects/icu/source/i18n' not found
[12:08:36] :	 [Step 5/5] ld: warning: search path '/Users/marc_durdin/buildAgent/work/99b311828f4ee7c/keyman/mac/KeymanEngine4Mac/../../core/build/mac/debug' not found
[12:08:36] :	 [Step 5/5] ld: library 'sicuuc' not found

Note that the older CI would do a debug build to run the tests, and then do a release build. The 18.0 CI does a release build and runs tests on that build.

sgschantz · 2024-05-21T07:50:09Z

I ran the debug build locally, ran tests and installed and ran the app and everything looked good.

I did need to include --quick to install locally. If I did only ./build.sh --debug install:app then I got a notarization error.

Also, deleting a core library caused it to be rebuilt, which is good. I don't think that was happening with the old script. (maybe?)

The clean action should only clean mac products and not clean anything that the mac build is dependent upon in core since core is a separate target managed with its own build script -- is that correct? That might be a dumb question, but I don't think I understand the relationship, because core gets built if I delete it and run build:app for mac.

mcdurdin · 2024-05-21T07:54:26Z

The clean action should only clean mac products and not clean anything that the mac build is dependent upon in core since core is a separate target managed with its own build script -- is that correct?

Correct. Dependencies will be automatically configured and built if they are missing. No other actions are automatic (e.g. clean, test, etc).

See

keyman/resources/build/builder.md

Lines 186 to 191 in b7c3d65

    
           * **dependencies**: these are other builder scripts which must be configured and 
        
             built before the actions in this script can continue. Only `configure` and 
        
             `build` actions are ever passed to dependency scripts; these actions will 
        
             execute by default, for all targets of the dependency script.  If you are 
        
             working on code within a dependency, you are currently expected to rebuild and 
        
             test that dependency locally.

for more details.

jahorton · 2024-05-27T02:01:06Z

mac/setup/build.sh

+if [ -e "$SOURCEAPP" ]; then
  rm -rf "$SOURCEAPP"
 fi


-f matches when "FILE" is a plain and simple file, but not when a directory.

Using -e - essentially, "is a valid path" or -d - "is a directory" will work for this check, while the original does not.

(This is why /mac/setup/build.sh was falling over for me on repeated local testing without an intervening /mac/build.sh clean command.)

jahorton · 2024-05-27T02:09:57Z

mac/build.sh

+  builder_heading "Cleaning pods folder (CocoaPods)"
+  rm -rf "$PODS_FOLDER"
+  builder_heading "Cleaning source (Carthage)"
+  rm -rf "$KEYMAN_MAC_BASE_PATH/Carthage"


I personally find it odd to remove configure artifacts during the clean action as is done here. That said, the original version of the script's clean... only cleaned the Carthage output.

I'm making a point of this to ask: should I drop these two lines from the clean action and keep the others? Or should I keep them here to stick closer to this script's original version, even if we don't clean configure outputs in other scripts?

Keyman Engine for Web scripts never clean configure outputs.

Granted, for Web... that's mostly just node_modules. So, it may be a special case.

I'm fairly sure our iOS and Android platforms' scripts similarly don't clean configure outputs.

Android downloads default keyboard + model resources... to a place that doesn't get cleaned. (Just .gitignore'd.)

Similarly for iOS.

Do our windows, developer, or linux platform build scripts ever do so?

We're not 100% consistent on it, and probably can't be, because of the variety of what goes in configure steps.

clean on core (meson) removes the build/<arch> folder which is created in the configure step.

clean on windows and developer, depends on the module type:

node, no, because that's is generally /node_modules

C++, meson toolchain, yes, removes build/<arch> folder which is created in the configure step

C++, msbuild toolchain, no, because configuration is shared across modules in /resources/build/win

Delphi, no, because configuration is shared across modules in /resources/build/win

I'm not too worried about it. The build scripts are pretty good at picking up the pieces after a build.sh clean and that's the main thing.

Also, Carthage has been a problem in the past with instability, which may be why the clean action removed everything there too. Personally, I'd leave the clean of Carthage and Cocoapods in this script for now.

And of course, git clean -fdx is the hardcore clean tool to really deep clean the repo. Such a fresh new feeling.

mcdurdin

LGTM. Some minor nits only.

mac/build.sh

mcdurdin · 2024-05-29T00:01:25Z

CI has been updated for test builds (and seems to be working), and for release builds (will be tested when this merges).

Co-authored-by: Marc Durdin <marc@durdin.net>

jahorton · 2024-05-29T01:33:02Z

One last remaining question: do we want to do something about this thread now, leave it as is and create a follow-up issue, or leave it as is permanently?

Perhaps it would be better to flip the [--quick] option and/or condition the install-type [local vs quicklocal] based on --debug? Talking with @sgschantz, he mentioned doing most local builds the equivalent of --debug and --quick.

Let's keep with the original design for those parameters for now and revisit once this is ready.

It appears we're pretty much there, so consider this the follow-up.

mcdurdin · 2024-05-29T01:56:00Z

One last remaining question: do we want to do something about this thread now, leave it as is and create a follow-up issue, or leave it as is permanently?

I think we leave it as is, and don't create a follow-up issue. I plan to do a future round of consolidation across all our build scripts and will probably revisit script-specific parameters and try and make them consistent (e.g. We have other parameters in Windows, Developer build scripts that could also benefit). But we don't need an issue for it because I would be doing a cross-script analysis anyway to start, so the issue would just add noise to our already very big queue.

dinakaranr · 2024-05-29T15:00:18Z

Test Results

**TEST_ABILITY_TO_TEST (FAILED):**I tested this issue with the attached "Keyman 18.0.31-alpha-local" build on the macOS environment: Here is my observation.

Installed the "Keyman 18.0.31-alpha-local.dmg" file and gave all permissions to the application.
Open the OSK by clicking the keyboard icon, and then click "On-Screen Keyboard.".

Here, a blank screen appeared in the OSK. The Configuration button does not work on the "About" window.If "Always show on-screen keyboard" is enabled, then I am unable to install or uninstall the keyboard, and OSK is not allowed to do it. It seems to be a problem. Hence, I have failed as of now. Please refer to the screenshot. thanks

jahorton · 2024-05-30T02:52:43Z

Discussion with @sgschantz pointed out that I may just need to merge in more recent fixes to this PR. I'm not always on top of when I most recently merged stuff in... and swapping back to master on the dev machine I was using... yeah, it's probably that.

Your branch is behind 'origin/master' by 357 commits, [...]

🤦

@keymanapp-test-bot retest all

dinakaranr · 2024-05-30T10:24:31Z

Test Results

TEST_ABILITY_TO_TEST (PASSED): I tested this issue with the attached "Keyman 18.0.45-alpha-local" build on the macOS environment. Here is my observation.

Installed the "Keyman 18.0.45-alpha-local.dmg" file and gave all permissions to the application.
Open the OSK by clicking the keyboard icon, and then click "On-Screen Keyboard".
Here, a blank screen does not appear in the OSK. The configuration button works on the "About" window. Thank you.

keyman-server · 2024-05-30T21:36:39Z

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

keyman-server · 2024-05-31T19:16:48Z

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

keyman-server · 2024-05-31T23:02:16Z

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

keyman-server · 2024-05-31T23:20:23Z

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

Joshua Horton added 5 commits May 15, 2024 09:48

chore(mac): converts mac's build.sh into a proper builder-based script

a979a3d

chore(mac): patch for last commit

2f88afd

fix(mac): clean should also clean installer

157da20

chore(mac): drops one local mac-only helper .sh b/c redundant

81600de

chore(mac): adds file to serve as CI flag for new param style

b76411d

keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label May 15, 2024

keymanapp-test-bot bot added this to the A18S2 milestone May 15, 2024

github-actions bot added mac/ chore labels May 15, 2024

Joshua Horton added 2 commits May 15, 2024 10:16

chore(mac): reorders the helper-func definitions

fcaceff

chore(mac): better builder_describe_outputs

5cd03ca

jahorton commented May 15, 2024

View reviewed changes

jahorton requested a review from mcdurdin May 15, 2024 03:45

jahorton marked this pull request as ready for review May 15, 2024 03:45

jahorton requested review from sgschantz and SabineSIL as code owners May 15, 2024 03:45

mcdurdin reviewed May 18, 2024

View reviewed changes

darcywong00 reviewed May 20, 2024

View reviewed changes

mac/build.sh Outdated Show resolved Hide resolved

darcywong00 reviewed May 20, 2024

View reviewed changes

chore(mac): incorporates many review suggestions + comments

42d1cf3

jahorton commented May 20, 2024

View reviewed changes

mcdurdin mentioned this pull request May 21, 2024

chore(common): Consolidate build scripts #6268

Open

13 tasks

mcdurdin modified the milestones: A18S2, A18S3 May 24, 2024

Joshua Horton added 2 commits May 27, 2024 08:42

fix(mac): non --debug test references 'release' artifacts

9c4eb99

fix(mac): -e to check if dir exists, as -f doesn't match against dirs

45e0d87

jahorton commented May 27, 2024

View reviewed changes

change(mac): relocates one shell var definition

e49d26a

jahorton commented May 27, 2024

View reviewed changes

mcdurdin approved these changes May 29, 2024

View reviewed changes

mac/build.sh Show resolved Hide resolved

mac/build.sh Outdated Show resolved Hide resolved

mac/build.sh Outdated Show resolved Hide resolved

mac/build.sh Outdated Show resolved Hide resolved

mac/build.sh Outdated Show resolved Hide resolved

jahorton and others added 2 commits May 29, 2024 08:26

change(mac): Apply suggestions from code review

12cb94c

Co-authored-by: Marc Durdin <marc@durdin.net>

chore(mac): replaces more 'typeset' uses

7008a39

jahorton mentioned this pull request May 29, 2024

chore(common): extract build-download.info.inc.sh? #11595

Open

keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed and removed user-test-missing User tests have not yet been defined for the PR labels May 29, 2024

keymanapp-test-bot bot added user-test-failed and removed user-test-required User tests have not been completed labels May 29, 2024

Merge branch 'master' into chore/mac/main-build.sh-rework

320b72b

keymanapp-test-bot bot added user-test-required User tests have not been completed and removed user-test-failed labels May 30, 2024

keymanapp-test-bot bot removed the user-test-required User tests have not been completed label May 30, 2024

jahorton merged commit 7feea5f into master May 30, 2024
5 checks passed

jahorton deleted the chore/mac/main-build.sh-rework branch May 30, 2024 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(mac): rework of main build script #11444

chore(mac): rework of main build script #11444

jahorton commented May 15, 2024 •

edited

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

edited

jahorton May 15, 2024

mcdurdin May 18, 2024

jahorton May 15, 2024

jahorton May 15, 2024 •

edited

jahorton May 15, 2024

mcdurdin left a comment

mcdurdin May 18, 2024

mcdurdin May 18, 2024

darcywong00 May 20, 2024

mcdurdin May 20, 2024

jahorton May 29, 2024

jahorton May 20, 2024

jahorton May 27, 2024 •

edited

mcdurdin May 27, 2024

mcdurdin commented May 21, 2024 •

edited

sgschantz commented May 21, 2024

mcdurdin commented May 21, 2024

jahorton May 27, 2024 •

edited

jahorton May 27, 2024 •

edited

mcdurdin May 27, 2024

mcdurdin left a comment

mcdurdin commented May 29, 2024

jahorton commented May 29, 2024

mcdurdin commented May 29, 2024

dinakaranr commented May 29, 2024

jahorton commented May 30, 2024

dinakaranr commented May 30, 2024

keyman-server commented May 30, 2024

keyman-server commented May 31, 2024

keyman-server commented May 31, 2024

keyman-server commented May 31, 2024

chore(mac): rework of main build script #11444

chore(mac): rework of main build script #11444

Conversation

jahorton commented May 15, 2024 • edited

User Testing

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

User Test Results

Test Artifacts

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahorton May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcdurdin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahorton May 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcdurdin commented May 21, 2024 • edited

sgschantz commented May 21, 2024

mcdurdin commented May 21, 2024

jahorton May 27, 2024 • edited

Choose a reason for hiding this comment

jahorton May 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcdurdin left a comment

Choose a reason for hiding this comment

mcdurdin commented May 29, 2024

jahorton commented May 29, 2024

mcdurdin commented May 29, 2024

dinakaranr commented May 29, 2024

Test Results

jahorton commented May 30, 2024

dinakaranr commented May 30, 2024

Test Results

keyman-server commented May 30, 2024

keyman-server commented May 31, 2024

keyman-server commented May 31, 2024

keyman-server commented May 31, 2024

jahorton commented May 15, 2024 •

edited

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

edited

jahorton May 15, 2024 •

edited

jahorton May 27, 2024 •

edited

mcdurdin commented May 21, 2024 •

edited

jahorton May 27, 2024 •

edited

jahorton May 27, 2024 •

edited