Run iOS build in CI by rajveermalviya · Pull Request #2307 · zulip/zulip-flutter

rajveermalviya · 2026-05-04T21:07:43Z

Fixes #773.
Fixes #329.

chrisbobbe

Thanks, this should be helpful!

Could #329 be done by basically just adding a check_no_changes call at the end of the new ios suite? That would be another nice win for our CI coverage of iOS.

chrisbobbe · 2026-05-07T01:02:46Z

+    - name: Install dependencies
+      run: brew install coreutils bash shellcheck


Interesting; I guess the need for coreutils and bash is explained in tools/lib/ensure-shell-deps.sh. Would you add a comment about that?

I'm not sure that shellcheck is needed; doesn't that get run in the "analyze, test, generate" job?

chrisbobbe · 2026-05-07T01:09:07Z

+    # iOS builds require macOS and Xcode.
+    if [ "$(uname -s)" != "Darwin" ]; then
+        if_verbose echo "Skipping ios suite (not on macOS)."
+        return 0
+    fi


In non-verbose mode, I think we'd ideally omit the "Skipping ios suite" and also the "Running ios..." line that precedes it. If omitting "Running ios..." seems complicated, I think it would be better to just log both lines, i.e. cut the if_verbose condition here.

chrisbobbe · 2026-05-07T01:11:35Z

+    xcrun swift-format lint --strict \
+        --configuration ios/.swift-format \
+        "${swift_targets[@]}" \
+    || return


Could we wire this up such that tools/check ios --fix causes the lint issues to be fixed?

chrisbobbe · 2026-05-07T01:12:32Z

