diff --git a/AGENTS.md b/AGENTS.md index 90b6441..a5996b5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -54,6 +54,16 @@ mix test --exclude integration # skip the device-dependent ones prefers `Documents/otp/` over its complete signed bundle. Replace that directory rather than incrementally merging it, require `.beam` before transfer, and verify the received bootstrap bytes before restarting. +- **iOS bundle ids resolve through one function per side, never `bundle_id/0`.** + Consumers (deploy, connect, provision, battery bench) use + `MobDev.Config.ios_bundle_id/0`; the build (sim bundle, device bundle, + codesign) uses `NativeBuild.ios_bundle_id/1` over the loaded cfg. Both are + `:ios_bundle_id || :bundle_id`. Reaching for plain `bundle_id/0` on an + iOS path is the bug that installed an app under one id and pushed BEAMs at + another ("App '…' is not installed on this device" right after a + successful install). Android keeps `bundle_id/0` — the two ids often + cannot be the same string (Apple forbids `_`). See + `decisions/2026-08-08-ios-bundle-id-single-source-and-deploy-exit-code.md`. - **`xcodebuild` errors get rewritten** to actionable hints by `diagnose_xcodebuild_failure/1` in `mob.provision`. Apple's verbatim text is preserved alongside our hint so the snippet stays google-able. Add new @@ -152,8 +162,15 @@ narrowing functions). Don't make them private: - `Mix.Tasks.Mob.Doctor.__zig_install_fix__/0` - `Mix.Tasks.Mob.Doctor.__zig_check_result__/1` - `Enable.inject_pythonx_dep/1`, `inject_pythonx_uv_init_gate/2`, `python_paths_module_template/1` +- `NativeBuild.ios_bundle_id/1` (the `:ios_bundle_id || :bundle_id` rule for the build side) +- `Deployer.ios_bundle_id/0`, `Connector.ios_bundle_id/0` (the same rule on the + consumer side — public so the WIRING is testable, not just the resolver; + reverting either to `bundle_id/0` was the original defect and the suite + did not notice) - `Emulators.parse_simctl_json/1`, `find_emulator_binary/1` - `Provision.diagnose_xcodebuild_failure/1` +- `Mix.Tasks.Mob.Deploy.failure_message/3` (which bucket makes a deploy exit non-zero) +- `Uninstaller.resolve_apps_for_device/3` (which id gets uninstalled, per platform) If you make any of these private, every downstream test breaks loudly — but you'll lose the ability to evolve the parsers safely. diff --git a/CHANGELOG.md b/CHANGELOG.md index 2bcaced..d0211ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,51 @@ Full module documentation: [hexdocs.pm/mob_dev](https://hexdocs.pm/mob_dev). ## [Unreleased] +### Changed + +- **`mix mob.deploy` now exits non-zero when a device fails.** A run that + printed `Failed on 1 device(s)` previously still returned status 0, so CI and + wrapper scripts read a failed deploy as success. Every device is still + attempted and the full summary still printed first — only the exit code + changed. A device that is *skipped* because the app is not installed on it + stays non-fatal on both platforms. **If you have CI that deploys to several + devices and has been passing, it may now fail** — check whether it was + passing on a partial deploy. + +### Added + +- **`:ios_bundle_id`** in `mob.exs`, for when Android's `applicationId` is not a + legal Apple bundle id. Apple forbids the underscores Android allows, and + `com.example.*` is often already claimed by another Apple team, so the two + frequently cannot be the same string. Every iOS path — deploy, connect, + provision, battery bench, uninstall, the simulator and device builds, + code-signing, and the release IPA — resolves `:ios_bundle_id || :bundle_id`. + Android keeps using `:bundle_id`. + +### Fixed + +- **iOS deploys installed the app under one bundle id and pushed BEAMs at + another.** `:ios_bundle_id` was resolved by the build but discarded by the + deployer and connector, so `mix mob.deploy --native --device ` installed + successfully and then failed with *"App '…' is not installed on this device."* + On a machine that also had an older build under the other id it was worse: the + push silently succeeded against the wrong app and reported success. +- **The simulator and device builds disagreed about the bundle id.** The + simulator build took it verbatim from `ios/Info.plist` while the device build + used the configured id, so `simctl launch ` failed. Both paths + now stamp and print the id they installed. +- **The device build looked its provisioning profile up by the Android id**, so + a profile minted by `mix mob.provision` (which uses the iOS id) was never + matched. +- **The release IPA was stamped and signed with the Android `applicationId`**, + which App Store Connect rejects when it contains an underscore. +- **`mix mob.uninstall` targeted the Android id on iOS devices**, removing + nothing and reporting success. +- **`mix mob.doctor` warned that `bundle_id` was unset** for a project that + correctly set only `ios_bundle_id`. +- **Stamping `CFBundleIdentifier` crashed on an `Info.plist` that lacked the + key** — plausible for `mix mob.adopt` projects — instead of adding it. + ## [0.6.33] - 2026-09-04 ### Fixed diff --git a/decisions/2026-08-08-ios-bundle-id-single-source-and-deploy-exit-code.md b/decisions/2026-08-08-ios-bundle-id-single-source-and-deploy-exit-code.md new file mode 100644 index 0000000..ea98e77 --- /dev/null +++ b/decisions/2026-08-08-ios-bundle-id-single-source-and-deploy-exit-code.md @@ -0,0 +1,72 @@ +# One iOS bundle id everywhere, and a non-zero exit on failed deploy + +- Date: 2026-08-08 +- Status: accepted + +## Context + +Three defects found deploying one app to a physical iPhone and an Android +phone, with `bundle_id: "com.example.mishka_mob"` (Android's +`applicationId`, underscore and all) plus +`ios_bundle_id: "com.genericjam.mishkamob"` in `mob.exs`: + +1. `MobDev.Deployer` resolved the iOS id with `MobDev.Config.bundle_id/0`, + discarding `:ios_bundle_id`. `MobDev.NativeBuild` honoured it. So + `mix mob.deploy --native --device ` installed the app under the + configured id and then pushed BEAMs at an id that was never installed: + `App 'com.example.mishka_mob' is not installed on this device.` The only + workaround was clobbering `:bundle_id` — which Android may not accept + (Apple forbids `_` in a bundle id; `com.example.*` is frequently already + claimed by another Apple team). +2. That run printed `Failed on 1 device(s)` and exited **0**. +3. The iOS *simulator* bundle never stamped `CFBundleIdentifier`, so it + inherited whatever `ios/Info.plist` carried, while the *device* bundle + stamped the configured id. Same project, two ids, and no way to tell + which one a given build used. + +## Decision + +**One resolver per side.** `MobDev.Config.ios_bundle_id/0` +(`:ios_bundle_id || bundle_id/0`) is what every iOS-targeting *consumer* +resolves — `Deployer`, `Connector`, `mob.provision`, `mob.battery_bench_ios`. +`MobDev.NativeBuild.ios_bundle_id/1` (`cfg[:ios_bundle_id] || cfg[:bundle_id]`) +is the same rule over the already-loaded build config, used by the sim +bundle, the device bundle, and code signing. The two agree because +`load_config/0` resolves `cfg[:bundle_id]` through `Config.bundle_id/0`. + +`mob.provision` is included deliberately: it mints the provisioning profile, +which must cover the id `NativeBuild` actually signs. + +**Simulator now stamps `CFBundleIdentifier` too**, rather than accepting the +divergence and only reporting it. No signing step rewrites the id on either +path, so there was no technical reason for them to differ. For projects that +never set `:bundle_id`/`:ios_bundle_id` this is a no-op — `bundle_id/0` +already falls back to `ios/Info.plist`, so the stamped value equals the +inherited one. Both paths additionally *print* the id they installed, since +`simctl launch` / `devicectl` / `mob.connect` all need it and it appeared +nowhere in the build output. + +**`mix mob.deploy` exits non-zero when the `failed` bucket is non-empty** +(`Mix.Tasks.Mob.Deploy.failure_message/3` → `Mix.raise`), after the full +summary is printed. + +- `skipped` stays non-fatal. It means "app not installed for that platform", + the expected outcome of building `--ios` with an Android phone also + plugged in. This keeps faith with the earlier fix that split `skipped` out + of `Failed on N` in `format_summary/4`. +- Partial success is fatal. The fan-out is unchanged — every targeted device + is still attempted and reported, so an operator can see which ones got the + BEAMs. But a *script* cannot notice that one device missed out if the + status code says everything is fine, and that is precisely who the exit + code is for. + +## Consequences + +- A CI job that deploys to several devices and previously "passed" with one + device failing now fails. That is the point, but it is a behaviour change + for anyone who was relying on the old status code. +- `:ios_bundle_id` is now a genuinely usable setting rather than one the + build honours and the deployer ignores; cross-platform projects no longer + have to pick an id that satisfies both Apple and Android. +- `NativeBuild.ios_bundle_id/1` joins the public-but-undocumented seams + (listed in `AGENTS.md`) — public for testing, don't privatise. diff --git a/lib/mix/tasks/mob.battery_bench_ios.ex b/lib/mix/tasks/mob.battery_bench_ios.ex index dec409a..7107d12 100644 --- a/lib/mix/tasks/mob.battery_bench_ios.ex +++ b/lib/mix/tasks/mob.battery_bench_ios.ex @@ -201,7 +201,7 @@ defmodule Mix.Tasks.Mob.BatteryBenchIos do # device_id is what we pass to xcrun devicectl (install, launch, terminate). udid = device_id - pkg = MobDev.Config.bundle_id() + pkg = MobDev.Config.ios_bundle_id() cfg = MobDev.Config.load_mob_config() # Workspace discovery is only needed when building. Skip it with --no-build. @@ -645,7 +645,7 @@ defmodule Mix.Tasks.Mob.BatteryBenchIos do defp dry_run!(opts) do cfg = MobDev.Config.load_mob_config() - pkg = MobDev.Config.bundle_id() + pkg = MobDev.Config.ios_bundle_id() scheme = opts[:scheme] || cfg[:ios_scheme] || diff --git a/lib/mix/tasks/mob.deploy.ex b/lib/mix/tasks/mob.deploy.ex index 6d76480..3e76764 100644 --- a/lib/mix/tasks/mob.deploy.ex +++ b/lib/mix/tasks/mob.deploy.ex @@ -116,6 +116,15 @@ defmodule Mix.Tasks.Mob.Deploy do # iOS simulator xcodebuild -scheme -destination 'platform=iOS Simulator,...' build xcrun simctl install booted .app + + ## Exit status + + Every targeted device is attempted and the full summary printed, then the + task exits non-zero if **any** device landed in the `Failed on N device(s)` + bucket — including a partial success where other devices deployed fine. + + Devices under `Skipped on N device(s)` (app not installed for that + platform) do not fail the run. """ @switches [ @@ -242,8 +251,38 @@ defmodule Mix.Tasks.Mob.Deploy do ) Enum.each(format_summary(deployed, failed, skipped, restart: restart), &IO.puts/1) + + # The full summary is printed first, then the status code is set — the + # fan-out across devices is unchanged, only the exit code is. + case failure_message(deployed, failed, skipped) do + nil -> :ok + message -> Mix.raise(message) + end end + @doc """ + The `Mix.raise` message for a finished deploy, or `nil` when the run + should exit 0. + + A deploy that printed "Failed on N device(s)" used to still exit 0, so + CI and wrapper scripts read a failed deploy as a success. + + Only `failed` (a real error during push) is fatal. `skipped` is not: + it means "app not installed for that platform", the expected outcome of + e.g. building `--ios` with an Android phone also plugged in — the same + distinction `format_summary/4` renders. + + Partial success is still a failure. Every targeted device is still + attempted and reported before this runs, so the operator can see which + ones got the BEAMs; a *script* has no way to notice one device missed + out if the status code says everything is fine. + """ + @spec failure_message([Device.t()], [Device.t()], [Device.t()]) :: String.t() | nil + def failure_message(_deployed, [], _skipped), do: nil + + def failure_message(_deployed, failed, _skipped), + do: "Deploy failed on #{length(failed)} device(s) — see errors above" + @doc """ Build the per-deploy summary lines from the three device buckets. diff --git a/lib/mix/tasks/mob.doctor.ex b/lib/mix/tasks/mob.doctor.ex index 53351a7..0939966 100644 --- a/lib/mix/tasks/mob.doctor.ex +++ b/lib/mix/tasks/mob.doctor.ex @@ -567,7 +567,10 @@ defmodule Mix.Tasks.Mob.Doctor do end defp check_bundle_id(cfg) do - case cfg[:bundle_id] do + # `ios_bundle_id` alone is a legitimate configuration — an iOS-only project + # whose Apple id is the only one it needs. Warning about a missing + # `bundle_id` there sends the user to add a key nothing reads. + case cfg[:bundle_id] || cfg[:ios_bundle_id] do nil -> {:warn, "bundle_id", "not set in mob.exs (only needed for mob.battery_bench)", "Add to mob.exs: config :mob_dev, bundle_id: \"com.example.myapp\""} diff --git a/lib/mix/tasks/mob.provision.ex b/lib/mix/tasks/mob.provision.ex index 73f7b6f..866ba4b 100644 --- a/lib/mix/tasks/mob.provision.ex +++ b/lib/mix/tasks/mob.provision.ex @@ -282,7 +282,9 @@ defmodule Mix.Tasks.Mob.Provision do end defp check_bundle_id! do - bundle_id = MobDev.Config.bundle_id() + # Must match what NativeBuild signs the .app with, or the profile this + # task provisions won't cover the installed binary. + bundle_id = MobDev.Config.ios_bundle_id() IO.puts(" #{green()}✓#{reset()} Bundle ID — #{bundle_id}") bundle_id end diff --git a/lib/mob_dev/config.ex b/lib/mob_dev/config.ex index d5864a3..6c43ae2 100644 --- a/lib/mob_dev/config.ex +++ b/lib/mob_dev/config.ex @@ -28,6 +28,25 @@ defmodule MobDev.Config do "#{bundle_prefix()}.#{app_name()}" end + @doc """ + Returns the iOS bundle ID: `mob.exs`'s `:ios_bundle_id` when set, + otherwise `bundle_id/0`. + + iOS and Android often *cannot* share one identifier — Apple forbids + underscores in a bundle id (Android's `applicationId` allows them), and + a `com.example.*` id is frequently already claimed by another Apple team. + `:ios_bundle_id` is the per-platform escape hatch. + + Every iOS-targeting caller must resolve through here. The native build + signs and installs with this value, so a caller that talks to the + installed app by `bundle_id/0` instead (terminate/launch/`devicectl + copy`) addresses an id that was never installed — the failure surfaces + as "App '...' is not installed on this device" *after* a successful + install. + """ + @spec ios_bundle_id() :: String.t() + def ios_bundle_id, do: load_mob_config()[:ios_bundle_id] || bundle_id() + @doc """ Default reverse-DNS prefix when no platform manifest is available. Honors `MOB_BUNDLE_PREFIX` so users with a corporate prefix can set diff --git a/lib/mob_dev/connector.ex b/lib/mob_dev/connector.ex index e3b3498..e46f83f 100644 --- a/lib/mob_dev/connector.ex +++ b/lib/mob_dev/connector.ex @@ -10,7 +10,9 @@ defmodule MobDev.Connector do defp bundle_id, do: MobDev.Config.bundle_id() defp android_package, do: bundle_id() - defp ios_bundle_id, do: bundle_id() + @doc false + @spec ios_bundle_id() :: String.t() | nil + def ios_bundle_id, do: MobDev.Config.ios_bundle_id() # ms to wait for node to appear @connect_timeout 25_000 # ms between polls @@ -161,7 +163,7 @@ defmodule MobDev.Connector do |> Enum.map(& &1.serial) |> MapSet.new() - case System.cmd("pgrep", ["-fl", bundle_id()], stderr_to_stdout: true) do + case System.cmd("pgrep", ["-fl", ios_bundle_id()], stderr_to_stdout: true) do {output, 0} -> output |> String.split("\n", trim: true) diff --git a/lib/mob_dev/deployer.ex b/lib/mob_dev/deployer.ex index 6ab3f24..f05cd9e 100644 --- a/lib/mob_dev/deployer.ex +++ b/lib/mob_dev/deployer.ex @@ -35,7 +35,9 @@ defmodule MobDev.Deployer do defp android_package, do: bundle_id() defp android_app_data, do: "/data/data/#{android_package()}/files" defp android_beams_dir, do: "#{android_app_data()}/otp/#{app_name()}" - defp ios_bundle_id, do: bundle_id() + @doc false + @spec ios_bundle_id() :: String.t() | nil + def ios_bundle_id, do: MobDev.Config.ios_bundle_id() defp ios_beams_dir do # The simulator's OTP_ROOT is resolved by `MobDev.Paths.sim_runtime_dir/1`. @@ -1163,28 +1165,38 @@ defmodule MobDev.Deployer do :ok {out, _} -> - reason = - if String.contains?(out, "ContainerLookupErrorDomain") do - """ - App '#{bundle}' is not installed on this device. - - To fix this, you need to build and install the app on the device first. - The easiest way is to open the ios/ directory in Xcode and run on device: - - open ios/*.xcodeproj (or ios/*.xcworkspace) - - Then select your device in Xcode and press Run (⌘R). - - Alternatively, if you have another app with a different bundle ID already - installed on the device, update bundle_id in mob.exs to match it: - - config :mob_dev, bundle_id: "com.yourcompany.yourapp" - """ - else - "devicectl copy failed: #{out}" - end - - throw({:error, reason}) + # "Not installed" is the same condition Android reports as :skipped + # (deploy_android/2), and it must be bucketed the same way. It means + # "this device is not a target for this app" — a phone that happens to + # be plugged in — not "the deploy failed". Since mob.deploy started + # returning a non-zero exit code for failures, tagging this as an + # error made a plain `mix mob.deploy` fail on any Mac with an + # unrelated iPhone attached. + if String.contains?(out, "ContainerLookupErrorDomain") do + throw( + {:skipped, + """ + App '#{bundle}' is not installed on this device. + + To fix this, you need to build and install the app on the device first. + The easiest way is to open the ios/ directory in Xcode and run on device: + + open ios/*.xcodeproj (or ios/*.xcworkspace) + + Then select your device in Xcode and press Run (⌘R). + + Alternatively, if you have another app with a different bundle ID already + installed on the device, update the ID in mob.exs to match it: + + config :mob_dev, ios_bundle_id: "com.yourcompany.yourapp" + + (`ios_bundle_id` overrides `bundle_id` on iOS only — use it when + Android's applicationId isn't a legal Apple bundle ID.) + """} + ) + else + throw({:error, "devicectl copy failed: #{out}"}) + end end received_bootstrap = Path.join(staging_parent, "received_#{app}.beam") @@ -1209,6 +1221,11 @@ defmodule MobDev.Deployer do Process.delete(:mob_ios_override_replaced) {:ok, device} catch + # Not annotated with override state: the copy never started, so nothing + # on the device was replaced and there is no partial override to warn about. + {:skipped, reason} -> + {:skipped, reason} + {:error, reason} -> {:error, annotate_override_state(reason, app)} after diff --git a/lib/mob_dev/native_build.ex b/lib/mob_dev/native_build.ex index ea9e527..2d95a39 100644 --- a/lib/mob_dev/native_build.ex +++ b/lib/mob_dev/native_build.ex @@ -1669,7 +1669,7 @@ defmodule MobDev.NativeBuild do {:ok, sim_id} <- pick_ios_sim(device_id), binary_path = "ios/zig-out/#{display_name}", :ok <- check_path(binary_path, "iOS binary"), - {:ok, app_path} <- bundle_ios_app(binary_path, display_name), + {:ok, app_path} <- bundle_ios_app(binary_path, display_name, cfg), :ok <- copy_tflite_frameworks_ios( tflite_build, @@ -4240,7 +4240,14 @@ defmodule MobDev.NativeBuild do "Pass a full UDID or a case-insensitive prefix that matches " <> "exactly one booted sim. Run `mix mob.devices` to see what's available." - defp bundle_ios_app(binary_path, display_name) do + # The sim bundle used to keep whatever CFBundleIdentifier ios/Info.plist + # happened to carry while the device bundle stamped `ios_bundle_id/1` — two + # builds of one project installing under different ids, so + # `xcrun simctl launch ` failed and callers had to + # guess which id a given build had used. + defp bundle_ios_app(binary_path, display_name, cfg) do + bundle_id = ios_bundle_id(cfg) + build_dir = Path.join(System.tmp_dir!(), "mob_ios_bundle_#{System.unique_integer([:positive])}") @@ -4261,11 +4268,35 @@ defmodule MobDev.NativeBuild do File.cp!("ios/Info.plist", info_plist) apply_plugin_plist_keys!(info_plist) apply_fonts_to_ios_bundle!(info_plist, app_path) + plist_set!(info_plist, ":CFBundleIdentifier", bundle_id) if File.dir?("ios/Assets.xcassets/AppIcon.appiconset"), do: compile_ios_icons(app_path) + announce_bundle_id(bundle_id) {:ok, app_path} end end + # The installed bundle id is what every follow-up command needs (`xcrun + # simctl launch`, `xcrun devicectl`, `mix mob.connect`) and it appears + # nowhere else in the build output. + defp announce_bundle_id(bundle_id), + do: IO.puts(" Bundle identifier: #{IO.ANSI.cyan()}#{bundle_id}#{IO.ANSI.reset()}") + + @doc """ + The bundle id every iOS build path stamps and signs with: `mob.exs`'s + `:ios_bundle_id` when set, else `:bundle_id`. + + Single source of truth for the sim bundle, the device bundle, and code + signing — those three disagreeing is how a project ends up installed + under one id and addressed by another. Mirrors + `MobDev.Config.ios_bundle_id/0`, which is what the deploy/connect side + resolves (`cfg[:bundle_id]` is already `Config.bundle_id/0` by the time + `load_config/0` is done with it). + + Public for testing. + """ + @spec ios_bundle_id(keyword()) :: String.t() | nil + def ios_bundle_id(cfg), do: cfg[:ios_bundle_id] || cfg[:bundle_id] + defp compile_ios_icons(app_path) do actool_plist = Path.join(System.tmp_dir!(), "mob_actool_#{System.unique_integer([:positive])}.plist") @@ -4432,7 +4463,7 @@ defmodule MobDev.NativeBuild do defp bundle_ios_device_app(binary_path, otp_root, cfg, build_dir) do app_name = ios_display_name() app_module = Mix.Project.config() |> Keyword.fetch!(:app) |> Atom.to_string() - bundle_id = cfg[:ios_bundle_id] || cfg[:bundle_id] + bundle_id = ios_bundle_id(cfg) if is_nil(bundle_id), do: throw_bundle_id_error() @@ -4456,6 +4487,7 @@ defmodule MobDev.NativeBuild do plist_set!(info_plist, ":CFBundleIdentifier", bundle_id) plist_set!(info_plist, ":CFBundleExecutable", app_name) plist_set!(info_plist, ":CFBundleName", app_name) + announce_bundle_id(bundle_id) if File.dir?("ios/Assets.xcassets/AppIcon.appiconset"), do: compile_ios_device_icons(app_path) @@ -4487,6 +4519,14 @@ defmodule MobDev.NativeBuild do end defp plist_set!(plist, key, value) do + # `Set` fails outright on a key that is not already present, which for an + # adopted project's hand-written Info.plist turned a previously working + # build into a MatchError. Add first and ignore its failure when the key + # does exist — the same idiom `release.ex` uses. + System.cmd("/usr/libexec/PlistBuddy", ["-c", "Add #{key} string #{value}", plist], + stderr_to_stdout: true + ) + {_, 0} = System.cmd("/usr/libexec/PlistBuddy", ["-c", "Set #{key} #{value}", plist], stderr_to_stdout: true @@ -5805,7 +5845,7 @@ defmodule MobDev.NativeBuild do defp codesign_ios_device_app(app_path, cfg, build_dir) do sign_identity = cfg[:ios_sign_identity] team_id = cfg[:ios_team_id] - bundle_id = cfg[:ios_bundle_id] || cfg[:bundle_id] + bundle_id = ios_bundle_id(cfg) IO.puts(" === Code signing") entitlements = resolve_or_generate_entitlements(app_path, build_dir, team_id, bundle_id) @@ -6020,7 +6060,13 @@ defmodule MobDev.NativeBuild do # keychain and provisioning profile directories. Fails with a clear message only # when auto-detection itself finds multiple candidates and can't pick one. defp check_device_signing_config(cfg) do - bundle_id = cfg[:bundle_id] + # The iOS id, not the generic one. `bundle_ios_device_app/3` stamps this id + # and `codesign_ios_device_app/3` signs against the profile chosen here, so + # looking it up by the Android id searches for a profile that was never + # minted — `mix mob.provision` creates it for `ios_bundle_id`. The two ids + # often cannot be the same string, since Apple rejects the underscores + # Android's applicationId allows. + bundle_id = ios_bundle_id(cfg) with {:ok, identity} <- resolve_sign_identity(cfg[:ios_sign_identity], cfg[:ios_team_id]), {:ok, {profile_uuid, team_id}} <- diff --git a/lib/mob_dev/release.ex b/lib/mob_dev/release.ex index 00f4673..f624960 100644 --- a/lib/mob_dev/release.ex +++ b/lib/mob_dev/release.ex @@ -112,7 +112,12 @@ defmodule MobDev.Release do @doc false @spec resolve_distribution_signing(keyword()) :: {:ok, keyword()} | {:error, String.t()} def resolve_distribution_signing(cfg) do - bundle_id = cfg[:bundle_id] + # See `NativeBuild.check_device_signing_config/1`: the distribution profile + # must be found by the id the IPA is actually stamped with. Getting this + # wrong ships an App Store build under the Android applicationId, which for + # a `com.example.*` id containing an underscore App Store Connect rejects + # outright. + bundle_id = MobDev.NativeBuild.ios_bundle_id(cfg) with {:ok, identity} <- resolve_dist_identity(cfg[:ios_dist_sign_identity]), {:ok, {profile_uuid, team_id}} <- @@ -358,7 +363,7 @@ defmodule MobDev.Release do {"MOB_ELIXIR_LIB", Path.expand(elixir_lib)}, {"MOB_IOS_DEVICE_OTP_ROOT", otp_root}, {"MOB_IOS_EPMD_BUILD_SRC", epmd_src}, - {"MOB_IOS_BUNDLE_ID", cfg[:bundle_id]}, + {"MOB_IOS_BUNDLE_ID", MobDev.NativeBuild.ios_bundle_id(cfg)}, {"MOB_IOS_TEAM_ID", cfg[:ios_team_id]}, {"MOB_IOS_SIGN_IDENTITY", cfg[:ios_dist_sign_identity]}, {"MOB_IOS_PROFILE_UUID", cfg[:ios_dist_profile_uuid]}, diff --git a/lib/mob_dev/uninstaller.ex b/lib/mob_dev/uninstaller.ex index 0bb573b..64cb074 100644 --- a/lib/mob_dev/uninstaller.ex +++ b/lib/mob_dev/uninstaller.ex @@ -397,7 +397,9 @@ defmodule MobDev.Uninstaller do # ── App resolution per device ────────────────────────────────────────── - defp resolve_apps_for_device(device, opts, bundle_prefix) do + @doc false + @spec resolve_apps_for_device(Device.t(), keyword(), String.t()) :: [String.t()] + def resolve_apps_for_device(device, opts, bundle_prefix) do cond do opts[:bundle_id] -> [opts[:bundle_id]] @@ -406,10 +408,16 @@ defmodule MobDev.Uninstaller do list_matching_packages(device, bundle_prefix) true -> - [opts[:project_bundle_id] || MobDev.Config.bundle_id()] + # Per platform: an iOS device holds the app under `ios_bundle_id`, so + # uninstalling by the generic id silently removes nothing and reports + # success. Same defect class as the deploy path this release fixed. + [opts[:project_bundle_id] || default_bundle_id(device)] end end + defp default_bundle_id(%Device{platform: :ios}), do: MobDev.Config.ios_bundle_id() + defp default_bundle_id(_device), do: MobDev.Config.bundle_id() + defp list_matching_packages(%Device{platform: :android, serial: serial}, prefix) do {output, _} = System.cmd("adb", ["-s", serial, "shell", "pm", "list", "packages", prefix], diff --git a/test/mix/tasks/mob_deploy_beam_flags_test.exs b/test/mix/tasks/mob_deploy_beam_flags_test.exs index b93c53e..32c7e19 100644 --- a/test/mix/tasks/mob_deploy_beam_flags_test.exs +++ b/test/mix/tasks/mob_deploy_beam_flags_test.exs @@ -204,4 +204,68 @@ defmodule Mix.Tasks.Mob.DeployBeamFlagsTest do refute joined =~ "Failed", "Bug fix: 5 not-installed devices must NOT count as failed" end end + + # ── failure_message/3 — deploy exit status ──────────────────────────────────── + # + # Original bug: a run that printed "Failed on 1 device(s)" still exited 0, + # so CI treated a failed deploy as a success. `failure_message/3` is the + # decision behind the `Mix.raise` — it must agree with `format_summary/4` + # about which bucket means failure. + + describe "failure_message/3" do + test "nothing attempted → no failure" do + assert Deploy.failure_message([], [], []) == nil + end + + test "all deployed → no failure" do + assert Deploy.failure_message([device("iPhone")], [], []) == nil + end + + test "skipped-because-not-installed is not a failure" do + skip = device("emulator-5554", "com.example not installed on emulator-5554") + assert Deploy.failure_message([], [], [skip]) == nil + end + + test "a skipped device does not absorb a failed one" do + # The combination the real bug produces — an iPhone that failed and an + # Android emulator without the app, on one default run — and the only + # combination no test covered. Two mutations passed the whole describe + # block without it: an added `failure_message(_, _, [_ | _]), do: nil` + # clause ("any skip makes the run non-fatal"), and counting + # `length(failed) + length(skipped)`. + failed = device("iPhone", "push timed out") + skipped = device("emulator-5554", "com.example not installed on emulator-5554") + + message = Deploy.failure_message([], [failed], [skipped]) + + assert message =~ "Deploy failed on 1 device(s)", + "the skipped device must neither suppress the failure nor inflate the count" + end + + test "a failed device produces a message naming the count" do + message = Deploy.failure_message([], [device("buggy", "push timed out")], []) + assert message =~ "Deploy failed on 1 device(s)" + end + + test "partial success still fails — one bad device out of three" do + deployed = [device("iPhone"), device("emulator-5554")] + fail = device("emulator-5556", "adb push failed: broken pipe") + + assert Deploy.failure_message(deployed, [fail], []) =~ "Deploy failed on 1 device(s)" + end + + test "the iPhone bundle-id scenario from the bug report" do + # `mix mob.deploy --native --device ` installed the app, then the + # BEAM push hit the wrong bundle id. Summary said "Failed on 1", exit + # status said 0. + fail = device("Kevin's iPhone", "App 'com.example.mishka_mob' is not installed") + + assert Deploy.failure_message([], [fail], []) =~ "Deploy failed on 1 device(s)" + end + + test "counts every failed device, not just the first" do + failed = for i <- 1..3, do: device("emulator-#{i}", "adb push failed") + assert Deploy.failure_message([], failed, []) =~ "Deploy failed on 3 device(s)" + end + end end diff --git a/test/mob_dev/config_test.exs b/test/mob_dev/config_test.exs index 34ba588..276ae14 100644 --- a/test/mob_dev/config_test.exs +++ b/test/mob_dev/config_test.exs @@ -1,8 +1,40 @@ defmodule MobDev.ConfigTest do - use ExUnit.Case, async: true + # bundle-id resolution reads mob.exs / ios/Info.plist relative to the cwd, + # so these tests chdir into a fixture project. + use ExUnit.Case, async: false alias MobDev.Config + setup do + cwd = File.cwd!() + tmp = Path.join(System.tmp_dir!(), "mob_config_test_#{System.unique_integer([:positive])}") + File.mkdir_p!(tmp) + File.cd!(tmp) + + on_exit(fn -> + File.cd!(cwd) + File.rm_rf!(tmp) + end) + + {:ok, tmp: tmp} + end + + defp write_mob_exs(tmp, body), do: File.write!(Path.join(tmp, "mob.exs"), body) + + defp write_info_plist(tmp, bundle_id) do + File.mkdir_p!(Path.join(tmp, "ios")) + + File.write!(Path.join([tmp, "ios", "Info.plist"]), """ + + + + CFBundleIdentifier + #{bundle_id} + + + """) + end + describe "parse_platforms/1" do test "nil (unset) defaults to both platforms" do assert Config.parse_platforms(nil) == [:android, :ios] @@ -34,4 +66,139 @@ defmodule MobDev.ConfigTest do assert Config.parse_platforms("ios") == [:android, :ios] end end + + # ── ios_bundle_id/0 ─────────────────────────────────────────────────────────── + # + # Original bug: `MobDev.Deployer` resolved the iOS id with plain + # `bundle_id/0`, discarding `:ios_bundle_id`. The native build honoured it, + # so `mix mob.deploy --native --device ` installed the app under the + # configured id and then pushed BEAMs to an id that was never installed + # ("App '...' is not installed on this device"). + # + # The two ids genuinely differ in real projects: Android's `applicationId` + # may contain underscores (`com.example.mishka_mob`) which Apple rejects. + + describe "uninstall resolves the id per platform" do + # An iOS device holds the app under `ios_bundle_id`, so uninstalling by the + # Android applicationId removes nothing and reports success — the same + # defect class as the deploy path, and missed by the original audit. + # + # Lives here rather than in `MobDev.UninstallerTest` because resolution + # reads `mob.exs` from the current working directory, and that module is + # `async: true` — changing the cwd there would corrupt its neighbours. + setup %{tmp: tmp} do + write_mob_exs(tmp, """ + import Config + config :mob_dev, + bundle_id: "com.example.mishka_mob", + ios_bundle_id: "com.genericjam.mishkamob" + """) + + :ok + end + + test "an iOS device resolves the iOS id" do + device = %MobDev.Device{name: "iPhone", serial: "udid", platform: :ios} + + assert MobDev.Uninstaller.resolve_apps_for_device(device, [], "com.") == + ["com.genericjam.mishkamob"] + end + + test "an Android device keeps the applicationId" do + device = %MobDev.Device{name: "emu", serial: "emulator-5554", platform: :android} + + assert MobDev.Uninstaller.resolve_apps_for_device(device, [], "com.") == + ["com.example.mishka_mob"] + end + + test "an explicit --bundle-id still wins" do + device = %MobDev.Device{name: "iPhone", serial: "udid", platform: :ios} + + assert MobDev.Uninstaller.resolve_apps_for_device(device, [bundle_id: "com.o.x"], "com.") == + ["com.o.x"] + end + end + + describe "the consumers are actually wired to it (MOB-150 review S1)" do + # The resolver was tested; the WIRING was not. Reverting either consumer to + # `defp ios_bundle_id, do: bundle_id()` — the original defect, byte for + # byte, in both modules — left the entire suite green. These pin the seam + # rather than the rule. + for {mod, name} <- [{MobDev.Deployer, "Deployer"}, {MobDev.Connector, "Connector"}] do + test "#{name}.ios_bundle_id/0 resolves the iOS id, not the Android one", %{tmp: tmp} do + write_mob_exs(tmp, """ + import Config + config :mob_dev, + bundle_id: "com.example.mishka_mob", + ios_bundle_id: "com.genericjam.mishkamob" + """) + + assert unquote(mod).ios_bundle_id() == "com.genericjam.mishkamob" + + refute unquote(mod).ios_bundle_id() == MobDev.Config.bundle_id(), + "an iOS consumer resolving the Android id installs under one id " <> + "and pushes at another" + end + end + end + + describe "ios_bundle_id/0" do + test ":ios_bundle_id wins over :bundle_id", %{tmp: tmp} do + write_mob_exs(tmp, """ + import Config + config :mob_dev, + bundle_id: "com.example.mishka_mob", + ios_bundle_id: "com.genericjam.mishkamob" + """) + + assert Config.ios_bundle_id() == "com.genericjam.mishkamob" + end + + test "differs from bundle_id/0 — the Android id is left alone", %{tmp: tmp} do + write_mob_exs(tmp, """ + import Config + config :mob_dev, + bundle_id: "com.example.mishka_mob", + ios_bundle_id: "com.genericjam.mishkamob" + """) + + assert Config.bundle_id() == "com.example.mishka_mob" + refute Config.ios_bundle_id() == Config.bundle_id() + end + + test "falls back to :bundle_id when :ios_bundle_id is unset", %{tmp: tmp} do + write_mob_exs(tmp, """ + import Config + config :mob_dev, bundle_id: "com.example.shared" + """) + + assert Config.ios_bundle_id() == "com.example.shared" + end + + test "falls through the whole bundle_id/0 chain to ios/Info.plist", %{tmp: tmp} do + write_mob_exs(tmp, """ + import Config + config :mob_dev, platforms: [:ios] + """) + + write_info_plist(tmp, "com.plisted.app") + + assert Config.ios_bundle_id() == "com.plisted.app" + end + + test ":ios_bundle_id beats ios/Info.plist", %{tmp: tmp} do + write_mob_exs(tmp, """ + import Config + config :mob_dev, ios_bundle_id: "com.genericjam.mishkamob" + """) + + write_info_plist(tmp, "com.example.mishka_mob") + + assert Config.ios_bundle_id() == "com.genericjam.mishkamob" + end + + test "no mob.exs at all → same value as bundle_id/0" do + assert Config.ios_bundle_id() == Config.bundle_id() + end + end end diff --git a/test/mob_dev/ios_bundle_id_wiring_test.exs b/test/mob_dev/ios_bundle_id_wiring_test.exs new file mode 100644 index 0000000..6e5a740 --- /dev/null +++ b/test/mob_dev/ios_bundle_id_wiring_test.exs @@ -0,0 +1,108 @@ +# credo:disable-for-this-file Jump.CredoChecks.VacuousTest +# +# These assert on source text rather than calling application code, which is +# what the check is designed to catch. It is the right shape here: the paths +# guarded need a keychain, a provisioning profile or a physical device to run, +# and every one of these fixes was silently revertible before they existed. +defmodule MobDev.IosBundleIdWiringTest do + @moduledoc """ + Every iOS path must resolve its bundle id through the `:ios_bundle_id || + :bundle_id` rule. + + Reaching for the generic `bundle_id/0` on an iOS path is the defect this was + written to eliminate: the app installs under one id and everything afterwards + addresses another. It was found twice in the deploy path, and a review then + found three more sites the original audit missed. + + These are source assertions because the paths need a keychain, a provisioning + profile or a physical device to execute. The behavioural seams are tested in + `MobDev.ConfigTest`; these cover what cannot be reached from a unit test. + """ + use ExUnit.Case, async: true + + @native_build File.read!(Path.expand("../../lib/mob_dev/native_build.ex", __DIR__)) + @release File.read!(Path.expand("../../lib/mob_dev/release.ex", __DIR__)) + @deployer File.read!(Path.expand("../../lib/mob_dev/deployer.ex", __DIR__)) + @deploy_task File.read!(Path.expand("../../lib/mix/tasks/mob.deploy.ex", __DIR__)) + + defp region(source, from, to) do + unless String.contains?(source, from), do: flunk("marker not found: #{inspect(from)}") + [_, rest] = String.split(source, from, parts: 2) + unless String.contains?(rest, to), do: flunk("end marker not found: #{inspect(to)}") + [body | _] = String.split(rest, to, parts: 2) + body + end + + describe "the device build signs against a profile for the id it stamps" do + test "check_device_signing_config/1 looks the profile up by the iOS id" do + # Without this the build searches for a profile minted for the Android + # applicationId. `mix mob.provision` creates it for `ios_bundle_id`, so + # the two disagree and the build falls back to a wildcard or fails + # naming an id Apple would reject outright. + body = region(@native_build, "defp check_device_signing_config(cfg) do", "\n end") + + assert body =~ "bundle_id = ios_bundle_id(cfg)" + refute body =~ "bundle_id = cfg[:bundle_id]" + end + end + + describe "the release/IPA path" do + test "resolves the distribution profile by the iOS id" do + body = region(@release, "def resolve_distribution_signing(cfg) do", "\n end") + + assert body =~ "MobDev.NativeBuild.ios_bundle_id(cfg)" + refute body =~ "bundle_id = cfg[:bundle_id]" + end + + test "MOB_IOS_BUNDLE_ID carries the iOS id" do + # The env var is named for iOS and fed the App Store build's + # CFBundleIdentifier. Reading `:bundle_id` here stamps a submitted IPA + # with the Android applicationId — which, for the id that motivated this + # work, App Store Connect rejects for containing an underscore. + assert @release =~ ~s|{"MOB_IOS_BUNDLE_ID", MobDev.NativeBuild.ios_bundle_id(cfg)}| + refute @release =~ ~s|{"MOB_IOS_BUNDLE_ID", cfg[:bundle_id]}| + end + end + + describe "a device without the app is skipped, not failed" do + test "the iOS not-installed branch throws :skipped" do + # Android already reports this as `{:skipped, ...}` — it means "this + # device is not a target", not "the deploy failed". Once mob.deploy + # started returning a non-zero exit code, tagging the iOS case as an + # error made a plain `mix mob.deploy` fail on any Mac with an unrelated + # iPhone attached. + body = region(@deployer, "ContainerLookupErrorDomain", "devicectl copy failed") + + assert body =~ "throw(\n {:skipped," + refute body =~ "throw({:error, reason})" + end + + test "the catch clause returns it as skipped without override annotation" do + # The copy never began, so nothing on the device was replaced and there + # is no partial-override warning to attach. + body = region(@deployer, " catch\n # Not annotated", "\n after") + + assert body =~ "{:skipped, reason} ->" + refute body =~ "{:skipped, reason} ->\n {:error," + end + end + + describe "the exit code is actually wired to the decision" do + test "failure_message/3 drives a Mix.raise, after the summary" do + # `failure_message/3` is well tested as a pure function, but deleting the + # block that CALLS it left the whole suite green — the deploy would go + # back to exiting 0 on failure, the original bug, with the function that + # decides it still perfectly covered. + body = region(@deploy_task, "Enum.each(format_summary(", "\n end") + + assert body =~ "case failure_message(deployed, failed, skipped) do" + assert body =~ "message -> Mix.raise(message)" + + # Order matters: raising before the summary loses the per-device detail + # for every device in the run, which is what an operator reads. + summary_at = 0 + raise_at = :binary.match(body, "Mix.raise(message)") |> elem(0) + assert summary_at < raise_at + end + end +end diff --git a/test/mob_dev/native_build_test.exs b/test/mob_dev/native_build_test.exs index f87d6d8..1dbdae4 100644 --- a/test/mob_dev/native_build_test.exs +++ b/test/mob_dev/native_build_test.exs @@ -2218,4 +2218,35 @@ defmodule MobDev.NativeBuildTest do assert File.exists?(logo) end end + + # ── ios_bundle_id/1 ─────────────────────────────────────────────────────────── + # + # The sim bundle, the device bundle, and code signing must all stamp the + # same id. They used to disagree: the sim path never touched + # CFBundleIdentifier (so the sim app kept ios/Info.plist's value) while the + # device path stamped this one, and `MobDev.Deployer` addressed a third. + + describe "ios_bundle_id/1" do + test ":ios_bundle_id overrides :bundle_id" do + cfg = [bundle_id: "com.example.mishka_mob", ios_bundle_id: "com.genericjam.mishkamob"] + assert NativeBuild.ios_bundle_id(cfg) == "com.genericjam.mishkamob" + end + + test "falls back to :bundle_id when :ios_bundle_id is absent" do + assert NativeBuild.ios_bundle_id(bundle_id: "com.example.shared") == + "com.example.shared" + end + + test "an underscored Android applicationId does not leak into the iOS id" do + # Apple rejects `_` in a bundle id, so a project whose Android + # applicationId has one must set :ios_bundle_id — the case that + # surfaced the bug. + cfg = [bundle_id: "com.example.mishka_mob", ios_bundle_id: "com.genericjam.mishkamob"] + refute NativeBuild.ios_bundle_id(cfg) =~ "_" + end + + test "nil when neither key is set — load_config/0 always supplies :bundle_id" do + assert NativeBuild.ios_bundle_id([]) == nil + end + end end