@@ -34,7 +34,7 @@ default_suites=(
    analyze test
    flutter_version
    build_runner l10n drift pigeon icons
-    android  # This takes multiple minutes in CI, so do it last.
+    android ios # These take multiple minutes in CI, so do them last.


nit: this file's convention seems to be two spaces before "#"

chrisbobbe · 2026-05-07T01:16:08Z

+// Compile Swift with "-warnings-as-errors"; but only in release builds.
+SWIFT_TREAT_WARNINGS_AS_ERRORS = YES


Interesting. Do debug and release builds give different warnings? What's the motivation for only doing this in release builds?

This is similar to what we currently have for Android:

zulip-flutter/android/app/build.gradle

Lines 94 to 102 in a48fad2

tasks.withType(org.jetbrains.kotlin.gradle.tasks.KotlinJvmCompile).configureEach {

// Compile Kotlin with `-Werror`... but only in release builds, so that it

// doesn't get in the way of quick local experiments for debugging.

//

// The string-searching makes this a bit of a mess, but it works.

// Better would be if we can add this to android.buildTypes.release above;

// but on a first attempt that didn't work (it affected debug builds too).

compilerOptions.allWarningsAsErrors = name.contains("Release")

}

The motivation is to not get in the way of quick local experiments for debugging? Seems reasonable; let's note that in a code comment if so.

rajveermalviya · 2026-05-11T14:21:49Z

Thanks for the review @chrisbobbe! Pushed an update, PTAL.

chrisbobbe

Not a full review (I'll come back for that soon), but I didn't want to lose track of an idea I had just now 🙂—

chrisbobbe · 2026-05-13T01:34:12Z

+      run: tools/ci/clone-flutter-sdk
+
+    - name: Download Flutter SDK artifacts (flutter precache)
+      run: flutter precache --universal


Can we save time by doing flutter precache --ios instead?

If so, then probably we can do the corresponding thing for the Android-only workflow. (Can we even skip flutter precache for the "analyze, test, generate" workflow, which doesn't do either build?)

From the output of flutter precache --help there doesn't seem to be an option for android, but actually flutter precache seems unnecessary in CI because Flutter tooling can automatically download only the required SDK artifacts for the invoked command. So, I think we can remove flutter precache step from all the jobs.

rajveermalviya · 2026-05-14T15:26:31Z

Thanks! Pushed an update, PTAL.

chrisbobbe

Thanks! Comments below, mostly small.

chrisbobbe · 2026-05-14T17:29:55Z

+      # Required by tools/lib/ensure-shell-deps.sh
+      run: brew install coreutils bash


The reference in the comment is helpful, because it helps the reader infer why we add this step in the iOS workflow but not the others. The reason is buried in the fourth paragraph of a long comment in tools/lib/ensure-shell-deps.sh, though, which makes it harder to discover:

# So this is really all about macOS, which comes with an ancient # Bash 3.2.57 dating to 2007 and an '80s-style BSD utility suite. # […]

So let's paraphrase it in the comment here. E.g.:

Suggested change

# Required by tools/lib/ensure-shell-deps.sh

run: brew install coreutils bash

# Modern Bash and coreutils don't come with macOS; get those.

# (See tools/lib/ensure-shell-deps.sh.)

run: brew install coreutils bash

chrisbobbe · 2026-05-14T17:31:43Z

-    - name: Download Flutter SDK artifacts (flutter precache)
-      run: flutter precache --universal


git grep precache shows a dangling reference to flutter precache in tools/check.

chrisbobbe · 2026-05-14T17:41:58Z

+        "${swift_targets[@]}" \
+    || return
+
+    check_no_changes "changes to swift files (swift-format)" ios/'*'.swift \


nit in dev-facing string:

Suggested change

check_no_changes "changes to swift files (swift-format)" ios/'*'.swift \

check_no_changes "changes to Swift files (swift-format)" ios/'*'.swift \

(Capitalize the name of the Swift programming language.)

chrisbobbe · 2026-05-14T17:45:03Z

+    check_no_changes "changes to ios" ios/ \
+    || return


nit: can be clearer that concretely the ios/ directory is meant:

Suggested change

check_no_changes "changes to ios" ios/ \

|| return

check_no_changes "changes to ios/" ios/ \

|| return

chrisbobbe · 2026-05-14T17:50:09Z

+    if_verbose echo "Checking for style issues in Swift code..."
+
+    check_no_uncommitted_or_untracked ios/'*'.swift \
+    || return


If the user accidentally left some uncommitted or untracked changes, then this check_no_uncommitted_or_untracked will fail. But that failure will come right after the tool logs "Checking for style issues in Swift code...", which seems like it would be misleading—there aren't necessarily any style issues, and the style-check hasn't even happened yet.

Probably the fix is to switch the order of these.

chrisbobbe · 2026-05-14T17:51:21Z

+    if_verbose echo "Checking for automated changes from Flutter tooling..."
+
+    check_no_uncommitted_or_untracked ios/ \
+    || return


Similarly here, about failing right after the log "automated changes from Flutter tooling" when the command that might produce the automated changes hasn't run yet.

chrisbobbe · 2026-05-14T17:55:36Z

+    # TODO: Once we have a dedicated code formatting check,
+    #       move this swift-format check there.
+    xcrun swift-format format --in-place \
+        --configuration ios/.swift-format \
+        "${swift_targets[@]}" \
+    || return


Suggested change

# TODO: Once we have a dedicated code formatting check,

# move this swift-format check there.

xcrun swift-format format --in-place \

--configuration ios/.swift-format \

"${swift_targets[@]}" \

|| return

# --in-place enables `tools/check --fix ios`; see `check_no_changes`.

# TODO: Once we have a dedicated code formatting check,

# move this swift-format check there.

xcrun swift-format format --in-place \

--configuration ios/.swift-format \

"${swift_targets[@]}" \

|| return

chrisbobbe · 2026-05-14T18:06:51Z

+// Compile Swift with "-warnings-as-errors"; but only in release builds.
+SWIFT_TREAT_WARNINGS_AS_ERRORS = YES


Bump on #2307 (comment).

chrisbobbe · 2026-05-14T18:42:32Z

+    if_verbose echo "Checking for automated changes from Flutter tooling..."
+
+    check_no_uncommitted_or_untracked ios/ \
+    || return
+
+    flutter build ios "${codesign_opt[@]}" --config-only \
+    || return
+
+    check_no_changes "changes to ios" ios/ \
+    || return


check: In iOS, check for automated changes from Flutter tooling Fixes-partly #329, for iOS.

I see. "Partly" is just because the issue also says flutter build macos --config-only, and we don't do that here; right?

Would the following work, for that?

Suggested change

if_verbose echo "Checking for automated changes from Flutter tooling..."

check_no_uncommitted_or_untracked ios/ \

|| return

flutter build ios "${codesign_opt[@]}" --config-only \

|| return

check_no_changes "changes to ios" ios/ \

|| return

if_verbose echo "Checking for automated changes from Flutter tooling..."

check_no_uncommitted_or_untracked ios/ macos/ \

|| return

flutter build ios "${codesign_opt[@]}" --config-only \

|| return

# Check macos/ too, because desktop platforms are supported for dev:

# https://github.com/zulip/zulip-flutter#desktop-support

# (This could live in its own suite. If doing that, we'd probably

# still want to run it in the same GitHub CI workflow as the iOS build,

# since GitHub's macOS-based runners use a lot of our free quota:

# https://github.com/zulip/zulip-flutter/issues/329#issue-1950704934

# .)

flutter build macos "${codesign_opt[@]}" --config-only \

|| return

check_no_changes "changes to ios" ios/ \

|| return

check_no_changes "changes to macos" macos/ \

|| return

chrisbobbe · 2026-05-14T18:50:56Z

+    check_no_uncommitted_or_untracked ios/ \
+    || return
+
+    flutter build ios "${codesign_opt[@]}" --config-only \


Not sure if we want to pass through the --no-pub option to these flutter build commands, like we do for flutter test and flutter analyze—what do you think?

Good catch! I think we should pass through --no-pub option, otherwise each flutter build command will unnecessarily run pub get: flutter build ios --config-only, flutter build macos --config-only and finally flutter build ipa.

rajveermalviya · 2026-05-18T17:20:16Z

Thanks for the review @chrisbobbe! Pushed an update, PTAL.

chrisbobbe

Thanks!

Using Claude's /code-review slash-command, I noticed one wart that Claude claims our other suites don't have: if some Swift code has formatting issues, then tools/check ios --fix will make changes in-place and then immediately fail with the message "Aborting, to avoid losing your work." That could be confusing (the changes aren't "your work"), and it'd be better to not do that.

But this only bites when you pass --fix, and we touch the Swift code relatively infrequently, and we don't pass --fix in CI. so I'll go ahead and merge this to clear #773 and #329, after fixing the one nit below. Then, since I've got some commits from my Claude session to address the --fix wart and a few other small things, I'll send those as a separate PR.

chrisbobbe · 2026-06-03T01:05:53Z

+    # it's fast subsequent times.)  That's even after Flutter tool's
+    # binary artifacts already downloaded (in CI, that's downloaded
+    # during `flutter pub get` step).


Suggested change

# it's fast subsequent times.) That's even after Flutter tool's

# binary artifacts already downloaded (in CI, that's downloaded

# during `flutter pub get` step).

# it's fast subsequent times.) That's even when the Flutter tool's

# binary artifacts have already been downloaded (which, in CI,

# happens during the `flutter pub get` step).

This step is unnecessary because Flutter tooling can automatically download only the required SDK artifacts for the invoked command. [chris: tweaked comment for grammar] Co-authored-by: Chris Bobbe <cbobbe@zulip.com>

The suite only runs on macOS; on other platforms it is skipped with an appropriate message logged.

Fixes zulip#773.

Fixes zulip#329.

chrisbobbe · 2026-06-03T04:13:36Z

Done, after rebasing atop current main and confirming that the new iOS suite passes.

chrisbobbe · 2026-06-03T17:01:50Z

And sent #2333.

rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label May 4, 2026

rajveermalviya assigned chrisbobbe May 4, 2026

rajveermalviya requested a review from chrisbobbe May 4, 2026 21:08

rajveermalviya force-pushed the pr-ci-ios-build branch from c94e750 to 9999dfc Compare May 6, 2026 08:14

chrisbobbe reviewed May 7, 2026

View reviewed changes

rajveermalviya force-pushed the pr-ci-ios-build branch 2 times, most recently from 4ac1344 to 5ab714a Compare May 11, 2026 14:05

rajveermalviya requested a review from chrisbobbe May 11, 2026 14:21

chrisbobbe reviewed May 13, 2026

View reviewed changes

rajveermalviya force-pushed the pr-ci-ios-build branch from 5ab714a to f4bb7df Compare May 14, 2026 15:14

rajveermalviya requested a review from chrisbobbe May 14, 2026 15:26

chrisbobbe reviewed May 14, 2026

View reviewed changes

rajveermalviya force-pushed the pr-ci-ios-build branch 2 times, most recently from 68911ca to 870670f Compare May 18, 2026 17:19

rajveermalviya requested a review from chrisbobbe May 18, 2026 17:20

rajveermalviya force-pushed the pr-ci-ios-build branch 2 times, most recently from 1fc6e07 to 169b362 Compare May 18, 2026 18:58

rajveermalviya mentioned this pull request Jun 1, 2026

notif ios: Fix notifications being silent on iOS #2300

Open

rajveermalviya added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Jun 1, 2026

chrisbobbe reviewed Jun 3, 2026

View reviewed changes

chrisbobbe force-pushed the pr-ci-ios-build branch 2 times, most recently from 4421708 to f889302 Compare June 3, 2026 03:52

rajveermalviya and others added 4 commits June 2, 2026 23:55

ci: Remove flutter precache steps from jobs

0493e7a

This step is unnecessary because Flutter tooling can automatically download only the required SDK artifacts for the invoked command. [chris: tweaked comment for grammar] Co-authored-by: Chris Bobbe <cbobbe@zulip.com>

ios build: Treat Swift warnings as errors, in release builds

abf6641

check: Add ios suite

a94f8fa

The suite only runs on macOS; on other platforms it is skipped with an appropriate message logged.

ci: Add iOS build job

3522656

Fixes zulip#773.

rajveermalviya added 3 commits June 2, 2026 23:55

ios [nfc]: Xcode run swift-format

a6a1cde

check: In iOS check, lint Swift files with swift-format

00a281a

check: Check for automated changes from Flutter tooling in iOS suite

6f4427c

Fixes zulip#329.

chrisbobbe force-pushed the pr-ci-ios-build branch from f889302 to 6f4427c Compare June 3, 2026 03:55

chrisbobbe merged commit 6f4427c into zulip:main Jun 3, 2026
3 checks passed

chrisbobbe mentioned this pull request Jun 3, 2026

check: Split out new swift_format suite; run it on macOS/ files too #2333

Open

		- name: Install dependencies
		run: brew install coreutils bash shellcheck

		// Compile Swift with "-warnings-as-errors"; but only in release builds.
		SWIFT_TREAT_WARNINGS_AS_ERRORS = YES

	tasks.withType(org.jetbrains.kotlin.gradle.tasks.KotlinJvmCompile).configureEach {
	// Compile Kotlin with `-Werror`... but only in release builds, so that it
	// doesn't get in the way of quick local experiments for debugging.
	//
	// The string-searching makes this a bit of a mess, but it works.
	// Better would be if we can add this to android.buildTypes.release above;
	// but on a first attempt that didn't work (it affected debug builds too).
	compilerOptions.allWarningsAsErrors = name.contains("Release")
	}

		# Required by tools/lib/ensure-shell-deps.sh
		run: brew install coreutils bash

		- name: Download Flutter SDK artifacts (flutter precache)
		run: flutter precache --universal

	check_no_changes "changes to swift files (swift-format)" ios/'*'.swift \
	check_no_changes "changes to Swift files (swift-format)" ios/'*'.swift \

Conversation

rajveermalviya commented May 4, 2026 • edited by chrisbobbe Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajveermalviya commented May 11, 2026

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajveermalviya commented May 14, 2026

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajveermalviya commented May 18, 2026

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jun 3, 2026 • 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!

Uh oh!

chrisbobbe commented Jun 3, 2026

Uh oh!

chrisbobbe commented Jun 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rajveermalviya commented May 4, 2026 •

edited by chrisbobbe

Loading

chrisbobbe Jun 3, 2026 •

edited

Loading